Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: avoid copy of period locks in epoch #7192

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Dec 22, 2023

Closes: #XXX

What is the purpose of the change

This speeds up epoch by avoiding copying stemming from period locks

New:
image

Old:
image

Testing and Verifying

Synched over epoch + 150 blocks.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@p0mvn p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v21.x backport patches to v21.x branch labels Dec 22, 2023
@p0mvn p0mvn marked this pull request as ready for review December 22, 2023 21:34
for _, lock := range locks {
if lock.Duration >= minDuration {
filteredLocks = append(filteredLocks, lock)
filteredLocks = append(filteredLocks, &lock)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? I thought this would have every filtered lock point to the same loop variable, not the copies in the original slice. (Which would be &locks[i])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It synched through epoch but I applied the suggestion and rerunning pprof

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like my suggstion doesnt work by go tests failing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force pushed an update that I think should work. Need to reprofile it now as it leads to having to take a copy of the pointer in another loop

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the bug fix:
image

Still much better overall. I will now do a long-lasting test as opposed to epoch only

@p0mvn p0mvn marked this pull request as draft December 22, 2023 21:43
@p0mvn p0mvn marked this pull request as ready for review December 22, 2023 22:41
@p0mvn
Copy link
Member Author

p0mvn commented Dec 22, 2023

Synching over many blocks. Will merge once ready

@p0mvn p0mvn merged commit 8d2621d into main Dec 22, 2023
1 check passed
@p0mvn p0mvn deleted the roman/avoid-copy-epoch branch December 22, 2023 23:09
mergify bot pushed a commit that referenced this pull request Dec 22, 2023
(cherry picked from commit 8d2621d)
p0mvn added a commit that referenced this pull request Dec 22, 2023
(cherry picked from commit 8d2621d)

Co-authored-by: Roman <roman@osmosis.team>
p0mvn added a commit that referenced this pull request Dec 23, 2023
@p0mvn p0mvn mentioned this pull request Dec 23, 2023
p0mvn added a commit that referenced this pull request Dec 23, 2023
This reverts commit 6cc26f9.
p0mvn added a commit that referenced this pull request Dec 23, 2023
p0mvn added a commit that referenced this pull request Dec 23, 2023
* updates (#7192)

* refactor: auto-set timeout-commit to 4s for faster block times (backport #7180)

* changelog

* Revert "updates (#7192)"

This reverts commit 6cc26f9.

* lint
czarcas7ic pushed a commit that referenced this pull request Dec 29, 2023
* updates (#7192)

* refactor: auto-set timeout-commit to 4s for faster block times (backport #7180)

* changelog

* Revert "updates (#7192)"

This reverts commit 6cc26f9.

* lint
czarcas7ic added a commit that referenced this pull request Dec 30, 2023
* Reduce discrepancy in fee simulation

* Update

* State compatible applyFuncIfNoError event reduction

* Update osmoutils/epochs versions

* Update SDK version

* One more epoch speedup

* Speedup core loop of epoch

* fix test (#7197)

* refactor: auto-set timeout-commit to 4s for faster block times (#7196)

* updates (#7192)

* refactor: auto-set timeout-commit to 4s for faster block times (backport #7180)

* changelog

* Revert "updates (#7192)"

This reverts commit 6cc26f9.

* lint

* Update changelog

* Update changelog

* changelog

* Speedup multiplication

* Try scratch slice

* refactor: arb filter for new authz exec swap (#7210)

* refactor: arb filter for new authz exec swap

* updates

* updates

* updates

* clean up

* Auto: update go.mod after push to adam/v21.1.x-merge-to-main that modified dependencies locally

* add 21.1.5 to changelog

* update comment

---------

Co-authored-by: Dev Ojha <dojha@berkeley.edu>
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: github-actions <github-actions@github.com>
mergify bot pushed a commit that referenced this pull request Dec 30, 2023
* Reduce discrepancy in fee simulation

* Update

* State compatible applyFuncIfNoError event reduction

* Update osmoutils/epochs versions

* Update SDK version

* One more epoch speedup

* Speedup core loop of epoch

* fix test (#7197)

* refactor: auto-set timeout-commit to 4s for faster block times (#7196)

* updates (#7192)

* refactor: auto-set timeout-commit to 4s for faster block times (backport #7180)

* changelog

* Revert "updates (#7192)"

This reverts commit 6cc26f9.

* lint

* Update changelog

* Update changelog

* changelog

* Speedup multiplication

* Try scratch slice

* refactor: arb filter for new authz exec swap (#7210)

* refactor: arb filter for new authz exec swap

* updates

* updates

* updates

* clean up

* Auto: update go.mod after push to adam/v21.1.x-merge-to-main that modified dependencies locally

* add 21.1.5 to changelog

* update comment

---------

Co-authored-by: Dev Ojha <dojha@berkeley.edu>
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: github-actions <github-actions@github.com>
(cherry picked from commit c5a5147)

# Conflicts:
#	go.mod
#	go.sum
#	osmomath/go.sum
czarcas7ic added a commit that referenced this pull request Dec 30, 2023
* chore: catch up main to v21.1.x (#7215)

* Reduce discrepancy in fee simulation

* Update

* State compatible applyFuncIfNoError event reduction

* Update osmoutils/epochs versions

* Update SDK version

* One more epoch speedup

* Speedup core loop of epoch

* fix test (#7197)

* refactor: auto-set timeout-commit to 4s for faster block times (#7196)

* updates (#7192)

* refactor: auto-set timeout-commit to 4s for faster block times (backport #7180)

* changelog

* Revert "updates (#7192)"

This reverts commit 6cc26f9.

* lint

* Update changelog

* Update changelog

* changelog

* Speedup multiplication

* Try scratch slice

* refactor: arb filter for new authz exec swap (#7210)

* refactor: arb filter for new authz exec swap

* updates

* updates

* updates

* clean up

* Auto: update go.mod after push to adam/v21.1.x-merge-to-main that modified dependencies locally

* add 21.1.5 to changelog

* update comment

---------

Co-authored-by: Dev Ojha <dojha@berkeley.edu>
Co-authored-by: Roman <roman@osmosis.team>
Co-authored-by: github-actions <github-actions@github.com>
(cherry picked from commit c5a5147)

# Conflicts:
#	go.mod
#	go.sum
#	osmomath/go.sum

* go mod update

* update go mods from v21.x

---------

Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
p0mvn added a commit that referenced this pull request Jan 3, 2024
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v21.x backport patches to v21.x branch A:no-changelog C:x/incentives C:x/lockup V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants