Further improvements to Merkleization #2328
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 allows us to dump ValidationInput messages from a runing nitro node, or even a system test, and use them to drive the benchmarking code.
We need to be able to see how frequently the memory is being hashed and from where. The problem is that with always_merkleize off, there are thousands of fewer calls to `Merkle::root()` and this is to help us figure out where the calls are coming from. Spoiler alert: It's from the flush_module macro.
The high-level benefit of this change is a 16.5% reduction in block processing time from ~17.6s to ~14.7s. The only job of that macro was to recalculate the hash of the module and update the module merkle tree's corresponding leaf with the new hash. Instead, the waits until the hash for the module is about to be calculated and calls `Merkle::set()` for each module in the modules vector. This eliminates thousands of calls to the possibly expensive `Merkle::root()` call on the `Memory` of each module. This pattern of delaying when to actually update the leaves of merkle trees until they will be used has a lot of potential for additional performance boosts.
This change just marks memory as dirty when storing new values rather than actually calculating the new hash for the data in memory and then calculates the hashes for dirty indices just before creating the merkle tree. On one test block, this ended up saving 23 million calls to keccack.
This is to prove that the code is still fast enough to pass the test-go suite of tests if the `always_merkleize` feature is enabled.
This is now fast enough to just use it as the only implementation and simplify the code a bit.
The prover no longer runs in any other mode.
The default behavior of an Arc is to allow sharing between different threads and different instances. So, the derived behavior for cloning was to put the same references on the clone as on the orignal. But, we actually want separate clones of our merkle trees to be independent. That is, changing the data on the clone, should have no affect on the original (and vice versa.) The test added in this commit failed before the change and passes afer.
cla-bot
bot
added
the
s
gligneul
approved these changes
Jun 6, 2024
anodar
approved these changes
Jun 6, 2024
rauljordan
approved these changes
Jun 18, 2024
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.
This PR encompasses two important changes:
The
flush_module!macro is removed from per-operation calls and the same work is done just before hashing the machine.The memory type now tracks dirty indices in the memory and only sets the corresponding leaves in the merkle tree just before hashing.
These two combine to make the performance of validating a whole block ~39% faster than on merkle-perf.
Based on #2273
Related to https://linear.app/offchain-labs/issue/NIT-2411/arbitrator-optimizations