fix(ci): bench gate should not attribute regressions to unrelated PRs - #371
Merged
Conversation
…ot reach the benchmarked crates The gate reported a +27.8..+34.4% regression (p = 0.00) in `ridge_decomposition_solve` — `crates/larql-compute` — on two branches that touched only `crates/larql-vindex`. Two independent samples, so not a one-off. The changed code could not be reached from the benchmark, so the attribution was false and the number was runner variance. The PR trigger now carries a `paths:` filter covering exactly the surface `BENCHES` in scripts/bench-regress.sh gates — `crates/larql-compute`, `crates/larql-compute-metal`, the script, the workflow and Cargo.lock. A PR that cannot reach those crates no longer produces a red. `workflow_dispatch` stays, so a reviewer can still ask for the check on a PR the filter skipped. This removes the false attribution. It does NOT fix the deeper problem, which is now written into the workflow header rather than left for the next person to rediscover: the baseline is recorded on a `main` push, cached, and restored onto a DIFFERENT macos-14 runner, so Criterion compares absolute timings across two machines at different times. This repo does not accept that anywhere else — q2a_decode_bench runs both arms in ONE process, interleaved with alternating order, min-of-N, "never compared against a number from another session or day (machine state shifts move e2e timings by ±6%)". A red here is a signal to go and measure properly, never proof that a change caused it. Same-runner branch-vs-main benchmarking is tracked in #370.
chrishayuk
added a commit
that referenced
this pull request
Sep 1, 2026
A capture is process-wide, and its test assumed an exclusivity it never had: a sibling test projecting on another thread lands inside the capture window. Six calls recorded for five issued turned larql-vindex red on Windows at 8ac0b7a, after fourteen green runs on an identical image with an identical test count — neither #364 nor #371 changed anything that compiles on Windows. Calls are now selected by operand address (`WeightRows::primary_addr`), and the two tests that OPEN a capture share one lock rather than one each. `record` also probed the lock with `try_lock` and returned on contention, reasoning that a racing worker "would only ever be inside a projection this call already recorded". Nothing enforces that: `parallel6` exists to run six independent branches on separate pool workers, and those issue distinct projections. That path is LATENT, not live — parallel6 has no production call site and the MoE fan-out reaches `project_rows` directly rather than through `project` — so no shipped measurement is known to have lost a call. A recorder whose correctness rests on no caller ever fanning out is still the wrong shape. An AtomicBool now gates the idle path, cheaper than the lock probe it replaces, and the operand is built before the lock so the held section is one push. Reproduces locally: both capture tests fail together in parallel and pass under --test-threads=1.
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
1 participant
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.
Reported a +27.8–34.4 % regression (p = 0.00) in
ridge_decomposition_solve(larql-compute) on two branches that touched onlylarql-vindex. Two independent samples.This PR: a
paths:filter restricting the PR trigger to the surfaceBENCHESactually gates —larql-compute,larql-compute-metal, the script, the workflow,Cargo.lock. A PR that cannot reach those crates no longer produces a red.workflow_dispatchretained so the check can still be requested.Not fixed here, now documented in the workflow header and tracked in #370: the baseline is recorded on a
mainpush and restored onto a different runner, so Criterion compares timings across two machines. The repo's ownq2a_decode_benchruns both arms in one process, interleaved, min-of-N, precisely because a cross-session number is not a measurement.Companion to #367 (coverage debt) and #369 (flaky 1 ns timeout test) — three independent ways CI currently fails to attribute a red to a change.