-
Notifications
You must be signed in to change notification settings - Fork 82
chore(l1): include Merkle step in metrics logs #3630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds Merkle-tree timing to block processing logs and exposes the new AccountUpdatesList
type for storage operations. Key changes:
- Export
AccountUpdatesList
in the storage crate. - Refactored
store_block
to accept a pre-computedAccountUpdatesList
and moved Merkle-tree application outside for timing. - Enhanced
print_add_block_logs
to include a Merkle step and percentage breakdown in the metrics log.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
crates/storage/lib.rs | Added AccountUpdatesList to the public exports. |
crates/blockchain/blockchain.rs | Propagates AccountUpdatesList , captures Merkle timing, and updates log formatting with percentage breakdown. |
Comments suppressed due to low confidence (2)
crates/blockchain/blockchain.rs:423
- The log string prints
Gas Used: {:.3} ({:.0}%)
without a unit on the first value. It would be clearer to include the unit (e.g.,Ggas
) next to that number.
"[METRIC] BLOCK EXECUTION THROUGHPUT ({}): {:.3} Ggas/s TIME SPENT: {:.0} ms. Gas Used: {:.3} ({:.0}%), #Txs: {}.",
crates/blockchain/blockchain.rs:403
- The new
print_add_block_logs
method and its formatting logic aren't covered by unit tests. Consider adding tests to verify the output string and percentage calculations.
fn print_add_block_logs(
crates/blockchain/blockchain.rs
Outdated
fn percentage(init: Instant, end: Instant, total: f64) -> f64 { | ||
(end.duration_since(init).as_millis() as f64 / total * 100.0).round() | ||
} | ||
|
||
let extra_log = if as_gigas > 0.0 { | ||
format!( | ||
" exec/Ggas: {execution_time_per_gigagas} ms ({execution_fraction}%), st/Ggas: {storage_time_per_gigagas} ms ({storage_fraction}%)", | ||
" exec: {}% merkle: {}% store: {}%", | ||
percentage(since, executed, interval), | ||
percentage(executed, merkelized, interval), | ||
percentage(merkelized, stored, interval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This nested percentage
function returns an f64
and lives inside the logging method. Consider returning an integer type (e.g., u64
) for rounded percentages and moving it out to improve readability.
Copilot uses AI. Check for mistakes.
Lines of code reportTotal lines added: Detailed view
|
55d699d
to
45c01a2
Compare
45c01a2
to
0d2cb3e
Compare
In preparation for performance analysis tooling, we need to include
Merkle-tree specific measures.
Rigorously speaking, this includes most of the data preparation before
committing to the DB. The code assumes Merkle-tree operations to be the
bottleneck there.
The notebook part will be handled in a separate PR based on this branch,
so we can keep the logic in it simpler by only handling one version of
the logs at a time. It also should make it easier to review.
Based on #3274
Coauthored-by: @Arkenan
Part of: #3331