Upgrade bencher cli by teofr · Pull Request #1622 · NomicFoundation/slang

teofr · 2026-04-07T11:39:13Z

Upgraded bencher cli
And made comparison of callgrind based benchmarks only against previous commit
Added a BencherThreshold struct to better represent thresholds

Test

Run smoke tests
Add ci:perf label and check comment

changeset-bot · 2026-04-07T11:39:20Z

⚠️ No Changeset found

Latest commit: eb39719

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2026-04-07T12:03:15Z

Bencher Report

Branch	teofr/upgrade-bencher
Testbed	ci

⚠️ WARNING: Truncated view!
The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 3 Alerts

🐰 View full continuous benchmarking report in Bencher

github-actions · 2026-04-07T12:44:41Z

Bencher Report

Branch	teofr/upgrade-bencher
Testbed	ci

⚠️ WARNING: Truncated view!
The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🐰 View full continuous benchmarking report in Bencher

github-actions · 2026-04-07T15:32:19Z

Bencher Report

Branch	teofr/upgrade-bencher
Testbed	ci

⚠️ WARNING: Truncated view!
The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 2 Alerts

🐰 View full continuous benchmarking report in Bencher

Copilot

Pull request overview

Upgrades the Bencher CLI used by the infra CLI and re-enables PR commenting for cargo benchmarks, while adding a new option to limit threshold sampling for PR benchmark comparisons.

Changes:

Bump bencher_cli dependency to the v0.6.1 commit.
Update run_bench to always post GitHub Actions PR comments for --pr-benchmark, and add threshold_max_sample_size.
Grant required GitHub Actions permissions and pass GITHUB_TOKEN in cargo benchmark workflows.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`crates/infra/cli/src/toolchains/bencher/mod.rs`	Adjusts bencher invocation for PR benchmarks (always uses `--github-actions`) and adds `--threshold-max-sample-size` plumbing.
`crates/infra/cli/src/commands/perf/npm/mod.rs`	Updates `run_bench` call site for the new function signature.
`crates/infra/cli/src/commands/perf/cargo/mod.rs`	Updates cargo perf benchmark invocation to set `threshold_max_sample_size`.
`Cargo.toml`	Pins `bencher_cli` to a newer upstream revision (v0.6.1).
`.github/workflows/benchmark_cargo_slang.yml`	Adds PR write permission and passes `GITHUB_TOKEN` for PR commenting.
`.github/workflows/benchmark_cargo_cmp.yml`	Adds PR write permission and passes `GITHUB_TOKEN` for PR commenting.

crates/infra/cli/src/toolchains/bencher/mod.rs

crates/infra/cli/src/commands/perf/cargo/mod.rs

nebasuke

One question, one possible suggestion, neither blocking. Thanks Teo!

nebasuke · 2026-04-08T18:40:25Z

crates/infra/cli/src/toolchains/bencher/mod.rs

+                        .expect("Threshold upper boundary must be set for --pr-benchmark"),
+                )
+                // Since we have to show it for all or for none of the thresholds, we just always
+                // show it and default to "_"


Is this a Bencher feature? How does Bencher handle _?

It ignores the given option for that feature. Bencher requires that, when specifying multiple thresholds, they all use the same options (I guess for easy disambiguation)

It is possible to create multiple Thresholds with the same bencher run invocation. When specifying multiple Thresholds, all of the same options must be used for each Threshold. To ignore an option for a specific Threshold, use an underscore (_).

From: https://bencher.dev/docs/explanation/thresholds/#multiple-thresholds

I added a comment

nebasuke · 2026-04-09T08:12:28Z

crates/infra/cli/src/toolchains/bencher/mod.rs

+                    "--threshold-upper-boundary",
+                    upper_boundary
+                        .as_ref()
+                        .expect("Threshold upper boundary must be set for --pr-benchmark"),


I found this a bit surprising, but I understand you're doing this for the builder pattern.

One option after having some discussions with Claude (take this with a grain of salt given my current Rust knowledge) is to have required params in the constructor, with only max_sample_size optional, and using a closure to do the measure name setting you want.

pub(crate) struct BencherThreshold { pub measure: String, pub upper_boundary: String, pub max_sample_size: Option<String>, } impl BencherThreshold { pub fn new(measure: &str, upper_boundary: &str) -> Self { Self { measure: measure.into(), upper_boundary: upper_boundary.into(), max_sample_size: None } } pub fn with_max_sample_size(mut self, v: &str) -> Self { self.max_sample_size = Some(v.into()); self } } Call site uses a closure to keep the "base config" pattern: let threshold = |measure| BencherThreshold::new(measure, "0.01").with_max_sample_size("2"); &[threshold("estimated-cycles"), threshold("instructions"), ...] And the npm site stays a one-liner: BencherThreshold::new("duration", "0.10").

That's a fair point, I think we may need a more flexible approach when we start fine tuning the thresholds, but for now this is more than enough. Thanks!

github-actions · 2026-04-13T12:01:24Z

Bencher Report

Branch	teofr/upgrade-bencher
Testbed	ci

⚠️ WARNING: Truncated view!
The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🐰 View full continuous benchmarking report in Bencher

teofr added the ci:perf Runs performance test dry-runs in a PR (rather than the smoke-tests) label Apr 7, 2026

teofr force-pushed the teofr/upgrade-bencher branch from 1d28cb6 to 607886d Compare April 7, 2026 13:14

teofr force-pushed the teofr/upgrade-bencher branch from 607886d to a931fdf Compare April 7, 2026 15:57

teofr marked this pull request as ready for review April 7, 2026 15:57

teofr requested review from a team as code owners April 7, 2026 15:57

teofr requested a review from Copilot April 7, 2026 15:58

Copilot started reviewing on behalf of teofr April 7, 2026 15:59 View session

Copilot AI reviewed Apr 7, 2026

View reviewed changes

crates/infra/cli/src/toolchains/bencher/mod.rs Outdated Show resolved Hide resolved

crates/infra/cli/src/commands/perf/cargo/mod.rs Outdated Show resolved Hide resolved

teofr marked this pull request as draft April 7, 2026 16:10

teofr force-pushed the teofr/upgrade-bencher branch 3 times, most recently from de39bef to 835f1ee Compare April 7, 2026 17:07

teofr marked this pull request as ready for review April 7, 2026 17:32

teofr force-pushed the teofr/upgrade-bencher branch from 835f1ee to f629797 Compare April 8, 2026 14:11

nebasuke approved these changes Apr 9, 2026

View reviewed changes

teofr force-pushed the teofr/upgrade-bencher branch from f629797 to 9bb37c8 Compare April 13, 2026 11:35

teofr added 3 commits April 13, 2026 08:37

Upgrade bencher cli

a700227

Improve thresholds

1b7b820

Use single comparison point on callgrind benchmarks

d9dce51

teofr force-pushed the teofr/upgrade-bencher branch from 9bb37c8 to 772b0a6 Compare April 13, 2026 11:40

Added Slang v2 after rebase

eb39719

teofr force-pushed the teofr/upgrade-bencher branch from 772b0a6 to eb39719 Compare April 13, 2026 11:48

teofr added this pull request to the merge queue Apr 13, 2026

Merged via the queue into main with commit d91d01a Apr 13, 2026
15 checks passed

teofr deleted the teofr/upgrade-bencher branch April 13, 2026 13:58

Conversation

teofr commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test

Uh oh!

changeset-bot bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ WARNING: Truncated view!

🚨 3 Alerts

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ WARNING: Truncated view!

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ WARNING: Truncated view!

🚨 2 Alerts

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

nebasuke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 13, 2026

⚠️ WARNING: Truncated view!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

teofr commented Apr 7, 2026 •

edited

Loading

changeset-bot bot commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading