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

[WIP] implement subtree-based SMT computations #341

Merged
merged 20 commits into from
Dec 4, 2024

Conversation

Qyriad
Copy link
Contributor

@Qyriad Qyriad commented Nov 4, 2024

Describe your changes

This is the working state for using the methods benchmarked in #334 to make the computations necessary to construct a new sparse Merkle tree from existing data, by splitting the work across fixed-size subtrees.

This is not ready to be merged, but does currently have passing tests for a singlethreaded version of the desired functionality.

@Qyriad Qyriad force-pushed the qyriad/parallel-construction branch from 0249e63 to ebc975f Compare November 15, 2024 02:49
@Qyriad Qyriad marked this pull request as ready for review November 15, 2024 02:51
@Qyriad
Copy link
Contributor Author

Qyriad commented Nov 15, 2024

This should now be ready for review!

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! This is a pretty superficial review as I'm currently traveling.

I left a couple of comments inline. The main one is that this crate should work in no_std context, and so we should make rayon an optional dependency behind a feature flag. This would also imply that rather than having separate functions for parallel and sequential execution, we should have just single versions which would contain parallelized code when concurrent feature is turned on.

Also, could you share the overall results of these optimizations? (i.e., for constructing trees of 2^16 and 2^20 key-value pairs)?

src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/merkle/smt/full/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/full/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
@Qyriad
Copy link
Contributor Author

Qyriad commented Nov 15, 2024

Benchmark comparison for 2^16 (65,536) leaves on my Ryzen 7950X:

Running a construction benchmark:
Constructed a SMT with 65536 key-value pairs in 22.326 seconds
Number of leaf nodes: 65536

Running a parallel construction benchmark:
Parallel-constructed an SMT with 65536 key-value pairs in 1.675 seconds
Number of leaf nodes: 65536

Benchmark comparison for 2^20 (1,048,576) leaves:

Running a construction benchmark:
Constructed a SMT with 1048576 key-value pairs in 366.505 seconds
Number of leaf nodes: 1048576

Running a parallel construction benchmark:
Parallel-constructed an SMT with 1048576 key-value pairs in 24.555 seconds
Number of leaf nodes: 1048576

I also have a graph! The green line is the singlethreaded version and the purple line is the multithreaded version.
Screenshot_20241115_115449

This will be composed into depth-8-subtree-based computation of entire
sparse Merkle trees.
This is intended for comparison with the benchmarks from the previous
commit. This benchmark represents the theoretical perfect-efficiency
performance we could possibly (but impractically) get for computing
depth-8 sparse Merkle subtrees.
This commit ensures that `SparseMerkleTree::build_subtree()` can
correctly compose into building an entire sparse Merkle tree, without
yet getting into potential complications concurrency introduces.
Building on the previous commit, this commit implements a test proving
that `SparseMerkleTree::build_subtree()` can be composed into itself not
just concurrently, but in parallel, without issue.
This commit adds a new required method to the SparseMerkleTree trait,
to allow generic construction from pre-computed parts.

This will be used to add a generic version of `with_entries()` in a
later commit.
What the previous few commits have been leading up to: SparseMerkleTree
now has a function to construct the tree from existing data in parallel.
This is significantly faster than the singlethreaded equivalent.
Benchmarks incoming!
@Qyriad Qyriad force-pushed the qyriad/parallel-construction branch from ebc975f to 3f52ef3 Compare November 15, 2024 19:45
@bobbinth
Copy link
Contributor

Benchmark comparison for 2^16 (65,536) leaves on my Ryzen 7950X:

Running a construction benchmark:
Constructed a SMT with 65536 key-value pairs in 22.326 seconds
Number of leaf nodes: 65536

Running a parallel construction benchmark:
Parallel-constructed an SMT with 65536 key-value pairs in 1.675 seconds
Number of leaf nodes: 65536

Benchmark comparison for 2^20 (1,048,576) leaves:

Running a construction benchmark:
Constructed a SMT with 1048576 key-value pairs in 366.505 seconds
Number of leaf nodes: 1048576

Running a parallel construction benchmark:
Parallel-constructed an SMT with 1048576 key-value pairs in 24.555 seconds
Number of leaf nodes: 1048576

Very cool! And also, AFAIK, Ryzen 7950X has only 16 cores - so, we are getting nearly perfect multi-threaded performance.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! Not a full review yet, but I left some comments inline.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/merkle/smt/full/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/simple/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
@krushimir krushimir force-pushed the qyriad/parallel-construction branch from 58fa0a0 to c9b4682 Compare November 22, 2024 21:48
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Still not a full review, but I left some more comments inline.

Also, what do you think about #341 (comment) ?

src/merkle/smt/full/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/simple/mod.rs Outdated Show resolved Hide resolved
src/merkle/store/tests.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/merkle/smt/tests.rs Outdated Show resolved Hide resolved
@krushimir krushimir force-pushed the qyriad/parallel-construction branch from 8b3c1bf to 6d93c0d Compare November 26, 2024 15:47
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! Unfortunately, still not a full review - but I left a couple of comments inline.

Also, we should update the README to add concurrent feature description to it.

src/main.rs Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you. Finally, a full review. I left some more comments inline.

README.md Outdated Show resolved Hide resolved
benches/parallel-subtree.rs Outdated Show resolved Hide resolved
benches/parallel-subtree.rs Outdated Show resolved Hide resolved
src/merkle/smt/full/mod.rs Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/full/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Show resolved Hide resolved
src/merkle/smt/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple small comments inline. After these are addressed, we should be good to merge.

Also, we should update the bench command in the Makefile (i.e., here) to run with the concurrent feature enabled.

Cargo.toml Outdated Show resolved Hide resolved
src/merkle/smt/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

@bobbinth
Copy link
Contributor

bobbinth commented Dec 3, 2024

@krushimir - seems like there are some conflicts with the next branch. Could you resolve this (via a merge commit, if possible)?

@bobbinth
Copy link
Contributor

bobbinth commented Dec 3, 2024

@krushimir - seems like some lint checks are failing. Could you fix these? (running make lint should fix them).

Copy link

sonarqubecloud bot commented Dec 4, 2024

@bobbinth bobbinth merged commit b151773 into 0xPolygonMiden:next Dec 4, 2024
13 checks passed
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