Merge pull request #353 from marius-bughiu/marius-bughiu-fix-issue-351-benchmark-flag-noise-guard

fix(ci): flag a benchmark row on a 3-sigma bar, not a 1-sigma sum

Author: marius-bughiu

.github/workflows/benchmarks.yml

@@ -305,181 +305,57 @@ jobs:
           retention-days: 30
 
       # ----- PR path: post the same-runner A/B comment -----
+      #
+      # The body is built by a script rather than inline here, because what it decides —
+      # which rows are flagged as a regression — is a statistical judgement that has to be
+      # testable. Inline in this file it was neither runnable nor pinned, and it shipped a
+      # guard that flagged 13 of 757 rows on a diff with zero IL in it (#351).
+      # `scripts/benchmark_comment.js --self-test` runs on every PR from `ci.yml` and pins
+      # the rule against the real measurements from that run.
+      - name: Build benchmark comment
+        if: github.event_name == 'pull_request' && steps.merge.outputs.have_head == 'true' && steps.merge.outputs.have_base == 'true'
+        env:
+          # A row is flagged when it moves past this ratio AND the gap exceeds
+          # ALERT_NOISE_SIGMAS times the two measurements' combined standard deviation,
+          # added in quadrature. Both defaults live in the script; they are repeated here
+          # because this is where someone tuning them will look first.
+          ALERT_THRESHOLD_RATIO: '1.10'
+          ALERT_NOISE_SIGMAS: '3'
+          MISSING_SHARDS: ${{ steps.merge.outputs.missing_shards }}
+        run: >-
+          node scripts/benchmark_comment.js
+          --pr /tmp/pr-report-full.json
+          --base /tmp/base-report-full.json
+          --out /tmp/benchmark-comment.md
+
       - name: Post benchmark comment
         if: github.event_name == 'pull_request' && steps.merge.outputs.have_head == 'true' && steps.merge.outputs.have_base == 'true'
         uses: actions/github-script@v7
         env:
-          ALERT_THRESHOLD_RATIO: '1.10'
           MISSING_SHARDS: ${{ steps.merge.outputs.missing_shards }}
         with:
           github-token: ${{ secrets.GITHUB_TOKEN }}
           script: |
             const fs = require('fs');
-
-            const pr = JSON.parse(fs.readFileSync('/tmp/pr-report-full.json', 'utf8'));
-            const base = JSON.parse(fs.readFileSync('/tmp/base-report-full.json', 'utf8'));
-
-            // Index base results by full benchmark name.
-            const baseMap = new Map();
-            for (const b of base.Benchmarks) {
-              baseMap.set(b.FullName, b.Statistics);
+            // The one definition of the marker, imported rather than copied: the string
+            // must match what the generated body carries and what the search below looks
+            // for, and a second copy here is a silent way for those to drift apart.
+            // github-script resolves a relative require against the workspace root.
+            const { COMMENT_MARKER: marker } = require('./scripts/benchmark_comment.js');
+
+            const body = fs.readFileSync('/tmp/benchmark-comment.md', 'utf8');
+            // Catches a truncated or empty build step, which would otherwise post a blank
+            // comment and orphan the previous one.
+            if (!body.includes(marker)) {
+              core.setFailed('The generated comment is missing its marker; posting it would orphan the previous one.');
+              return;
             }
 
-            const formatNs = (ns) => {
-              if (ns == null) return 'n/a';
-              if (ns < 1000) return `${ns.toFixed(1)} ns`;
-              if (ns < 1_000_000) return `${(ns/1000).toFixed(2)} μs`;
-              if (ns < 1_000_000_000) return `${(ns/1_000_000).toFixed(2)} ms`;
-              return `${(ns/1_000_000_000).toFixed(2)} s`;
-            };
-
-            const threshold = parseFloat(process.env.ALERT_THRESHOLD_RATIO);
-            const thresholdPct = ((threshold - 1) * 100).toFixed(0);
-            // Each entry: { name, isHasher, prCell, stdCell, baseCell, deltaCell, flag }
-            // where flag is 'regression' | 'improvement' | null.
-            const entries = [];
-            let regressions = 0;
-            let improvements = 0;
-            let errored = 0;
-
-            for (const b of pr.Benchmarks) {
-              const name = b.FullName;
-              // BenchmarkDotNet emits a benchmark with null Statistics and empty
-              // Measurements when that case errored or was not run (a throwing
-              // [GlobalSetup], a cancelled shard, an OOM, ...). Surface it as a row
-              // rather than dereferencing null — one errored case must never crash
-              // the aggregate job and red-X the whole PR gate.
-              if (b.Statistics == null) {
-                errored++;
-                const baseStats = baseMap.get(name);
-                entries.push({
-                  name,
-                  isHasher: /Hasher/.test(name),
-                  prCell: '⚠️ errored',
-                  stdCell: 'n/a',
-                  baseCell: baseStats ? formatNs(baseStats.Mean) : 'n/a',
-                  deltaCell: 'no measurements',
-                  flag: null,
-                });
-                continue;
-              }
-              const prMean = b.Statistics.Mean;
-              const prStdDev = b.Statistics.StandardDeviation;
-              const baseStats = baseMap.get(name);
-              let deltaCell = baseMap.has(name) ? '⚠️ base errored' : '🆕 new';
-              let flag = null;
-              if (baseStats) {
-                const baseMean = baseStats.Mean;
-                const baseStdDev = baseStats.StandardDeviation;
-                const ratio = prMean / baseMean;
-                const pct = (ratio - 1) * 100;
-                const sign = pct >= 0 ? '+' : '';
-                deltaCell = `${sign}${pct.toFixed(1)}%`;
-                // Flag only when the change clears the threshold AND exceeds the
-                // combined run-to-run noise of both measurements.
-                const beyondNoise = Math.abs(prMean - baseMean) > (prStdDev + baseStdDev);
-                if (ratio >= threshold && beyondNoise) {
-                  deltaCell += ' ⚠️';
-                  flag = 'regression';
-                  regressions++;
-                } else if (ratio <= (1 / threshold) && beyondNoise) {
-                  deltaCell += ' ✅';
-                  flag = 'improvement';
-                  improvements++;
-                }
-              }
-              entries.push({
-                name,
-                // Hasher throughput benchmarks (StringHasherBenchmark,
-                // IntegerHasherBenchmark) get their own section; everything else
-                // is a collection benchmark.
-                isHasher: /Hasher/.test(name),
-                prCell: formatNs(prMean),
-                stdCell: formatNs(prStdDev),
-                baseCell: baseStats ? formatNs(baseStats.Mean) : 'n/a',
-                deltaCell,
-                flag,
-              });
-            }
-
-            let subtitle;
-            if (regressions > 0) {
-              subtitle = `${regressions} regression${regressions === 1 ? '' : 's'} ⚠️ vs main` +
-                (improvements > 0 ? `, ${improvements} improvement${improvements === 1 ? '' : 's'} ✅` : '') +
-                ` (rows past ±${thresholdPct}% beyond noise).`;
-            } else if (improvements > 0) {
-              subtitle = `No regressions — ${improvements} improvement${improvements === 1 ? '' : 's'} ✅ vs main.`;
-            } else {
-              subtitle = `No significant change vs main (all within ±${thresholdPct}% or noise).`;
-            }
-            if (errored > 0) {
-              subtitle += ` ⚠️ ${errored} benchmark${errored === 1 ? '' : 's'} errored (no measurements) — see the table.`;
-            }
-
-            const baseSha = (process.env.BASE_SHA || '').slice(0, 7);
-            const footer = `<sub>Same-runner A/B (sharded ${process.env.SHARD_TOTAL || ''}-way): main (\`${baseSha}\`) and this PR were built and benchmarked back-to-back on the same runner per shard, so hardware variance cancels out. ⚠️ = PR mean ≥ +${thresholdPct}% slower than main and beyond combined std-dev; ✅ = correspondingly faster.</sub>`;
-
-            const header = ['| Benchmark | This PR | StdDev | main | Δ |', '|---|---:|---:|---:|---:|'];
-            const toRow = (e) => `| \`${e.name}\` | ${e.prCell} | ${e.stdCell} | ${e.baseCell} | ${e.deltaCell} |`;
-
-            // A collapsible section per benchmark family, collapsed by default so
-            // the (often large) tables stay below the fold.
-            const section = (title, list) => {
-              if (list.length === 0) return [];
-              return [
-                '<details>',
-                `<summary><b>${title}</b> (${list.length})</summary>`,
-                '',
-                ...header,
-                ...list.map(toRow),
-                '</details>',
-                '',
-              ];
-            };
-
-            // Highlights: only the flagged rows stay above the fold so reviewers
-            // see what actually moved without expanding either table.
-            const flagged = entries.filter(e => e.flag);
-            const highlights = flagged.length === 0 ? [] : [
-              '**Highlights**',
-              '',
-              ...header,
-              ...flagged.map(toRow),
-              '',
-            ];
-
-            const collections = entries.filter(e => !e.isHasher);
-            const hashers = entries.filter(e => e.isHasher);
-
-            // A partial merge otherwise reads exactly like a complete one: the tables are
-            // well-formed and simply have fewer rows, so a silently-dropped shard looks
-            // like a clean report. Say it above the fold, before any numbers.
             const missingShards = (process.env.MISSING_SHARDS || '').trim();
-            const incomplete = missingShards.length === 0 ? [] : [
-              `> [!WARNING]`,
-              `> **Incomplete report.** Shard(s) \`${missingShards}\` produced no measurements, so every`,
-              `> benchmark class packed onto them is missing from the tables below — including, possibly,`,
-              `> the ones this PR changed. Treat the comparison as partial rather than as a clean run.`,
-              '',
-            ];
             if (missingShards.length > 0) {
               core.warning(`Benchmark comparison is missing shard(s) ${missingShards}.`);
             }
 
-            const marker = '<!-- celerity-benchmarks-comment -->';
-            const body = [
-              marker,
-              '## Benchmarks',
-              '',
-              ...incomplete,
-              subtitle,
-              '',
-              ...highlights,
-              ...section('Collections', collections),
-              ...section('Hashers', hashers),
-              footer,
-            ].join('\n');
-
             const { data: comments } = await github.rest.issues.listComments({
               owner: context.repo.owner,
               repo: context.repo.repo,

.github/workflows/ci.yml

@@ -143,7 +143,9 @@ jobs:
   # The benchmark relevance gate decides whether an expensive sharded A/B run happens at
   # all, so the C# lexer it rests on is pinned here rather than only exercised in the
   # workflow it gates — where a wrong answer costs either runner hours or an unmeasured
-  # regression, and neither failure announces itself.
+  # regression, and neither failure announces itself. The comment builder is pinned
+  # alongside it for the same reason: it decides which rows are called a regression, and
+  # a guard that is too permissive is indistinguishable from a clean report (#351).
   benchmark-gate:
     name: benchmark-gate
     runs-on: ubuntu-latest
@@ -156,6 +158,9 @@ jobs:
       - name: Pin the comment-stripping lexer
         run: node scripts/benchmark_relevant_changes.js --self-test
 
+      - name: Pin the regression-flag rule
+        run: node scripts/benchmark_comment.js --self-test
+
   aot-publish:
     name: aot-publish (linux-x64, ${{ matrix.tfm }})
     runs-on: ubuntu-latest

CHANGELOG.md

@@ -26,12 +26,15 @@ All notable changes to Celerity are documented here. This project follows [Keep
 - A "Documentation links" section in `CONTRIBUTING.md` covering the slug rule and how to look an anchor up rather than guess it. Closes [#339](https://github.com/marius-bughiu/Celerity/issues/339).
 - `scripts/benchmark_relevant_changes.js` — a CI gate that skips the sharded benchmark run on a pull request whose diff cannot move a measured number: documentation, the test / fuzz / AOT-smoke projects, or comments inside `.cs` files. It skips only what it can prove inert and never applies to `main`. Closes [#335](https://github.com/marius-bughiu/Celerity/issues/335).
 - A `--shard-dry-run` switch on the benchmarks runner that resolves a shard's class list without measuring anything. Closes [#300](https://github.com/marius-bughiu/Celerity/issues/300).
+- `scripts/benchmark_comment.js` — the pull-request benchmark comment, moved out of `benchmarks.yml` so the rule deciding which rows count as a regression is runnable and testable, with a `--self-test` wired into `ci.yml`. Closes [#351](https://github.com/marius-bughiu/Celerity/issues/351).
+- That comment now publishes the run's own **measured noise floor** — the p50, p90 and p95 of |Δ| across every paired row, of which any one pull request changes only a handful — so a flag can be read against the drift it arrived in instead of an assumed one. Closes [#351](https://github.com/marius-bughiu/Celerity/issues/351).
 
 ### Fixed
 
 - Pushing to a pull request now supersedes that PR's in-flight benchmark run instead of stacking another eight-runner matrix behind it, so `CI` and `Coverage` no longer queue behind superseded perf runs. Pushes to `main` are keyed per commit and never cancelled. Closes [#319](https://github.com/marius-bughiu/Celerity/issues/319).
 - A benchmark shard no longer times out on a pull request that adds a benchmark class: the `main` base now replays the class list the PR head resolved, so shard *i* is the same slice on both sides. The job budget was also resized to the measured slices, which the suite had outgrown. Closes [#300](https://github.com/marius-bughiu/Celerity/issues/300).
 - A benchmark comparison that is missing a shard now says so in the PR comment, instead of reading exactly like a complete run. Closes [#300](https://github.com/marius-bughiu/Celerity/issues/300).
+- The PR benchmark comment no longer cries wolf: its noise guard is now a **3σ** bar over the two measurements' combined standard deviation, added in quadrature, rather than a 1σ sum. Replayed over a run whose library IL was byte-identical to `main`, that cuts flagged rows from **13 to 2** while detecting the same regressions on every benchmark precise enough to resolve one. Closes [#351](https://github.com/marius-bughiu/Celerity/issues/351).
 
 - `PartialSort.TopK` now throws `ArgumentException` when its `destination` overlaps its `source`, instead of silently returning a wrong answer and writing to the source it documents as untouched. Disjoint slices of one array are still accepted, matching `RadixSort` and `CountingSort`.
 - Corrected `RadixSort.ArgSort` XML documentation: only its `ReadOnlySpan<int>` overload rejects `indices` that shares storage with `keys`. Documentation only.

CONTRIBUTING.md

@@ -82,14 +82,15 @@ dotnet run -c Release -- --filter '*' # run everything with the default (slow, h
 
 Results are parsed by [`benchmark-action/github-action-benchmark`](https://github.com/benchmark-action/github-action-benchmark) and:
 
-- **On a PR**: a comment is posted with the same-runner A/B comparison vs `main`. Rows that move by more than ±10% *and* beyond the combined standard deviation of both measurements are flagged; the flags are advisory, so a noisy row does not fail the job. If any shard failed to report, the comment says so above the fold — a partial comparison is otherwise indistinguishable from a clean one.
+- **On a PR**: a comment is posted with the same-runner A/B comparison vs `main`. A row is flagged when it moves past ±10% *and* the gap exceeds **3σ** of the two measurements' combined standard deviation (added in quadrature); the flags are advisory, so a noisy row does not fail the job. The comment also publishes that run's **measured noise floor** — the p50, p90 and p95 of |Δ| across every paired row, of which any one PR changes only a handful — so a flag can be read against the drift it arrived in rather than against an assumed one. If any shard failed to report, the comment says so above the fold — a partial comparison is otherwise indistinguishable from a clean one.
 - **On a push to `main`**: the new measurement is appended to the `gh-pages`-stored history powering the dashboard at <https://marius-bughiu.github.io/Celerity/dev/bench/>.
 
 Three things about the run are worth knowing before you wonder why it did or did not happen:
 
 - **It supersedes itself.** Pushing to a PR cancels that PR's in-flight benchmark run rather than stacking another eight-runner matrix behind it; only the newest numbers are ever read. Pushes to `main` are keyed per commit instead, so none is ever cancelled and the published history has no gaps.
 - **It is skipped when the diff cannot move a number.** [`scripts/benchmark_relevant_changes.js`](scripts/benchmark_relevant_changes.js) gates the PR path: a diff that touches only documentation, only the test / fuzz / AOT-smoke projects, or only comments inside `.cs` files does not buy a three-hour A/B run. The gate is one-directional — anything it cannot prove inert (an added or deleted file, a `.csproj`, a git command that fails) runs the suite — and it never applies to `main`, so a wrongly-skipped PR is still measured on merge. Run it yourself with `node scripts/benchmark_relevant_changes.js <base> <head>`.
 - **Shard *i* means the same slice on both sides.** The base run replays the class list the head resolved instead of packing its own. Shard membership comes from bin-packing over the benchmark class list, so a PR that *adds* a benchmark class would otherwise pack the two sides differently and could pair a light head slice with a heavy base one.
+- **A flag is evidence, not a verdict.** Even at 3σ, two rows in a ~750-row run still flagged on a diff whose IL was byte-identical to `main`: a case whose two builds land in different code or data layouts shifts by tens of percent with a tight spread on both sides, and nothing inside a single A/B pass separates that from a real change. Re-run before acting on a flag near the run's published p95. The rule lives in [`scripts/benchmark_comment.js`](scripts/benchmark_comment.js), which documents how the 3σ bar was calibrated; `node scripts/benchmark_comment.js --self-test` pins it against the measurements it was chosen against.
 
 If a change is motivated by performance, include before/after numbers from a local Release run in the PR description — the CI job is a guardrail, not a precision instrument. Numbers without `-c Release` are not useful — BenchmarkDotNet refuses to run in Debug.