starknet_committer: add number of empty leaves to TimeMeasurementTrait#11919
Conversation
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rotem-starkware).
crates/starknet_committer/src/block_committer/commit.rs line 54 at r1 (raw file):
let n_empty_leaves = actual_storage_updates .values() .map(|value| value.values().filter(|value| value.0 == Felt::ZERO).count())
Suggestion:
|storage_entry|dfb7333 to
fbfea7b
Compare
298e403 to
1867b6b
Compare
rotem-starkware
left a comment
There was a problem hiding this comment.
@rotem-starkware made 1 comment.
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
crates/starknet_committer/src/block_committer/commit.rs line 54 at r1 (raw file):
let n_empty_leaves = actual_storage_updates .values() .map(|value| value.values().filter(|value| value.0 == Felt::ZERO).count())
Done.
1867b6b to
f2bef7a
Compare
fbfea7b to
3a69cc4
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @rotem-starkware).
a discussion (no related file):
@dorimedini-starkware Are you interested only in the number of emptied leaves in the storage?
crates/starknet_committer/src/block_committer/measurements_util.rs line 110 at r2 (raw file):
pub contracts_trie: usize, pub classes_trie: usize, pub empty_leaves: usize,
WDYT?
Suggestion:
pub emptied_storage_leaves: usize,
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 1 comment.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @rotem-starkware, and @yoavGrs).
a discussion (no related file):
Previously, yoavGrs wrote…
@dorimedini-starkware Are you interested only in the number of emptied leaves in the storage?
these are the only leaves that can be deleted.
we do not support deployed contract deletion (i.e. setting a class hash to zero in a contract leaf), or "undeclaring" a contract class in the classes tree.
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 5 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rotem-starkware).
3a69cc4 to
44247cb
Compare
f2bef7a to
2c96606
Compare
rotem-starkware
left a comment
There was a problem hiding this comment.
@rotem-starkware made 1 comment.
Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
crates/starknet_committer/src/block_committer/measurements_util.rs line 110 at r2 (raw file):
Previously, yoavGrs wrote…
WDYT?
Done.
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 4 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).
2c96606 to
d5eed12
Compare
44247cb to
e6060ff
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 6 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @rotem-starkware).
crates/starknet_committer/src/block_committer/commit.rs line 52 at r3 (raw file):
.values() .map(|storage_entry| { storage_entry.values().filter(|value| value.0 == Felt::ZERO).count()
non-blocking, but doesn't Felt have an is_zero method?
Suggestion:
filter(Felt::is_zero)crates/starknet_committer/src/block_committer/commit.rs line 62 at r3 (raw file):
classes_trie: actual_classes_updates.len(), emptied_storage_leaves: n_empty_leaves, });
probably would be nice to put this logic in it's own function, so it's clear this is all measurement-related and non-critical logic
Code quote:
// Record the number of modifications.
let n_empty_leaves = actual_storage_updates
.values()
.map(|storage_entry| {
storage_entry.values().filter(|value| value.0 == Felt::ZERO).count()
})
.sum::<usize>();
let n_storage_tries_modifications =
actual_storage_updates.values().map(|value| value.len()).sum();
measurements.set_number_of_modifications(BlockModificationsCounts {
storage_tries: n_storage_tries_modifications,
contracts_trie: n_contracts_trie_modifications,
classes_trie: actual_classes_updates.len(),
emptied_storage_leaves: n_empty_leaves,
});crates/starknet_committer_cli/src/utils.rs line 199 at r4 (raw file):
"n_contracts_trie_modifications", "n_classes_trie_modifications", "n_emptied_storage_leaves",
same backward-compatilibity question here (python scripts)
Code quote:
"n_emptied_storage_leaves",d5eed12 to
10302a6
Compare
e6060ff to
024a84f
Compare
10302a6 to
5cf5be0
Compare
rotem-starkware
left a comment
There was a problem hiding this comment.
@rotem-starkware made 3 comments.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs).
crates/starknet_committer/src/block_committer/commit.rs line 52 at r3 (raw file):
Previously, dorimedini-starkware wrote…
non-blocking, but doesn't
Felthave anis_zeromethod?
I couldn't find this method.
crates/starknet_committer/src/block_committer/commit.rs line 62 at r3 (raw file):
Previously, dorimedini-starkware wrote…
probably would be nice to put this logic in it's own function, so it's clear this is all measurement-related and non-critical logic
Done.
crates/starknet_committer_cli/src/utils.rs line 199 at r4 (raw file):
Previously, dorimedini-starkware wrote…
same backward-compatilibity question here (python scripts)
I added this to the plotting as optional
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @rotem-starkware).

No description provided.