perf(trie): reserve space for new proof nodes ahead of time #15637
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
shekhirin
added
C-perf
shekhirin
changed the title
shekhirin
force-pushed
the
alexey/sparse-trie-children-reserve
branch
from
85854e7 to
af50233
Compare
shekhirin
force-pushed
the
alexey/sparse-trie-children-reserve
branch
from
af50233 to
e249a4c
Compare
shekhirin
force-pushed
the
alexey/sparse-trie-children-reserve
branch
from
93afc06 to
b6a2a7e
Compare
shekhirin
changed the title
shekhirin
force-pushed
the
alexey/sparse-trie-children-reserve
branch
from
9c95e46 to
c8dfb3f
Compare
shekhirin
force-pushed
the
alexey/sparse-trie-children-reserve
branch
from
c8dfb3f to
060f965
Compare
fgimenez
reviewed
Apr 10, 2025
| skipped_nodes: u64, | ||
| /// Number of new nodes that will be revealed. This includes all children of branch nodes, even | ||
| /// if they are not in the proof. | ||
| new_nodes: usize, |
There was a problem hiding this comment.
why is new_nodes usize and total_nodes u64? according to the logic, always new_nodes >= total_nodes. i guess it is bc new_nodes is used to reserve, why not everything usize?
There was a problem hiding this comment.
was just more convenient to return u64 because it's reported in metrics as u64, but think it's nicer to have consistency in types, so fixed
|
|
||
| /// Decodes the proof nodes returning additional information about the number of total, skipped, and | ||
| /// new nodes. | ||
| fn decode_proof_nodes( |
There was a problem hiding this comment.
can we have a unit test for this?
| @@ -36,24 +36,24 @@ impl SparseStateTrieMetrics { | |||
| .record(self.multiproof_total_storage_nodes as f64); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
should we have new_nodes metrics too, same as skipped and total nodes?
rkrasiuk
requested changes
Apr 10, 2025
Comment on lines
+284
to
+287
| let DecodedProofNodes { nodes, total_nodes, skipped_nodes, new_nodes } = | ||
| decode_proof_nodes(account_subtree, &self.revealed_account_paths)?; | ||
| self.metrics.increment_total_account_nodes(total_nodes as u64); | ||
| self.metrics.increment_skipped_account_nodes(skipped_nodes as u64); |
There was a problem hiding this comment.
let's pass metrics to fn and avoid returning that in the result
There was a problem hiding this comment.
state and storage tries use different metrics
Comment on lines
+791
to
+797
| /// Number of nodes in the proof. | ||
| total_nodes: usize, | ||
| /// Number of nodes that were skipped because they were already revealed. | ||
| skipped_nodes: usize, | ||
| /// Number of new nodes that will be revealed. This includes all children of branch nodes, even | ||
| /// if they are not in the proof. | ||
| new_nodes: usize, |
There was a problem hiding this comment.
new_nodes is needed for reserving, the rest is metrics
rkrasiuk
approved these changes
Apr 10, 2025
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.
Problem
When revealing multiproofs, we insert new nodes into the
nodeshashmap of theRevealedSparseTriestruct. If there's not enough capacity in the hashmap, it allocates new memory with more space, copies old elements into this memory, and inserts a new element. This can happen multiple times when revealing the proof, because proofs can be large.Solution
Iterate over the proof nodes once, count the number of new nodes, reserve the space in the nodes hashmap ahead of time for them, and proceed as before.
Before
After