chore!: remove mutable mmr #5954
Conversation
ghpbot-tari-project
added
the
CR-insufficient_context
ghpbot-tari-project
added
P-acks_required
ghpbot-tari-project
removed
the
P-reviews_required
|
It's a breaking change, so will need a major version change on the crate at next release. Edit: though it looks like we haven't published it in a while: https://crates.io/crates/tari_mmr |
Yeah its synced to the tari version atm which is 0.53.0 |
| t | ||
| } | ||
|
|
||
| pub fn root_hash(_c: &mut Criterion) { |
There was a problem hiding this comment.
Can these operations be refactored to use Criterion's bench_function design? It might get a little tricky because of how its iterations work, since you want to work on a fresh tree for each of its iterations to ensure the operations are being run as expected. But at least then you can probably get better timing data.
| @@ -19,7 +19,7 @@ fn create_smt() -> SparseMerkleTree<Blake2b<U32>> { | |||
| SparseMerkleTree::<Blake2b<U32>>::new() | |||
| } | |||
|
|
|||
| pub fn benchmark_sparse_merkle_trees(c: &mut Criterion) { | |||
| pub fn benchmark_smt_insert(c: &mut Criterion) { | |||
There was a problem hiding this comment.
Looking at this more closely, I think there's a subtle error in this benchmark. A fresh tree is created for each sample, but then each iteration of that sample applies the upsert operations to the same tree. This means the first iteration will perform inserts, and subsequent iterations will perform updates. The timing data will only be accurate if an insert and update take the same amount of time.
|
Since rewriting the benchmarks to use a more standard Criterion design might take a bit, should this be merged as is? Benchmarks could be improved separately. |
|
Fixes and updates to the benchmarks are addressed in #5962. |
Description --- Updates and fixes Merkle benchmarks. Closes #5962. Motivation and Context --- Recent work in #5954 removes mutable Merkle mountain range (MMR) code, which became unused with the addition of sparse Merkle trees (SMTs). Some existing benchmarks were removed or moved to accommodate this change. During review, some minor issues were identified: - An existing MMR and SMT benchmark reused tree structures between benchmark iterations, which could provide incorrect timing data. - New SMT benchmarks used bespoke timing code that didn't take full advantage of Criterion's functionality. This PR fixes these issues, albeit at the expense of more benchmark setup overhead. This shouldn't be particularly problematic, as benchmarks are only run manually as needed. How Has This Been Tested? --- The benchmarks run and appear to give reasonable data. What process can a PR reviewer use to test or verify this change? --- Check that the updated and new benchmarks exercise the desired functionality. Check that operations use fresh tree structures for each test iteration as needed, to ensure correct timing data.
Description
removes croaring and the mutable mmr struct
Motivation and Context
There are not used anymore