starknet_committer: add number of modifications to BlockMeasurement and impl set function#11917
Conversation
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @rotem-starkware).
crates/starknet_committer/src/block_committer/timing_util.rs line 106 at r1 (raw file):
pub n_contracts_trie_modifications: usize, pub n_classes_trie_modifications: usize, }
Consider dividing into sub-structs.
non-blocking
Suggestion:
pub block_duration: BlockDuration,
pub block_modifications: BlockModifications,
}crates/starknet_committer/src/block_committer/commit.rs line 45 at r1 (raw file):
let n_storage_tries_modifications = actual_storage_updates.values().map(|value| value.len()).sum(); time_measurement.set_number_of_modifications(
It's not only about timing...
Do you plan to rename it and timing_util.rs? (not in this PR)
Or separating timing from counting?
Suggestion:
measurements79a6e56 to
efa049b
Compare
7ed6325 to
9e72203
Compare
rotem-starkware
left a comment
There was a problem hiding this comment.
@rotem-starkware made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
crates/starknet_committer/src/block_committer/commit.rs line 45 at r1 (raw file):
Previously, yoavGrs wrote…
It's not only about timing...
Do you plan to rename it andtiming_util.rs? (not in this PR)
Or separating timing from counting?
Done, in PR #11970
crates/starknet_committer/src/block_committer/timing_util.rs line 106 at r1 (raw file):
Previously, yoavGrs wrote…
Consider dividing into sub-structs.
non-blocking
Done, in separate PRs
9e72203 to
f353436
Compare
efa049b to
ed7f57c
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs partially reviewed 4 files, made 1 comment, and resolved 1 discussion.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @rotem-starkware).
crates/starknet_committer/src/block_committer/measurements_util.rs line 69 at r2 (raw file):
n_storage_tries_modifications: usize, n_contracts_trie_modifications: usize, n_classes_trie_modifications: usize,
Suggestion:
block_modificationsC_counts: BlockModificationsCounts,ed7f57c to
87774bf
Compare
f353436 to
3f611bf
Compare
rotem-starkware
left a comment
There was a problem hiding this comment.
@rotem-starkware made 1 comment.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
crates/starknet_committer/src/block_committer/measurements_util.rs line 69 at r2 (raw file):
n_storage_tries_modifications: usize, n_contracts_trie_modifications: usize, n_classes_trie_modifications: usize,
Done.
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 6 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @rotem-starkware).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rotem-starkware).
crates/starknet_committer_cli/src/utils.rs line 198 at r4 (raw file):
"n_storage_tries_modifications", "n_contracts_trie_modifications", "n_classes_trie_modifications",
do you need to update the python scripts for these new columns?
any backward-compatibility issues?
Code quote:
"n_storage_tries_modifications",
"n_contracts_trie_modifications",
"n_classes_trie_modifications",87774bf to
8e71adc
Compare
3f611bf to
5387965
Compare
rotem-starkware
left a comment
There was a problem hiding this comment.
@rotem-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware).
crates/starknet_committer_cli/src/utils.rs line 198 at r4 (raw file):
Previously, dorimedini-starkware wrote…
do you need to update the python scripts for these new columns?
any backward-compatibility issues?
I actually didn't add plotting of those values, but that's a good idea.
PTAL at the next PR in the stack.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @rotem-starkware).
…nd impl set function
5387965 to
295fcc5
Compare
8e71adc to
104d43b
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @rotem-starkware).
010e55d

No description provided.