|
| 1 | +--- |
| 2 | +name: bench-regression |
| 3 | +description: Check a PR for performance regressions by comparing its benchmark run against its merge-base on next. Use when a dev asks "did my PR regress anything?" or wants to vet benchmark impact before merge. |
| 4 | +argument-hint: [pr-ref] (defaults to HEAD) |
| 5 | +--- |
| 6 | + |
| 7 | +# PR benchmark regression check |
| 8 | + |
| 9 | +Compares a PR's benchmark run against its baseline (merge-base on `next`, optionally a window of |
| 10 | +baseline runs) and reports only the regressions that (a) are statistically real and (b) plausibly |
| 11 | +relate to what the PR changed. The data-pulling and diff math are done by `ci3/bench_compare`; your |
| 12 | +job is to get a run, invoke the script, then filter the output with judgement. |
| 13 | + |
| 14 | +## How benchmark data is stored (context) |
| 15 | + |
| 16 | +Each CI benchmark run uploads a single `bench-out/bench.json` (a flat array of `{name, unit, value}`, |
| 17 | +`customSmallerIsBetter` — higher = worse) to the build-cache, keyed by the commit's **git tree hash** |
| 18 | +(`bench-<treehash>.tar.gz`, ~40 KB). A PR only produces one on an *uploadable* run: labels |
| 19 | +`ci-full-no-test-cache`, `ci-full`, or a merge-queue run. `ci3/bench_compare` pulls these per-run |
| 20 | +blobs — never the multi-MB per-branch `data.js` graph history. |
| 21 | + |
| 22 | +## Steps |
| 23 | + |
| 24 | +### 1. Get a benchmark run for the PR |
| 25 | + |
| 26 | +- Default to `HEAD` (or the ref the user gives). Try step 2 directly — `ci3/bench_compare` fails |
| 27 | + clearly if the commit has no bench data. |
| 28 | +- **If there's no run and the user is fine using the last one:** walk back the branch for the most |
| 29 | + recent benched commit — `for c in $(git rev-list -n 30 HEAD); do ci3/bench_compare "$c" >/dev/null 2>&1 && echo "$c" && break; done` — and use that commit. |
| 30 | +- **If a fresh run is needed:** tell the user to add the `ci-full-no-test-cache` label to the PR (that |
| 31 | + triggers x-bench on a dedicated metal box and uploads) and to re-run this once CI finishes. Do not |
| 32 | + block waiting unless asked. |
| 33 | + |
| 34 | +### 2. Determine the baseline branch — do NOT assume `next` |
| 35 | + |
| 36 | +The comparison is `merge-base(PR, baseline)`, so the baseline MUST be the PR's *actual* target branch. |
| 37 | +A PR onto `v5-next` or a `merge-train/*` branch compared against `next` would report the entire |
| 38 | +branch-vs-branch perf delta as bogus regressions. Get it from the PR itself: |
| 39 | + |
| 40 | +```bash |
| 41 | +gh pr view <pr-or-branch> --json baseRefName -q .baseRefName # e.g. next, v5-next, merge-train/spartan |
| 42 | +git fetch origin <baseRefName> # ensure origin/<baseRefName> is current |
| 43 | +``` |
| 44 | + |
| 45 | +(You do NOT need to worry about cross-branch data mixups: the bench cache is content-addressed by git |
| 46 | +tree hash, so `next`, `v5-next`, etc. already occupy different keys — a next commit's tree is only ever |
| 47 | +benched by next-content. The only thing you control, and must get right, is *which branch's lineage* |
| 48 | +you merge-base against.) |
| 49 | + |
| 50 | +### 3. Run the comparison |
| 51 | + |
| 52 | +```bash |
| 53 | +ci3/bench_compare <pr-commit> --baseline origin/<baseRefName> --window 5 --out /tmp/bench-report.json |
| 54 | +``` |
| 55 | + |
| 56 | +- Always pass `--baseline` explicitly (the script auto-detects via `gh` if omitted, but be explicit). |
| 57 | + It prints `baseline = …` to stderr — confirm it matches the PR's target before trusting the numbers. |
| 58 | +- `--window 5` averages the merge-base + 4 preceding baseline runs, yielding `stddev` and `z` per bench |
| 59 | + — essential for separating real regressions from flaky benches. Use `--window 1` if history is sparse. |
| 60 | +- JSON on `--out` is `{ meta, benches:[{name,unit,pr,baseline,n,pct,stddev,z,status}] }`, sorted by |
| 61 | + `pct` desc. Human summary on stderr. |
| 62 | + |
| 63 | +### 4. Filter to what matters (your judgement) |
| 64 | + |
| 65 | +Read the JSON and keep a regression only if **all** hold: |
| 66 | + |
| 67 | +- **Statistically real:** `z >= ~4` (the PR value is ≥4 baseline-stddevs above the mean). A large |
| 68 | + `pct` with small/absent `z` is baseline noise — a flaky bench (e.g. `p2p/BatchTxRequester/.../duration` |
| 69 | + hitting a 30 s timeout, `pct` in the thousands) — **ignore it**. When window is 1 (no `z`), fall back |
| 70 | + to requiring a large `pct` AND a meaningful absolute magnitude. |
| 71 | +- **Material magnitude:** skip tiny absolutes (e.g. `0.01 ms -> 0.02 ms` = +100% but noise). |
| 72 | +- **Plausibly caused by the PR:** map the bench `name` prefix to the PR's changed areas. Get the diff |
| 73 | + with `git diff --name-only $(git merge-base HEAD origin/next)..HEAD | cut -d/ -f1-2 | sort -u`, then |
| 74 | + keep benches whose names start with a touched area (`yarn-project/simulator`, `barretenberg/cpp`, |
| 75 | + `avm-transpiler`, …) and treat far-away regressions (a TS-only PR moving a C++ proving bench) as |
| 76 | + drift/noise, not this PR. |
| 77 | + |
| 78 | +### 5. Report |
| 79 | + |
| 80 | +List the surviving regressions with `baseline -> pr unit`, `pct`, and `z`, grouped by area, and give a |
| 81 | +one-line verdict (clean / N real regressions in <area>). Note anything you filtered out and why, so |
| 82 | +the dev can override your judgement. |
| 83 | + |
| 84 | +## Notes |
| 85 | + |
| 86 | +- Baseline is `origin/next` by default; `git fetch` it first if stale (`ci3/bench_compare` errors if |
| 87 | + the ref is missing). |
| 88 | +- Requires `jq`, `python3`, and read access to the build-cache (the public `build-cache.aztec-labs.com` |
| 89 | + endpoint needs no creds; the in-VPC S3 path is a fallback). |
| 90 | +- To author or wire up new benchmarks, see the `adding-benchmarks` skill; this skill only *checks* them. |
0 commit comments