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] Store blocks in a potentially smarter way #1335

Merged
merged 5 commits into from
Feb 4, 2025

Conversation

Golovanov399
Copy link
Contributor

@Golovanov399 Golovanov399 commented Feb 3, 2025

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

This comment has been minimized.

This comment has been minimized.

@Golovanov399 Golovanov399 force-pushed the perf/offline-memory-block-storage branch 2 times, most recently from d49cacd to 43a7e21 Compare February 3, 2025 20:14
@Golovanov399 Golovanov399 force-pushed the perf/offline-memory-block-storage branch from 43a7e21 to 9481f0d Compare February 3, 2025 20:14

This comment has been minimized.

This reverts commit 9481f0d.

This comment has been minimized.

crates/vm/src/system/memory/offline.rs Show resolved Hide resolved
crates/vm/src/system/memory/offline.rs Outdated Show resolved Hide resolved
crates/vm/src/system/memory/offline.rs Show resolved Hide resolved
Copy link

github-actions bot commented Feb 3, 2025

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (+31 [+1.4%]) 2,171 513,879 18,711,232 - - -
fibonacci_program (-50 [-1.0%]) 4,950 1,500,095 51,485,080 - - -
regex_program (+60 [+0.4%]) 14,853 1,914,103 165,455,373 - - -
ecrecover_program (-11 [-0.4%]) 2,523 284,567 15,055,723 - - -

Commit: 3589cb5

Benchmark Workflow

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

Nice

@jonathanpwang jonathanpwang merged commit 57d11cb into main Feb 4, 2025
22 checks passed
@jonathanpwang jonathanpwang deleted the perf/offline-memory-block-storage branch February 4, 2025 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants