Skip to content

Conversation

TzahiTaub
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented Sep 17, 2025

Copy link
Contributor Author

TzahiTaub commented Sep 17, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@TzahiTaub TzahiTaub self-assigned this Sep 17, 2025
@TzahiTaub TzahiTaub marked this pull request as ready for review September 17, 2025 12:10
Copy link

github-actions bot commented Sep 17, 2025

Benchmark movements: No major performance changes detected.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorimedini-starkware reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @TzahiTaub)


crates/starknet_committer_cli/src/commands.rs line 1 at r1 (raw file):

#![allow(clippy::as_conversions)]

I prefer you mark each line you need a conversion... maybe we can mitigate some with use of Ratio<u128>

Code quote:

#![allow(clippy::as_conversions)]

crates/starknet_committer_cli/src/commands.rs line 3 at r1 (raw file):

#![allow(clippy::as_conversions)]

use std::fs::{self, File};

self?

Code quote:

use std::fs::{self, File}

crates/starknet_committer_cli/src/commands.rs line 17 at r1 (raw file):

pub type InputImpl = Input<ConfigImpl>;
struct TimeMeasurement {

newline between blocks

Suggestion:

pub type InputImpl = Input<ConfigImpl>;

struct TimeMeasurement {

crates/starknet_committer_cli/src/commands.rs line 58 at r1 (raw file):

        self.n_facts.push(facts_count);
        self.facts_in_db.push(self.total_facts);
        self.total_facts += facts_count;

Suggestion:

        let duration =
            self.timer.expect("stop_measurement called before start_measurement").elapsed();
        let millis = duration.as_millis();
        info!(
            "Time elapsed for iteration {}: {} milliseconds",
            self.n_results(),
            duration.as_millis()
        );
        self.total_time += millis;
        self.per_fact_durations
            .push(duration.div_f32(facts_count as f32).as_micros().try_into().unwrap());
        self.block_durations.push(millis.try_into().unwrap());
        self.n_facts.push(facts_count);
        self.facts_in_db.push(self.total_facts);
        self.total_facts += facts_count;

crates/starknet_committer_cli/src/commands.rs line 58 at r1 (raw file):

        self.n_facts.push(facts_count);
        self.facts_in_db.push(self.total_facts);
        self.total_facts += facts_count;

shouldn't these two lines be in the opposite order?

Code quote:

        self.facts_in_db.push(self.total_facts);
        self.total_facts += facts_count;

crates/starknet_committer_cli/src/commands.rs line 98 at r1 (raw file):

            self.n_results()
        );
        println!("Average block time: {:.2} milliseconds", self.block_average_time());

Suggestion:

        println!(
            "Total time: {} milliseconds for {} iterations.",
            self.total_time,
            self.n_results()
        );
        println!("Average block time: {:.2} milliseconds.", self.block_average_time());

crates/starknet_committer_cli/src/commands.rs line 107 at r1 (raw file):

            let norm = fact_duration / max;
            let width = (norm * 40.0).round() as usize; // up tp 40 characters wide
            let bar = "█".repeat(width.max(1));

what's this magic box of green?

Code quote:

let bar = "█".repeat(width.max(1));

crates/starknet_committer_cli/src/commands.rs line 113 at r1 (raw file):

    fn to_csv(&self, path: &str, output_dir: &str) {
        let _ = fs::create_dir_all(output_dir);

why do you need the _ variable?

Code quote:

let _ = fs::create_dir_all(output_dir);

crates/starknet_committer_cli/src/commands.rs line 146 at r1 (raw file):

}

pub async fn run_storage_benchmark(n_iterations: usize, output_dir: &str) {

docstring? seems like the main API of this module

Code quote:

pub async fn run_storage_benchmark(n_iterations: usize, output_dir: &str) {

crates/starknet_committer_cli/src/commands.rs line 147 at r1 (raw file):

pub async fn run_storage_benchmark(n_iterations: usize, output_dir: &str) {
    let seed = 42_u64; // Constant seed for reproducibility

maybe this should be input? so users can rerun with different randomness

Code quote:

let seed = 42_u64; // Constant seed for reproducibility

crates/starknet_committer_cli/src/commands.rs line 154 at r1 (raw file):

    let mut storage = MapStorage::default();
    let mut contracts_trie_root_hash = HashOutput::default();
    let mut classes_trie_root_hash = HashOutput::default();

in future PRs, these three will be inputs to the function?
i.e. I will be able to

  1. choose a specific initial state (maybe with existing facts and history)?
  2. choose a different storage impl?

Code quote:

    let mut storage = MapStorage::default();
    let mut contracts_trie_root_hash = HashOutput::default();
    let mut classes_trie_root_hash = HashOutput::default();

crates/starknet_committer_cli/src/commands.rs line 170 at r1 (raw file):

        let n_new_facts = filled_forest.write_to_storage(&mut storage);

        time_measurement.stop_measurement(n_new_facts);

for subsequent PRs: would be interesting to get separate time measurements for these two phases (compute, and then write)

Code quote:

        time_measurement.start_measurement();
        let filled_forest =
            commit_block(input, &mut storage).await.expect("Failed to commit the given block.");
        let n_new_facts = filled_forest.write_to_storage(&mut storage);

        time_measurement.stop_measurement(n_new_facts);

crates/starknet_committer_cli/Cargo.toml line 6 at r1 (raw file):

edition.workspace = true
repository.workspace = true
license-file.workspace = true

we are trying to delete the license file and just keep the license name

Suggestion:

license.workspace = true

Cargo.toml line 205 at r1 (raw file):

const_format = "0.2.30"
criterion = "0.5.1"
csv = "1.3.1"

make sure semgrep passes on this PR (when adding new deps it's important)

Code quote:

csv = "1.3.1"

crates/starknet_committer_cli/src/main.rs line 48 at r1 (raw file):

            log_filter_handle
                .modify(|filter| *filter = level.into())
                .expect("Failed to set the log level.");

extract to a handle_log_level(level: String, log_filter_handle: Handle<LevelFilter, Registry) function please

Code quote:

            let level = match args.log_level.to_lowercase().as_str() {
                "error" => Level::ERROR,
                "warn" => Level::WARN,
                "info" => Level::INFO,
                "debug" => Level::DEBUG,
                "trace" => Level::TRACE,
                _ => Level::INFO,
            };
            log_filter_handle
                .modify(|filter| *filter = level.into())
                .expect("Failed to set the log level.");

@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from 114e111 to fd1fbdd Compare September 17, 2025 14:38
Copy link
Contributor Author

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dorimedini-starkware)


crates/starknet_committer_cli/Cargo.toml line 6 at r1 (raw file):

Previously, dorimedini-starkware wrote…

we are trying to delete the license file and just keep the license name

Done.


crates/starknet_committer_cli/src/commands.rs line 3 at r1 (raw file):

Previously, dorimedini-starkware wrote…

self?

Apperantly it's a way to import the fs crate/lib module + an inner module in a single use command. Needed for the later fs::create_dir_all


crates/starknet_committer_cli/src/commands.rs line 17 at r1 (raw file):

Previously, dorimedini-starkware wrote…

newline between blocks

Done.


crates/starknet_committer_cli/src/commands.rs line 58 at r1 (raw file):

Previously, dorimedini-starkware wrote…

shouldn't these two lines be in the opposite order?

No, I want the total facts to have the number of facts before the block (for the csv facts_in_db column). Added a comment


crates/starknet_committer_cli/src/commands.rs line 107 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what's this magic box of green?

It's a hand-made visualization @yoavGrs did.
Run the script and you'll see the magic
image.png


crates/starknet_committer_cli/src/commands.rs line 113 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why do you need the _ variable?

Some auto fixes (there's a result returned), replaced with expect.


crates/starknet_committer_cli/src/commands.rs line 146 at r1 (raw file):

Previously, dorimedini-starkware wrote…

docstring? seems like the main API of this module

Done.


crates/starknet_committer_cli/src/commands.rs line 154 at r1 (raw file):

Previously, dorimedini-starkware wrote…

in future PRs, these three will be inputs to the function?
i.e. I will be able to

  1. choose a specific initial state (maybe with existing facts and history)?
  2. choose a different storage impl?

Storage for sure, thats the purpose, initial state, if we decide it is needed (will need to have the storage initialized as well).


crates/starknet_committer_cli/src/main.rs line 48 at r1 (raw file):

Previously, dorimedini-starkware wrote…

extract to a handle_log_level(level: String, log_filter_handle: Handle<LevelFilter, Registry) function please

In tracing_utils?

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorimedini-starkware reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)


crates/starknet_committer_cli/src/main.rs line 48 at r1 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

In tracing_utils?

sure


crates/starknet_committer_cli/src/main.rs line 40 at r2 (raw file):

    info!("Starting committer-cli with command: \n{:?}", committer_command);
    match committer_command.command {
        Command::StorageBenchmark(args) => {

we usually destructure these - non blocking

Suggestion:

 Command::StorageBenchmark(StorageArgs { seed, n_iterations, log_level, output_dir }) => {

@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from fd1fbdd to f09e492 Compare September 18, 2025 07:51
Copy link
Contributor Author

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TzahiTaub reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @TzahiTaub)

Copy link
Contributor Author

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TzahiTaub reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dorimedini-starkware reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

@TzahiTaub TzahiTaub merged commit 386ce2a into committer_storage_benchmark Sep 18, 2025
34 of 36 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants