Benchmark regressions are not gated — baselines are compared by hand #54

New Issue

2026-09-08T19:14:31Z

logaritmisk commented

2026-09-08 19:14:31 +00:00

Split from #26, which is otherwise delivered. This is infra rather than test coverage, and it needs a threshold decision before it can be built.

Current state

benches/ holds six benchmarks and two committed baselines, baseline.txt and scored_baseline.txt. The justfile recipes are --save-baseline base and --baseline base, compared by reading the output. Nothing fails.

So a performance regression is caught only if somebody happens to run the benchmarks and happens to read the numbers. Several changes this crate has made were justified by measurements — the joint factorisation split, the libm routing, the whitening change — and none of them would have been protected by anything if they had later regressed.

Why it is not just "add critcmp"

The threshold is the whole problem, and it is a judgement call:

Criterion on a developer laptop is noisy. Thermal state and background load move results by several percent, which is the same order as a regression worth catching.
Too tight and CI goes red on noise, and a gate people ignore is worse than no gate — it trains the reflex to re-run until green.
Too loose and it never fires.

The measurements this crate cares about vary in how noisy they are. joint_factorise_480_appearances at ~9 ms is fairly stable; gaussian microbenchmarks are not.

Options

critcmp in CI with a fixed threshold, e.g. fail at >10% slower. Simple, and needs a stable runner to mean anything.
Per-benchmark thresholds, tight where the measurement is stable and loose where it is not. More honest, more to maintain.
Track rather than gate: publish the comparison as a CI artifact or a comment, and let a human judge. Never produces a false failure, and relies on someone reading it.
Leave it. The benchmarks exist and can be run deliberately when a change is expected to matter, which is roughly what happens today.

What is needed to decide

Whether the CI runner is stable enough for (1) to mean anything — the gitea Actions setup is what decides this, and I have not measured its run-to-run variance. That measurement is the actual first step: run one unchanged benchmark ten times on the runner and look at the spread. If it is 15%, options (1) and (2) are both dead and the answer is (3) or (4).

Recorded rather than actioned, since #26 is otherwise complete and this has been the open item on it since 2026-08-04.

Split from #26, which is otherwise delivered. This is infra rather than test coverage, and it needs a threshold decision before it can be built. ## Current state `benches/` holds six benchmarks and two committed baselines, `baseline.txt` and `scored_baseline.txt`. The `justfile` recipes are `--save-baseline base` and `--baseline base`, compared by reading the output. Nothing fails. So a performance regression is caught only if somebody happens to run the benchmarks and happens to read the numbers. Several changes this crate has made were justified by measurements — the joint factorisation split, the libm routing, the whitening change — and none of them would have been protected by anything if they had later regressed. ## Why it is not just "add critcmp" The threshold is the whole problem, and it is a judgement call: - Criterion on a developer laptop is noisy. Thermal state and background load move results by several percent, which is the same order as a regression worth catching. - Too tight and CI goes red on noise, and a gate people ignore is worse than no gate — it trains the reflex to re-run until green. - Too loose and it never fires. The measurements this crate cares about vary in how noisy they are. `joint_factorise_480_appearances` at ~9 ms is fairly stable; `gaussian` microbenchmarks are not. ## Options 1. **`critcmp` in CI with a fixed threshold**, e.g. fail at >10% slower. Simple, and needs a stable runner to mean anything. 2. **Per-benchmark thresholds**, tight where the measurement is stable and loose where it is not. More honest, more to maintain. 3. **Track rather than gate**: publish the comparison as a CI artifact or a comment, and let a human judge. Never produces a false failure, and relies on someone reading it. 4. **Leave it.** The benchmarks exist and can be run deliberately when a change is expected to matter, which is roughly what happens today. ## What is needed to decide Whether the CI runner is stable enough for (1) to mean anything — the gitea Actions setup is what decides this, and I have not measured its run-to-run variance. That measurement is the actual first step: run one unchanged benchmark ten times on the runner and look at the spread. If it is 15%, options (1) and (2) are both dead and the answer is (3) or (4). Recorded rather than actioned, since #26 is otherwise complete and this has been the open item on it since 2026-08-04.

logaritmisk added the decision infra testing labels 2026-09-08 19:14:41 +00:00

logaritmisk referenced this issue

2026-09-08 19:15:01 +00:00

Input validation is debug-only, and the `History` ingestion path bypasses `InferenceError` entirely #18

logaritmisk referenced this issue

2026-09-08 19:15:19 +00:00

Test suite has no coverage for degenerate inputs, release-mode behaviour, or non-finite results #26

logaritmisk referenced this issue from a commit

2026-09-10 04:55:31 +00:00

ci: measure the runner's own benchmark variance, and fix the joint bench

logaritmisk referenced this issue from a commit

2026-09-10 04:55:31 +00:00

Merge infra/bench-variance (#54)

logaritmisk commented

2026-09-10 04:55:42 +00:00

The measurement step is in, as Benchmark variance — 0801ace (merged as d36d125). workflow_dispatch only, with a runs input defaulting to 10.

It runs joint_factorise_480_appearances repeatedly and reports min / median / max / mean and (max-min)/min. That benchmark is the right probe for the reason the issue gives: ~9 ms, so not timer-dominated, and it is the measurement this crate most wants protected — the dense factorisation #52 is about replacing.

Run it, and the number decides the issue. Near 15% and options 1 and 2 are dead, leaving (3) track-don't-gate. Near 2% and a gate at 10% is meaningful. The report prints that framing next to the number so whoever runs it does not have to come back here.

Something the issue could not have known: the joint benchmark did not run

thread 'main' panicked at benches/joint.rs:47:26:
called `Result::unwrap()` on an `Err` value:
  NotConverged { iterations: 30, final_step: (0.00045227832, 0.0), epsilon: 1e-10 }

Its fixture asked for epsilon: 1e-10 inside max_iter: 30 and never got there. That was invisible while converge returned short fits silently, and became a panic when it stopped. It now uses the default ITERATIONS cap — measuring a factorisation on an unconverged fit is measuring something nobody would run. I checked the other four benchmarks; they are fine.

This is itself evidence for the issue's premise. A benchmark nobody runs is a benchmark that has been broken for an unknown length of time.

Two bugs in the report step, both caught by running it

asort is a gawk extension and ubuntu-latest's awk is mawk. It failed immediately on a local sample. Sorting goes through sort -n now.
Criterion picks its unit per run, so a mixed batch would compare 9 ms against 9 µs as if they were the same number — exactly the plausible-looking wrong answer this crate keeps removing. The job now refuses to report a spread unless every run agrees on the unit. Both the firing and the passing case were tested against real criterion output.

Leaving this open until the job has been run and the threshold decided.

The measurement step is in, as `Benchmark variance` — 0801ace (merged as d36d125). `workflow_dispatch` only, with a `runs` input defaulting to 10. It runs `joint_factorise_480_appearances` repeatedly and reports min / median / max / mean and `(max-min)/min`. That benchmark is the right probe for the reason the issue gives: ~9 ms, so not timer-dominated, and it is the measurement this crate most wants protected — the dense factorisation #52 is about replacing. **Run it, and the number decides the issue.** Near 15% and options 1 and 2 are dead, leaving (3) track-don't-gate. Near 2% and a gate at 10% is meaningful. The report prints that framing next to the number so whoever runs it does not have to come back here. ## Something the issue could not have known: the joint benchmark did not run ``` thread 'main' panicked at benches/joint.rs:47:26: called `Result::unwrap()` on an `Err` value: NotConverged { iterations: 30, final_step: (0.00045227832, 0.0), epsilon: 1e-10 } ``` Its fixture asked for `epsilon: 1e-10` inside `max_iter: 30` and never got there. That was invisible while `converge` returned short fits silently, and became a panic when it stopped. It now uses the default `ITERATIONS` cap — measuring a factorisation on an unconverged fit is measuring something nobody would run. I checked the other four benchmarks; they are fine. This is itself evidence for the issue's premise. A benchmark nobody runs is a benchmark that has been broken for an unknown length of time. ## Two bugs in the report step, both caught by running it - `asort` is a **gawk extension** and `ubuntu-latest`'s `awk` is mawk. It failed immediately on a local sample. Sorting goes through `sort -n` now. - Criterion picks its unit per run, so a mixed batch would compare `9 ms` against `9 µs` as if they were the same number — exactly the plausible-looking wrong answer this crate keeps removing. The job now refuses to report a spread unless every run agrees on the unit. Both the firing and the passing case were tested against real criterion output. Leaving this open until the job has been run and the threshold decided.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#54