ReviewOS

also looking at this

craft-native/craft

fix(ci): make the benchmark gate measure something, and stop it failing at random

#75
Merged glennmichael123 wants to merge fix/benchmark-gate into main
5 files +263 -145
11 days ago

The benchmark gate ran craft --help five times, averaged the wall clock in whole milliseconds, compared that against a number cached from a main build on another runner on another day, and failed the PR at 20% drift.

It could not see a real regression, and it failed on ones that did not exist.

It never measured startup

craft --help parses arguments, prints, and exits. It never opens a window, never touches AppKit or GTK, never starts a WebContent process. The step called it "Startup Time" — which is how a number that cannot move ended up gating the code that would move it.

Observed on this repo, this week:

PRwhat it changesmeasuredresult
baseline (cached, other runner)—41ms—
#73 — installs an NSApplicationDelegate on every startup pathmore startup work30ms✅ passed
#71 — macOS bridge dispatcher only, unreachable from --helpnothing53ms, then 50ms❌ failed twice

The PR that adds startup work measured 40% faster than the one that adds none.

The noise is more than double the threshold

Measured here on one unchanging binary:

n=60   min 19.8ms   p50 22.9ms   p95 38.8ms   max 65.6ms   stdev 7.1ms

Heavy-tailed — the distribution a mean-of-five is least able to summarise. Running the gate's own method twelve times against that same binary:

22 22 23 23 23 23 24 24 26 26 28 32   →  45% spread, 20% trip point

No code was involved in any of that.

The fix: compare two binaries, not a binary and a memory

The cache now holds main's binary instead of main's timing. Both are measured interleaved on the same runner in the same sitting and compared by p50. Whatever slows one round slows both halves of it, so nearly all the variance cancels:

interleaved A/B, byte-identical binaries:
  trial 1: +1.7%   trial 2: -3.5%   trial 3: +2.4%   trial 4: -0.5%   trial 5: +1.9%
  worst |delta|: 3.5%          (gate fires at 20%)

~13× less false signal, and the 20% threshold finally has room to mean something.

Interleaving is what does the work. Measuring all of A then all of B would attribute the drift between the halves to the change. The script also alternates which binary goes first each round, so neither sits in a fixed position.

What is deliberately not changed

The measurement stays --help, because a CI runner has no display and a real startup benchmark needs one. What changes is that it is called what it is — binary load time — in the step name, the job summary, the PR comment and the README. benchmarks/startup.bench.ts already measures real startup (window → webview → first paint) and stays a by-hand benchmark.

Two more bugs found while in there

  • The memory metric measured the wrong process. It read /proc/self/status inside the run step, so "Memory Usage" was the shell's RSS and never craft's. Dropped rather than reported.
  • The gate could not have fired. It took $? after a tee pipeline, which is tee's status. Rewritten to redirect-then-cat, with a comment saying why — relying on pipefail being set is not something a gate should do.

How it's tested

The comparison lives in benchmarks/binary-load-ab.ts rather than in YAML, so it runs and is checkable by hand. All four paths verified locally with the exact shell from the workflow:

caseresult
byte-identical binariesregressed=0, delta -0.05%
binary wrapped to be slowerregressed=1, delta +97.65%
no baseline cachedcompared=false, reports without gating
--threshold 3.0 on the slow binarypasses — threshold honoured, not hardcoded

Workflow YAML parsed and asserted: 15 steps, none malformed, gate condition wired to the script's exit code.

bunx --bun pickier . — 38 warnings, unchanged from main.

Note

Opened during the GitHub Actions outage, so this has no CI run yet either. It will queue with the rest.

5 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.