Skip to content

Measure bench baselines on the same runner - #144

Merged
owjs3901 merged 2 commits into
mainfrom
owjs3901/fix-bench-gate
Aug 30, 2026
Merged

Measure bench baselines on the same runner#144
owjs3901 merged 2 commits into
mainfrom
owjs3901/fix-bench-gate

Conversation

@owjs3901

@owjs3901 owjs3901 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Problem

The Criterion regression gate compared measurements taken on two different machines.

The baseline was produced only on push to main, persisted through the actions cache, then restored in a later PR job and compared against numbers measured on whatever runner that PR happened to land on. GitHub's ubuntu-latest fleet is heterogeneous, so fleet variance registered as a code regression.

Run 33302880992 failed 15 benches at +25% to +148% on #141 — a changepacks release PR whose entire diff is version = "0.3.1""0.4.0" strings plus three deleted changepack logs. Zero bytes of crates/** changed. The same benchmark code had passed on main eight minutes earlier (run 33302590831).

The documented noise filter (mean > 10% AND CI lower > 5%) cannot catch this. Criterion's confidence interval measures within-run variance, not between-machine variance — when the two sides run on different hardware criterion becomes more confident, not less. Hence the observed signature of a huge mean shift inside a very tight band (+148.04% mean, +145.58% CI lower).

Secondarily, Cargo.toml is a path trigger, so every changepacks "Update Versions" PR drags the perf gate along despite touching no source.

Changes

  • Same-runner comparison. Measure the PR merge base with --save-baseline main, then the PR head with --baseline main, back to back in one job. The cross-run baseline cache is removed entirely.
  • No stale estimates. rm -rf target/criterion before the baseline run, so no change/estimates.json survives from an unrelated run.
  • Version-only PRs skip the gate. Both revisions' Cargo.toml are parsed with tomllib and compared after removing only changepacks-managed release versions (workspace.package.version, plus version on path dependencies pointing at crates/<name>). Anything else — external dependency bumps, profile edits, new dependencies, an unparseable file — fails safe and runs the gate.
  • Thresholds calibrated to the measured noise floor: +10%/+5%+30%/+20%.

Why the thresholds moved

Same-runner comparison cut the noise by an order of magnitude, but did not remove it. Baseline and head are measured minutes apart, so runner drift and co-tenancy still shift the mean.

This PR is its own control: it touches only bench.yml, so its merge base and head hold byte-identical Rust. Every reported delta below is pure noise.

Run Range Max Benches tripped at +10%/+5%
1 −12.33% … +12.20% +12.20% borrowed_pre_P2/1
2 (re-run) −4.68% … +14.92% +14.92% borrowed_pre_P2/1024, owned_post_P2/1024
3 −2.07% … +12.33% +12.33% borrowed_pre_P2/64

All three identical-code runs would have failed the old gate, each on a different benchmark. A gate with a ~100% false-positive rate on unchanged code is not a gate. +30%/+20% sits at roughly 2x the highest observed noise, so the timing gate now catches only big regressions — which is what the header comment always claimed it was for. Precise, deterministic regressions remain covered noise-free by the alloc_budget integration test in the normal cargo test job.

Verification

Local, against the real revisions (merge base 31f7e652, #141 head):

Case should_run Expected
Real #141 (release version bump) false false
External dependency version change (garde) true true
Profile change (profile.bench.codegen-units) true true
New dependency added true true
  • The Determine PR benchmark scope step was extracted from this YAML and executed under bash -eo pipefail (matching GitHub's shell: bash) against the real revisions — it emits merge_base=31f7e652… and should_run=false, confirming the heredoc-inside-block-scalar indentation is correct.
  • Workflow parses cleanly; 8 steps with the expected conditionals.
  • cargo bench -p vespera_inprocess --bench dispatch -- --list still resolves all 15 filtered benches.
  • jni-bench.yml and CI.yml are untouched.
  • CI on this PR is green, including the Criterion gate running its own new logic.

Cost

PR runs measure benches twice, so bench wall-clock roughly doubles (3m50s → ~6m40s). Cargo build outputs are reused across the two measurements within the job, so compile time is not doubled. Release PRs like #141 now skip the gate entirely and get faster.

Note

This must land before #141 can go green — #141 re-run under the current main workflow keeps comparing against the cached cross-machine baseline.

@github-actions

Copy link
Copy Markdown

Changepacks

vespera@0.3.1 → 0.4.0 - Cargo.toml

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

vespera@0.3.1 → 0.4.0 - crates/vespera/Cargo.toml

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

vespera_core@0.3.1 → 0.4.0 - crates/vespera_core/Cargo.toml

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

vespera_inprocess@0.3.1 → 0.4.0 - crates/vespera_inprocess/Cargo.toml

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

vespera_jni@0.3.1 → 0.4.0 - crates/vespera_jni/Cargo.toml

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

vespera_macro@0.3.1 → 0.4.0 - crates/vespera_macro/Cargo.toml

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.
  • Fail the build when merged apps define conflicting same-named OpenAPI schemas, turning a previously silent first-wins condition into an actionable compile error while preserving identical-schema deduplication.

Patch

  • Cover every export prefix branch and require 100% Rust line coverage in CI.

vespera-bridge@0.3.1 → 0.4.0 - libs/vespera-bridge/build.gradle.kts

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

vespera-bridge-gradle-plugin@0.3.1 → 0.4.0 - libs/vespera-bridge-gradle-plugin/build.gradle.kts

Minor

  • Add an explicit export_app! prefix option that namespaces routes, OpenAPI paths, and generated component schemas.

@owjs3901
owjs3901 merged commit db70d36 into main Aug 30, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant