Skip to content

Conversation

@AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

AvivYossef-starkware commented Oct 19, 2025

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

@github-actions
Copy link

github-actions bot commented Oct 19, 2025

Benchmark movements: No major performance changes detected.

Copy link
Collaborator

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

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

@meship-starkware reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware)


bench_tools/src/benches/dummy_bench.rs line 14 at r1 (raw file):

    c.bench_function("dummy_sum_1000", |b| b.iter(|| dummy_function(black_box(1000))));
}

Is this the function that creates the benchmark files? Where are we going to use it?
Can you add a small documentation on the purpose of this function?

Code quote:

#[allow(dead_code)]
fn dummy_benchmark(c: &mut Criterion) {
    c.bench_function("dummy_sum_100", |b| b.iter(|| dummy_function(black_box(100))));

    c.bench_function("dummy_sum_1000", |b| b.iter(|| dummy_function(black_box(1000))));
}

@avi-starkware
Copy link
Collaborator

bench_tools/src/benches/dummy_bench.rs line 11 at r1 (raw file):

#[allow(dead_code)]
fn dummy_benchmark(c: &mut Criterion) {
    c.bench_function("dummy_sum_100", |b| b.iter(|| dummy_function(black_box(100))));

Why is black_box needed here?
And shouldn't it be black_box(dummy_function(100))?

Additionally, criterion::black_box is deprecated (see docs) you should use std::docs::black_box instead

Code quote:

    c.bench_function("dummy_sum_100", |b| b.iter(|| dummy_function(black_box(100))));

@avi-starkware
Copy link
Collaborator

bench_tools/src/benches/dummy_bench.rs line 11 at r1 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

Why is black_box needed here?
And shouldn't it be black_box(dummy_function(100))?

Additionally, criterion::black_box is deprecated (see docs) you should use std::docs::black_box instead

correction: it is std::hint::black_box

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

@avi-starkware reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware and @meship-starkware)


bench_tools/src/types/estimates_test.rs line 30 at r1 (raw file):

    // 2) Collect ALL estimates.json files under target/criterion/**/new/
    let patterns =

Why do you collect all estimates.json files and not just the ones related to dummy_benches?

Code quote:

    // 2) Collect ALL estimates.json files under target/criterion/**/new/
    let patterns =

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/deserialize_cargo_bench_result branch 2 times, most recently from bd42739 to 576ba64 Compare October 22, 2025 06:32
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware 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: 0 of 12 files reviewed, 3 unresolved discussions (waiting on @avi-starkware and @meship-starkware)


bench_tools/src/types/estimates_test.rs line 30 at r1 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

Why do you collect all estimates.json files and not just the ones related to dummy_benches?

Thanks


bench_tools/src/benches/dummy_bench.rs line 11 at r1 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

correction: it is std::hint::black_box

According to AI, the compiler might delete the dummy_function calculation because we don't use the result.


bench_tools/src/benches/dummy_bench.rs line 14 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Is this the function that creates the benchmark files? Where are we going to use it?
Can you add a small documentation on the purpose of this function?

It's a test util, we use it in estimates_test

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

@avi-starkware reviewed 9 of 12 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 10 of 12 files reviewed, 3 unresolved discussions (waiting on @meship-starkware)


crates/bench_tools/src/types/estimates_test.rs line 27 at r3 (raw file):

fn workspace_root(bench_tools_crate_dir: PathBuf) -> PathBuf {
    bench_tools_crate_dir.parent().unwrap().parent().unwrap().to_path_buf()
}

Consider using workspace_root() from the crate infra_utils.

Code quote:

fn workspace_root(bench_tools_crate_dir: PathBuf) -> PathBuf {
    bench_tools_crate_dir.parent().unwrap().parent().unwrap().to_path_buf()
}

crates/bench_tools/src/types/estimates_test.rs line 87 at r3 (raw file):

fn deserialize_dummy_bench_estimates(dummy_bench_results_dir: PathBuf, dummy_bench_names: &[&str]) {
    assert_deserialize_dummy_bench_estimates(&dummy_bench_results_dir, dummy_bench_names);
}

Why is this test needed?

Code quote:

#[rstest]
/// Test that Estimates can be deserialized from the saved results.
fn deserialize_dummy_bench_estimates(dummy_bench_results_dir: PathBuf, dummy_bench_names: &[&str]) {
    assert_deserialize_dummy_bench_estimates(&dummy_bench_results_dir, dummy_bench_names);
}

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/deserialize_cargo_bench_result branch from 576ba64 to 405ef34 Compare October 22, 2025 12:13
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware 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: 7 of 12 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @meship-starkware)


crates/bench_tools/src/types/estimates_test.rs line 27 at r3 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

Consider using workspace_root() from the crate infra_utils.

Done


crates/bench_tools/src/types/estimates_test.rs line 87 at r3 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

Why is this test needed?

to verify that the struct deserializes the json successfully

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

@avi-starkware reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @meship-starkware)


crates/bench_tools/src/types/estimates_test.rs line 87 at r3 (raw file):

Previously, AvivYossef-starkware wrote…

to verify that the struct deserializes the json successfully

But isn't this verified in the test above as well?

@AvivYossef-starkware AvivYossef-starkware changed the base branch from aviv/bench_tools_scaffold to main-v0.14.1 October 23, 2025 08:25
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/deserialize_cargo_bench_result branch from b2a3463 to 82ff5e6 Compare October 23, 2025 08:25
@graphite-app
Copy link

graphite-app bot commented Oct 23, 2025

Merge activity

  • Oct 23, 8:25 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-starkware 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: 9 of 12 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @meship-starkware)


crates/bench_tools/src/types/estimates_test.rs line 87 at r3 (raw file):

Previously, avi-starkware (Avi Cohen) wrote…

But isn't this verified in the test above as well?

But the test above is ignored because I dont want to actual run the benechmark in the CI

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

@avi-starkware reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 10 of 12 files reviewed, all discussions resolved (waiting on @meship-starkware)

Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

@avi-starkware reviewed 2 of 12 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

@AvivYossef-starkware AvivYossef-starkware added this pull request to the merge queue Oct 23, 2025
Copy link
Collaborator

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)

Merged via the queue into main-v0.14.1 with commit 2a4cd4d Oct 23, 2025
30 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 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.

5 participants