ci: add performance regression check for PRs / merges - #24562
Open
Dandandan wants to merge 10 commits into
Open
Conversation
Adds a `Benchmarks` workflow that runs TPC-H SF1 twice on one machine -- once for the base branch, once for the PR merged into it -- and fails when the PR is slower than the configured limits allow. The data is generated with tpchgen-cli, both sides are built with `--release` into separate target directories, and each side is measured with `benchmark_runner tpch --output`, so the JSON results feed straight into `compare.py`. `compare.py` grows the gate it needs for that: `--fail-threshold` and `--fail-total-threshold` make it exit non-zero on a per-query or total-time regression (both off by default, so existing usage is unchanged), a query that fails on only one side is reported instead of being silently dropped, and named query ids such as `tpch/Q01/sf1` are no longer rendered as `Qtpch/Q01/sf1`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
development-process
`cargo install` in a workflow is rejected by ci/scripts/check_no_cargo_install_in_workflows.sh. Pin 3.0.0 so the generated data stays fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two builds dominate this job. `release-nonlto` is `release` with `lto = false` and 16 codegen units, and fat LTO with a single codegen unit roughly doubles the build; both sides are built identically, so the ratio the gate looks at is unaffected. `workflow_dispatch` can still pick `release` for numbers comparable with locally posted bench.sh results. The builds also run concurrently now, since neither keeps every core busy on its own. They keep a target directory each, because cargo locks one exclusively and sharing it would serialize them; their logs are captured and replayed in groups so the two do not interleave line by line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24562 +/- ##
==========================================
+ Coverage 81.23% 81.36% +0.12%
==========================================
Files 1112 1117 +5
Lines 390635 397872 +7237
Branches 390635 397872 +7237
==========================================
+ Hits 317350 323717 +6367
- Misses 54650 55235 +585
- Partials 18635 18920 +285 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Backgrounding both builds in one job shared 16 cores between them. Split the workflow into three jobs instead: one resolves the base commit, two build a `benchmark_runner` each on their own 32-core runner, and the last one measures both binaries back to back on one machine -- which is the part that has to stay on a single machine for the timings to be comparable. Passing a binary between jobs needs care: `benchmark_runner` finds `sql_benchmarks` through the CARGO_MANIFEST_DIR baked in at compile time, so each side's tree is checked out at the same fixed path in the job that builds it and in the job that runs it, and the benchmark job verifies that each binary still sees the tpch suite before measuring anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SF1 runs the whole 22-query suite in ~6 seconds, so per-query timings sit in the range where runner noise is a large part of the measurement. SF10 gives the gate something to measure; the scale factor is a dispatch input, so SF1 is still one click away. tpchgen-cli now comes from its PyPI wheel: same 3.0.0 release as the crate, but a ~4MB download rather than a build from source -- the project attaches no binaries to its GitHub releases, so nothing could fetch a prebuilt one. That also drops the Rust toolchain from the benchmark job, which no longer compiles anything at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dandandan
changed the title
Both CI runs of this workflow so far failed on a PR that changes no Rust at all: Q02 came out 1.22x slower at SF1 and 1.28x at SF10, tight on both sides each time. The two binaries turn out to have byte-identical `.text` layout, so this was never a code difference -- it is the measurement design. Running all of one side and then all of the other lets anything that drifts between the two blocks, a runner that slows down halfway through or a process that drew an unlucky heap, show up as a clean per-query offset on one side only, and a point ratio against a fixed threshold cannot tell that from a regression. So the benchmark job now measures the sides interleaved: a discarded warmup pass, then six rounds of one pass each, alternating which side leads so each pays the first-position cost the same number of times. Every round is a fresh process, which turns per-process luck into round-to-round spread instead of a fixed offset, and six rounds of one iteration cost about what five iterations of one block did. `compare.py` grows the statistics that design makes possible. Either path may now be a directory of per-round summaries, paired with the other side's in sorted filename order, and a query has to clear three bars to fail: - the median of its per-round ratios is above the limit, so one slow pass on either side cannot decide the verdict - the regression costs at least `--fail-min-delta-ms`, which is what stops an SF1 run failing on queries where 1.20x is four milliseconds - it exceeds the spread the base side showed against itself across the rounds, because a query whose own baseline moved 26% between rounds cannot support a 25% verdict Anything that clears the first bar but not the others is printed under "Not counted against the gate" rather than dropped, the table gains per-round and noise columns, and the summary reports the noise floor, the geometric mean of the per-query ratios, and the CPU count -- the last because without the RunsOn variable set this job silently lands on a shared 4-vCPU runner, which is where these numbers came from. Against a noise model fitted to those two runs, the odds that a 22-query run reports at least one regression on unchanged code drop from 78% to 1%, while detection of a real 1.30x regression goes up from 86% to 98%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check is opt-in on pull requests, which means most changes land without ever being measured. So it now also runs on every push to `main`, where the base is the commit the merge landed on: a regression that no pull request measured is still attributed to the one merge that introduced it, with no bisect to run afterwards. Doc-only merges are skipped through the same `paths-ignore` list `rust.yml` uses. `HEAD^1` is the right base for a push whether the merge was squashed into one commit or kept as a merge commit, but the existing fallback would have fetched the default branch and compared its tip against itself, so the push case is resolved explicitly. The `performance` label still gates pull requests; every other event runs unconditionally. The candidate side is a merged commit on `main` as often as it is a PR now, so it is renamed from `pr` to `head` -- base and head being what GitHub calls these two anyway. It also happens to make the two worktree paths the same length, so the file paths baked into the two binaries are the same length too, which is where most of the bytes that differ between them come from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two runs whose false Q02 regressions prompted the interleaved design were measured on four vCPUs, not on the 16-vCPU runner the `runs-on` lines ask for. The reason is not the workflow: GitHub withholds `vars` from workflows triggered by a pull request from a fork, so `vars.USE_RUNS_ON` reads as empty there however the repository has it set, and every job takes the `ubuntu-latest` fallback. The same thing happens to all of `rust.yml` -- `linux build test` runs on `runs-on=...,cpu=8` for a push to `main` and on `ubuntu-latest` for a fork pull request. Nothing can be done about that from inside the workflow: reaching the larger runner from a fork pull request would mean `pull_request_target` or `workflow_run`, which is building and running unreviewed code with the base repository's token. What can be done is to stop it being a surprise, so the machine step now explains the fallback rather than only warning about the core count, and points at the `main` run after the merge as the authoritative one -- that one does get the 16-vCPU runner, which is the other half of why measuring every merge is worth the runner time. Also notes the consequence of the faster machine: an SF10 query that takes 300ms on four vCPUs takes under 100ms on sixteen, and a 20% regression on that is inside the 25ms floor, so the shortest few queries stop being gated unless the run is dispatched with a smaller floor or a larger scale factor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l clock
tpchgen-cli 3.0.0 warns that `--format` goes away in 4.0.0 and that
`--parquet-compression` is deprecated, both in favour of a subcommand per
format. `tpchgen-cli parquet --compression=...` produces a byte-identical tree
at SF0.01, so this is a rename; `bench.sh` gets the same treatment for its
parquet, csv and sort-pushdown calls, and says which version it needs.
The step timings of the SF10 run say where the half hour goes, and it is not
the part that looks expensive:
build base runner 990s (835s cargo, 131s freeing disk space)
build head runner 966s (845s cargo, 96s freeing disk space)
benchmark 461s (410s measuring, 34s generating the data)
So generating the data is 34 seconds and needs nothing done to it, while the
builds are two minutes of deleting Android SDKs followed by fourteen minutes of
compiling dependencies that neither side changed. The builds now restore a
`Swatinem/rust-cache` entry, shared between the two sides because they are a
commit or two apart, written only by pushes to `main` so pull request runs do
not each save a near-identical copy; and the disk cleanup is skipped when the
runner already asked for `disk=large`, which is every run that gets the RunsOn
machine.
The warmup pass is now a read of the data files rather than a discarded pass of
the suite. Each round is a fresh process, so the page cache is the only thing a
warmup can carry between rounds, and reading the files fills it in seconds
where a pass of the suite cost as much as a measured round.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dandandan
changed the title
Most of the prose was two or three times longer than the point it made. Cut to a sentence or two each, keeping the facts that are not visible from the code: why the sides are interleaved, the three bars a query clears to fail the gate, and that fork pull requests cannot reach the larger runner. 107 lines lighter, with no behaviour change -- `compare.py` gives the same verdict on the last run's rounds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dandandan
changed the title
Dandandan
marked this pull request as ready for review
August 21, 2026 18:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
We want to catch regressions / track possible performance improvements.
What changes are included in this PR?
New
Benchmarksworkflow (.github/workflows/benchmark.yml), three jobs:Currently it is opt-in during development: when adding the
performancelabel it runs the TPC-H SF=10 benchmark on each commit.It also runs on a merge to main (so I believe with the merge queue it should run before it merges to main).
It also adds a way to run benchmarks with much less noise (usually it sits at ~1% here):
Are these changes tested?
Are there any user-facing changes?
No API changes