starknet_committer: parallel reads using unordered futures#12167
starknet_committer: parallel reads using unordered futures#12167nimrod-starkware wants to merge 1 commit intonimrod/parallel-reads/sequentiallyfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
60c43c9 to
aff17da
Compare
4c3a6e9 to
db442ed
Compare
7020fae to
ab6c75f
Compare
db442ed to
25d8851
Compare
25d8851 to
c241a58
Compare
ab6c75f to
1cc055f
Compare
1cc055f to
cc3622f
Compare
c241a58 to
94e1db2
Compare
27025e8 to
927724f
Compare
5f31a49 to
1744c70
Compare
1744c70 to
5c29a32
Compare
01d2b5b to
2a03926
Compare
0b1a9e6 to
dda84d4
Compare
dda84d4 to
8c2f0f1
Compare
2a03926 to
2520ad4
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp partially reviewed 3 files and made 3 comments.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware).
crates/starknet_committer/src/db/trie_traversal.rs line 493 at r1 (raw file):
// Prepare data for the async operation. let address = *address;
remove this line and use storage_tries.insert(*address, original_skeleton); at the end of the function
crates/starknet_committer/src/db/trie_traversal.rs line 494 at r1 (raw file):
// Prepare data for the async operation. let address = *address; let cloned_storage = storage.clone();
We no longer need to clone, you can delete this line and use storage as-is below
crates/starknet_committer/src/db/trie_traversal.rs line 511 at r1 (raw file):
&leaf_modifications, None, &address,
address
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp partially reviewed 2 files, made 1 comment, and resolved 3 discussions.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @nimrod-starkware).
8c2f0f1 to
f55e97b
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 3 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on nimrod-starkware).
crates/starknet_committer/src/db/trie_traversal.rs line 0 at r3 (raw file):
without usage context, this is hard to review - you are rewriting existing trie-reading logic? copied from where?
crates/starknet_committer/src/db/trie_traversal.rs line 466 at r3 (raw file):
// TODO(Nimrod): Remove the `allow(dead_code)` once we use this function. #[allow(dead_code)] async fn create_storage_tries_concurrently<'a, Layout: NodeLayoutFor<StarknetStorageValue>>(
right?
Suggestion:
async fn create_storage_trie_original_skeletons_concurrently<crates/starknet_committer/src/db/trie_traversal.rs line 490 at r3 (raw file):
let trie_config = OriginalSkeletonTrieConfig::new_for_classes_or_storage_trie( config.warn_on_trivial_modifications(), );
this can be created outside the loop and cloned when needed, no?
Code quote:
let trie_config = OriginalSkeletonTrieConfig::new_for_classes_or_storage_trie(
config.warn_on_trivial_modifications(),
);crates/starknet_committer/src/db/trie_traversal.rs line 510 at r3 (raw file):
address, ) .await?;
makes it easier to read
Suggestion:
// Create the future - tokio will poll all futures concurrently.
futures.push(async move {
let previous_leaves = None;
let original_skeleton = create_original_skeleton_tree::<Layout::DbLeaf, Layout>(
storage,
contract_state.storage_root_hash,
sorted_leaf_indices,
&trie_config,
&leaf_modifications,
previous_leaves,
address,
)
.await?;7488c1e to
e819a39
Compare
f55e97b to
be9859b
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on nimrod-starkware).
be9859b to
096619a
Compare
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware).
crates/starknet_committer/src/db/trie_traversal.rs line 466 at r3 (raw file):
Previously, dorimedini-starkware wrote…
right?
Yes, this is correct, but in the context of this file - we only build the original skeletons. So i think renaming it would be confusing.
crates/starknet_committer/src/db/trie_traversal.rs line 490 at r3 (raw file):
Previously, dorimedini-starkware wrote…
this can be created outside the loop and cloned when needed, no?
Done.
crates/starknet_committer/src/db/trie_traversal.rs line 510 at r3 (raw file):
Previously, dorimedini-starkware wrote…
makes it easier to read
Done.
crates/starknet_committer/src/db/trie_traversal.rs line at r3 (raw file):
Previously, dorimedini-starkware wrote…
without usage context, this is hard to review - you are rewriting existing trie-reading logic? copied from where?
Sorry, I agree.
The logic is copied from the sequential version. (see prev PR).
Somewhere up in the stack I have a PR that share the logic between these two flavors. (here)
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on nimrod-starkware).
crates/starknet_committer/src/db/trie_traversal.rs line 502 at r5 (raw file):
// Create the future - tokio will poll all futures concurrently. futures.push(async move { let prevoius_leaves = None;
typo
Code quote:
let prevoius_leaves = None;
Note
Low Risk
The change is additive and currently unused, with minimal impact beyond adding a new dependency; risk is mainly around future adoption of the concurrent code path and its correctness/perf characteristics.
Overview
Adds
futuresas a new dependency forstarknet_committerand wires it into trie traversal.Introduces an (currently unused)
create_storage_tries_concurrentlyimplementation that builds per-contract storage trie skeletons in parallel viaFuturesUnordered, collecting results as they complete; existing behavior continues to use the sequential path.Written by Cursor Bugbot for commit 096619a. This will update automatically on new commits. Configure here.