-
Notifications
You must be signed in to change notification settings - Fork 7
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
126 speedup ammr #132
126 speedup ammr #132
Conversation
4d568b2
to
dbeb83e
Compare
When appending a leaf to an MMR, we have the new leaf's authentication path as we are building the MMR. So there's no reason to throw the auth path away and then get the auth path later in a separate call. This commit builds the auth path exactly once, and not twice as it was done before. It also avoids unnecessary recursion. This commit provides a more than 2x speedup for the mean and median case, a 1.3x speedup for the slowest case and a more than 30x(!) speedup for cases where the appends are followed by a `persist` to the DB.
Remove auth path arg from input to speed up the function for mutating a leaf in an AMMR. Speeds up the associated benchmark by ~20 % Old mutate code: ╰─ mutate │ │ │ │ │ ╰─ mutate_100_of_10000 │ │ │ │ │ ├─ leaf_mutation 1.942 ms │ 2.435 ms │ 1.952 ms │ 1.991 ms │ 100 │ 100 ╰─ leaf_mutation_and_persist 34.1 ms │ 55.25 ms │ 39.7 ms │ 40.91 ms │ 100 │ 100 new mutate code: ╰─ mutate │ │ │ │ │ ╰─ mutate_100_of_10000 │ │ │ │ │ ├─ leaf_mutation 1.607 ms │ 1.922 ms │ 1.613 ms │ 1.656 ms │ 100 │ 100 ╰─ leaf_mutation_and_persist 20.36 ms │ 27.2 ms │ 22.44 ms │ 22.73 ms │ 100 │ 100
Since the function interface was changed, we no longer need to construct the AMMR membership proof for each loop-iteration in the benchmark.
…or archival-MMR This interface is changed as the MMR-accumulator and the achival-MMR no longer share an interface, meaning that they can have separate function signatures. As the archival-MMR does not need the membership-proofs of the leaves that are mutated, this abstract arugment is removed.
No real noticable in performance, but this interface was not well liked, so we remove the peaks from the return value. In theory this should be slightly faster, as a step of the calculation is skipped, and since the peak digests no longer need to be read from the database.
With this commit the DB is only accessed once when counting leaves, whereas earlier, a helper function that read multiple digests from the MMRA was used. Also changes index arithmetic for getting the relevant node indices for an MMR authentication path slightly, and then reads all the digests in one go instead of in a loop. Good speedup seen from this commit: - batch-mutation benchmark is 10 % faster, regular mutation is 20 % faster.
dbeb83e
to
0c5b017
Compare
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.
Looks like a very good change, and I'm happy to see the benchmarks. nice!
I just had the one q about the new ArchivalMmr::append() impl.
self.prove_membership_async(leaf_index).await.0 | ||
let mut node_index = self.digests.len().await; | ||
let leaf_index = node_index_to_leaf_index(node_index).unwrap(); | ||
let right_lineage_length = right_lineage_length_from_leaf_index(leaf_index); |
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.
It seems we could just call prove_membership_async()
as the old code did, and it would now be faster because it no longer returns peaks.
I assume this code is not doing that because prove_membership_async()
would still be doing work that we don't need done here, is that correct?
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.
Yes. Here, we use each instance of left_sibling_hash
for two purposes: For finding the parent digest in the Merkle tree, and as an element in the authentication path that append
returns. Calling prove_membership_async
would in essence mean we fetched the exact same digests twice.
Some optimizations of the archival-MMR which is used extensively by the archival mutator set.
The method
prove_membership_async
changes its interface to only return the authentication path (membership proof) and not also the peaks of the MMR. If the peaks are needed, they can be requested separately.append
is more than 2x faster, and more than 10x (30x in one measurement) faster for theappend_and_persist
benchmark. The other benchmarks show improvements of around 20-50 %.append
andbatch_mutate_leaf_and_update_mps
are the most relevant methods, as they are used by the archival mutator set which is what we're aiming to speed up in #127.Previous code:
This PR:
This closes #126.