-
Notifications
You must be signed in to change notification settings - Fork 30
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] Store blocks in a potentially smarter way #1335
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d49cacd
to
43a7e21
Compare
43a7e21
to
9481f0d
Compare
This comment has been minimized.
This comment has been minimized.
This reverts commit 9481f0d.
This comment has been minimized.
This comment has been minimized.
zlangley
approved these changes
Feb 3, 2025
Commit: 3589cb5 |
jonathanpwang
approved these changes
Feb 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Metric: tracegen
Before: 21246 ms
After: 19288 ms
Percentage gain: -9.2%
Source: https://github.com/axiom-crypto/openvm-reth-benchmark/actions/runs/13120710102
This resolves INT-3235.
Quote from the ticket.
Currently one actual block corresponds to several memory addresses. Whenever we modify the block itself (e.g. change the timestamp), we have to change it for every memory address it covers.
We could instead store the blocks we constructed in a separate storage (Vec), and in the positions we will store a simple usize denoting the block index in the storage. This also kind of simplifies the concept of the "default" block (we assign 0 to be a fictious index meaning "not initialized yet"), and generally makes the block data memory take less space and probably makes constructing it easier. Also, we can now only change the block of size 4 once and not touch 4 of its copies.
One drawback is that we don't remove the erased blocks from the storage, which means that we can potentially run out of memory because of this storage. On the other hand, this will probably happen about when we run out of timestamps anyway.
End of quote.
Side note: I also tried to merge a block with the next in a smarter way: instead of creating a completely new block, just modify one of the existing ones. This is worse for some reason: https://github.com/axiom-crypto/openvm-reth-benchmark/actions/runs/13122033497