From 0801acebd182cafdbcbdde6c440f4849158437c2 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Thu, 10 Sep 2026 06:55:25 +0200 Subject: [PATCH] ci: measure the runner's own benchmark variance, and fix the joint bench MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #54 asks whether benchmark regressions can be gated. The threshold is the whole problem — too tight and CI goes red on noise, which trains the reflex to re-run until green; too loose and it never fires — and which of those is possible depends on a number nobody has measured. This adds a manually-triggered job that runs one unchanged benchmark ten times and reports min/median/max/mean and the spread. `joint_factorise_480_appearances` is the probe: ~9 ms, long enough not to be dominated by timer overhead, and the measurement this crate most wants protected — it is the dense factorisation #52 is about replacing. `benches/joint.rs` did not run at all. Its fixture asked for `epsilon: 1e-10` within `max_iter: 30` and never got there, so once `converge` stopped returning short fits silently it panicked: NotConverged { iterations: 30, final_step: (4.5e-4, 0.0), epsilon: 1e-10 } It now uses the default `ITERATIONS` cap. Measuring a factorisation on an unconverged fit would have been measuring something nobody runs. The other four benchmarks were checked and are fine. Two things in the report step were got wrong first and fixed by running them, not by reading them: - `asort` is a gawk extension and the runner's `awk` is mawk. Sorting goes through `sort -n` instead. - Criterion picks a unit per run, so a mixed batch would compare 9 ms against 9 us as though they were the same number. The job refuses to report a spread unless every run agrees on the unit. Refs #54. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- .gitea/workflows/bench-variance.yml | 91 +++++++++++++++++++++++++++++ benches/joint.rs | 8 ++- 2 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 .gitea/workflows/bench-variance.yml diff --git a/.gitea/workflows/bench-variance.yml b/.gitea/workflows/bench-variance.yml new file mode 100644 index 0000000..23c4fd3 --- /dev/null +++ b/.gitea/workflows/bench-variance.yml @@ -0,0 +1,91 @@ +# Measure the CI runner's own benchmark variance. +# +# #54 asks whether benchmark regressions can be gated. The threshold is the +# whole problem: too tight and CI goes red on noise, which trains people to +# re-run until green; too loose and it never fires. Which of those is possible +# depends on a number nobody has measured — how much this runner's results move +# between identical runs. +# +# So: run one unchanged benchmark ten times and report the spread. If it is +# ~15%, a fixed-threshold gate is dead and the answer is a tracker; if it is +# ~2%, a gate at 10% is meaningful. +# +# Manual only. It takes ten benchmark runs and answers a question that is asked +# once, not every push. +name: Benchmark variance + +on: + workflow_dispatch: + inputs: + runs: + description: How many repeats + required: false + default: "10" + +jobs: + variance: + name: runner variance on one benchmark + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + + # `joint_factorise_480_appearances` is the right probe: ~9 ms, so it is + # long enough not to be dominated by timer overhead, and it is the + # measurement this crate most wants protected — the dense factorisation + # #52 is about replacing. + - name: Warm up + run: cargo bench --bench joint -- joint_factorise_480_appearances --warm-up-time 1 --measurement-time 3 + + - name: Repeat the same benchmark + run: | + set -euo pipefail + for i in $(seq 1 "${{ inputs.runs || '10' }}"); do + echo "== run $i ==" + cargo bench --bench joint -- \ + joint_factorise_480_appearances --warm-up-time 1 --measurement-time 3 \ + 2>&1 | tee -a raw.txt + done + + - name: Report the spread + run: | + set -euo pipefail + # Criterion prints `time: [lo mid hi]` with a unit after each. Take + # the midpoints. `sort -n` rather than awk's `asort`, which is a gawk + # extension the runner's mawk does not have — that failed on the + # first try here. + grep -oE 'time:[[:space:]]+\[[^]]+\]' raw.txt \ + | sed -E 's/.*\[[^ ]+ [^ ]+ ([0-9.]+) ([^ ]+).*/\1 \2/' > mids.txt + echo "--- midpoints ---" + cat mids.txt + # Criterion picks a unit per run, so mixed units would have us + # comparing 9 ms against 9 us as if they were the same number — the + # plausible-looking wrong answer this crate keeps removing. Refuse. + if [ "$(cut -d' ' -f2 mids.txt | sort -u | wc -l)" -ne 1 ]; then + echo "runs reported different units; the spread would be meaningless" + cut -d' ' -f2 mids.txt | sort | uniq -c + exit 1 + fi + sort -n mids.txt | awk '{ v[NR]=$1; u=$2; s+=$1 } + END { + if (NR == 0) { print "no samples parsed - see the raw.txt artifact"; exit 1 } + printf "n = %d\n", NR + printf "min = %.4f %s\n", v[1], u + printf "median = %.4f %s\n", v[int((NR+1)/2)], u + printf "max = %.4f %s\n", v[NR], u + printf "mean = %.4f %s\n", s/NR, u + printf "spread = %.2f%% (max-min)/min\n", 100*(v[NR]-v[1])/v[1] + print "" + print "Read it against #54: a spread near 15% kills both" + print "fixed-threshold options and the answer is a tracker;" + print "a spread near 2% makes a gate at 10% meaningful." + }' + + - uses: actions/upload-artifact@v4 + if: always() + with: + name: bench-variance-raw + path: | + raw.txt + mids.txt diff --git a/benches/joint.rs b/benches/joint.rs index e56b9cd..a535f7a 100644 --- a/benches/joint.rs +++ b/benches/joint.rs @@ -18,8 +18,14 @@ fn fitted() -> History { .beta(1.0) .score_sigma(2.0) .drift(ConstantDrift::new(0.05)) + // `max_iter: 30` was here, and this fixture needs more: `converge` + // reported `NotConverged { iterations: 30, final_step: (4.5e-4, 0.0) }` + // once it stopped returning short fits silently. The benchmark measures + // the factorisation, whose cost depends on the fit's *shape* rather + // than its exactness — but measuring it on an unconverged fit is still + // measuring something nobody would run. .convergence(ConvergenceOptions { - max_iter: 30, + max_iter: trueskill_tt::ITERATIONS, epsilon: 1e-10, alpha: 1.0, })