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

fix(ci): count a base-side benchmark failure, and stop calling the run's spread a noise floor

Author: marius-bughiu

CHANGELOG.md

@@ -27,14 +27,15 @@ All notable changes to Celerity are documented here. This project follows [Keep
 - `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).
+- That comment now publishes the run's own **observed spread** — the p50, p90 and p95 of |Δ| across every paired row — so a flag can be read against the run it arrived in instead of an assumed floor. 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).
+- A benchmark whose `main` side failed to measure, or whose two means cannot be compared at all, is now counted in that comment's warning total. Previously such a row appeared in the table while the summary line still read "No significant change vs main". 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,7 +82,7 @@ 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`. 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 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 **observed spread** — the p50, p90 and p95 of |Δ| across every paired row — so a flag can be read against the run it arrived in rather than against an assumed floor. On a typical PR that spread is almost all runner drift, but a change to a shared primitive moves many rows at once and raises those figures itself, so it is not a floor the PR cannot have caused. 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:

scripts/benchmark_comment.js

@@ -12,7 +12,7 @@
 // (8 regressions, 5 improvements) out of 757, four of them on `Dictionary<,>` / `HashSet<>`
 // baseline arms whose code is the same on both sides by definition (#351).
 //
-// The measured noise floor of that run, which is the reason no purely per-row rule can
+// The measured spread of that run, which is the reason no purely per-row rule can
 // reach zero: |Δ| has a p50 of 0.7% and a p90 of 6.2%, but 43 rows exceed ±10%,
 // 16 exceed ±25% and the worst is 222%. The distribution is sharply peaked with a fat tail
 // of individually unstable cases — sensitive to code and data layout, which differs
@@ -47,7 +47,7 @@
 // sides (`SmallDictionaryBenchmark.Dictionary_Remove(ItemCount: 64)`, +33.1% with 3.4% and
 // 3.5% relative SD). Both launches of each side agree closely, so nothing inside a single
 // A/B pass can tell it from a real change; only re-measuring against a fresh pair of builds
-// can. That residual is why the comment publishes the run's measured noise floor rather
+// can. That residual is why the comment publishes the run's own observed spread rather
 // than presenting a flag as a verdict.
 //
 // Usage:
@@ -114,7 +114,7 @@ function classifyDelta(prMean, prStdDev, baseMean, baseStdDev, thresholdRatio, n
 
 // Nearest-rank percentile over an already-sorted ascending array: the reported value is
 // always an |Δ| some row actually exhibited, never an interpolation between two rows. That
-// is the right choice for a noise floor — "p95 is 10.3%" should name an observed
+// is the right choice for a published spread — "p95 is 10.3%" should name an observed
 // measurement — but it does mean p50 is not the textbook median on an even-length sample,
 // which is why nothing below calls it one.
 function percentile(sorted, p) {
@@ -123,12 +123,23 @@ function percentile(sorted, p) {
   return sorted[Math.min(sorted.length - 1, Math.max(0, rank - 1))];
 }
 
-// The run's own noise floor, measured rather than assumed (#351). Any one pull request
-// changes a handful of the ~750 tracked cases, so the spread of the whole delta
-// distribution is dominated by what the runner did between the head slice and the base
-// slice. Publishing it on every comment means the threshold can be argued about against
-// evidence, and a run that was unusually hostile says so instead of looking clean.
-function noiseProfile(deltaPercents) {
+// The run's own delta distribution, measured rather than assumed (#351). On a typical pull
+// request — a handful of the ~750 tracked cases touched — this is a noise floor: almost
+// every row in it is the runner's own drift between the head slice and the base slice.
+//
+// It is not *only* that, which is why neither this function nor the footer it feeds calls
+// it a noise floor. A change to a shared primitive or to the benchmark configuration moves
+// many rows at once, and those real effects sit in the same sample. Deriving the floor from
+// a control group instead was considered and rejected: the obvious controls are the BCL
+// baseline arms, but a layout shift moves those too, so they would understate the tail
+// while looking authoritative. Reporting the distribution and saying what is in it is the
+// honest version.
+//
+// `noiseSigmas` and `ALERT_NOISE_SIGMAS` keep their names on purpose. That guard is scaled
+// by the two measurements' own standard deviations, which really are per-row measurement
+// noise — a different quantity from the run-wide distribution measured here, and one the
+// word describes correctly.
+function spreadProfile(deltaPercents) {
   if (deltaPercents.length === 0) return null;
   const sorted = [...deltaPercents].sort((a, b) => a - b);
   return {
@@ -199,10 +210,21 @@ function buildComment(prReport, baseReport, options = {}) {
     const prMean = b.Statistics.Mean;
     const prStdDev = b.Statistics.StandardDeviation;
     const baseStats = baseMap.get(name);
-    let deltaCell = baseMap.has(name) ? '⚠️ base errored' : '🆕 new';
+    let deltaCell;
     let flag = null;
 
-    if (baseStats) {
+    if (!baseStats) {
+      // Present in the base report but with no statistics means the base side of this pair
+      // errored, so there is nothing to compare against. Counted, because a subtitle
+      // reading "No significant change" over a row that could not be measured is the same
+      // false reassurance a too-permissive guard gives.
+      if (baseMap.has(name)) {
+        errored++;
+        deltaCell = '⚠️ base errored';
+      } else {
+        deltaCell = '🆕 new';
+      }
+    } else {
       const baseMean = baseStats.Mean;
       // A row whose ratio cannot mean anything is reported as such rather than rendered as
       // `+Infinity%` and flagged. It is counted with the errored rows because it is the
@@ -263,18 +285,22 @@ function buildComment(prReport, baseReport, options = {}) {
     `measurements' combined standard deviation; ✅ = correspondingly faster.</sub>`,
   ];
 
-  const noise = noiseProfile(deltaPercents);
-  if (noise) {
-    // Stated as what it is: a floor, not a verdict. A reviewer comparing a flagged row
-    // against p95 can see immediately whether it stands out from the run it came in.
+  const spread = spreadProfile(deltaPercents);
+  if (spread) {
+    // Named for what it is rather than for what it usually is. On a narrow change this
+    // reads as the run's noise floor; on a broad one it moves with the change itself, and
+    // a reviewer told "this is drift" would talk themselves out of a real regression.
     // Reported as percentiles, not as a mean and a "median": each figure is the |Δ| of an
     // actual row (see `percentile`), and naming them p50/p90/p95 says so.
     footer.push('');
     footer.push(
-      `<sub>**Measured noise floor for this run** — |Δ| across all ${noise.n} paired rows, of which a pull ` +
-      `request changes only a handful, so the rest is the runner's own drift: p50 ` +
-      `${noise.p50.toFixed(1)}%, p90 ${noise.p90.toFixed(1)}%, p95 ${noise.p95.toFixed(1)}% ` +
-      `(nearest-rank). Read a flag near that p95 with suspicion, and re-run before acting on it.</sub>`
+      `<sub>**Observed spread of this run** — |Δ| across all ${spread.n} paired rows: p50 ` +
+      `${spread.p50.toFixed(1)}%, p90 ${spread.p90.toFixed(1)}%, p95 ${spread.p95.toFixed(1)}% ` +
+      `(nearest-rank). A pull request usually touches a handful of these, so this is mostly the ` +
+      `runner's own drift between the two slices — but a change to a shared primitive moves many ` +
+      `rows at once and would raise these figures itself, so read it as the run's spread rather ` +
+      `than as a floor the PR cannot have caused. A flag that does not stand out against it is ` +
+      `worth re-running before acting on.</sub>`
     );
   }
 
@@ -318,7 +344,7 @@ function buildComment(prReport, baseReport, options = {}) {
     ...footer,
   ].join('\n');
 
-  return { body, entries, regressions, improvements, errored, noise, missingShards, thresholdRatio, noiseSigmas };
+  return { body, entries, regressions, improvements, errored, spread, missingShards, thresholdRatio, noiseSigmas };
 }
 
 // ---- self-test ------------------------------------------------------------------------
@@ -416,13 +442,13 @@ function selfTest() {
   // 117.8) = 1614.7 against a 665.3 gap.
   check('3σ clears PooledCelerityDictionary_Lookup', classifyDelta(5372.3, 525.2, 4707.0, 117.8, 1.10, 3), null);
 
-  // ---- noise profile ----
-  const profile = noiseProfile([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
-  check('noise profile', [profile.n, profile.p50, profile.p90, profile.p95], [10, 5, 9, 10]);
-  check('noise profile of nothing', noiseProfile([]), null);
+  // ---- spread profile ----
+  const profile = spreadProfile([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
+  check('spread profile', [profile.n, profile.p50, profile.p90, profile.p95], [10, 5, 9, 10]);
+  check('spread profile of nothing', spreadProfile([]), null);
   // Nearest-rank, so every reported figure is a value that appears in the sample. Pinned
   // because the footer names the statistics after this definition.
-  check('percentiles are order statistics, not interpolations', noiseProfile([1, 2]).p50, 1);
+  check('percentiles are order statistics, not interpolations', spreadProfile([1, 2]).p50, 1);
 
   // ---- comment composition ----
   const prReport = {
@@ -432,6 +458,7 @@ function selfTest() {
       { FullName: 'ABenchmark.A_Noisy(ItemCount: 8)', Statistics: statsFor(120, 40) },
       { FullName: 'ABenchmark.A_New(ItemCount: 8)', Statistics: statsFor(50, 1) },
       { FullName: 'ABenchmark.A_Errored(ItemCount: 8)', Statistics: null, Measurements: [] },
+      { FullName: 'ABenchmark.A_BaseErrored(ItemCount: 8)', Statistics: statsFor(100, 1) },
       { FullName: 'StringHasherBenchmark.Fnv1a(Length: 8)', Statistics: statsFor(10, 0.1) },
     ],
   };
@@ -441,25 +468,32 @@ function selfTest() {
       { FullName: 'ABenchmark.A_Steady(ItemCount: 8)', Statistics: statsFor(100, 1) },
       { FullName: 'ABenchmark.A_Noisy(ItemCount: 8)', Statistics: statsFor(100, 40) },
       { FullName: 'ABenchmark.A_Errored(ItemCount: 8)', Statistics: statsFor(100, 1) },
+      // Present, but the base side of this pair produced no statistics.
+      { FullName: 'ABenchmark.A_BaseErrored(ItemCount: 8)', Statistics: null },
       { FullName: 'StringHasherBenchmark.Fnv1a(Length: 8)', Statistics: statsFor(10, 0.1) },
     ],
   };
 
   const built = buildComment(prReport, baseReport, { baseSha: 'abcdef1234', shardTotal: '8' });
   check('regressions', built.regressions, 1);
   check('improvements', built.improvements, 0);
-  check('errored', built.errored, 1);
+  check('errored', built.errored, 2);
   check('marker', built.body.startsWith(COMMENT_MARKER), true);
   check('highlights hold only the flagged row', (built.body.match(/A_Slow/g) || []).length, 2);
   check('a noisy row is not flagged', built.body.includes('A_Noisy(ItemCount: 8)` | 120.0 ns | 40.0 ns | 100.0 ns | +20.0% |'), true);
   check('a new row is reported as new', built.body.includes('🆕 new'), true);
   check('an errored row is reported', built.body.includes('⚠️ errored'), true);
+  // An unusable comparison must reach the count, not just the table: a subtitle reading
+  // "No significant change" over a pair that could not be compared is false reassurance.
+  check('a base-side failure is reported', built.body.includes('⚠️ base errored'), true);
+  check('a base-side failure reaches the subtitle', built.body.includes('2 benchmarks produced no usable comparison'), true);
   check('hashers get their own section', built.body.includes('<summary><b>Hashers</b> (1)</summary>'), true);
-  check('collections section excludes hashers', built.body.includes('<summary><b>Collections</b> (5)</summary>'), true);
-  // The errored row has no delta, so it is outside the noise profile; the new row has no
-  // base and is outside it too. Four rows remain.
-  check('noise profile counts only paired rows', built.noise.n, 4);
-  check('noise floor is published', built.body.includes('**Measured noise floor for this run**'), true);
+  check('collections section excludes hashers', built.body.includes('<summary><b>Collections</b> (6)</summary>'), true);
+  // The errored row has no delta, the base-errored row has no base statistics, and the new
+  // row has no base at all. Four rows remain.
+  check('spread profile counts only paired rows', built.spread.n, 4);
+  check('the spread is published', built.body.includes('**Observed spread of this run**'), true);
+  check('the spread is not called a floor', /noise floor/i.test(built.body), false);
   check('no incomplete banner by default', built.body.includes('[!WARNING]'), false);
 
   const partial = buildComment(prReport, baseReport, { missingShards: '3, 5' });
@@ -479,7 +513,7 @@ function selfTest() {
   check('zero sigmas leaves the ratio gate alone', [noGuard.regressions, noGuard.improvements], [2, 0]);
   check('zero sigmas is not replaced by the default', noGuard.noiseSigmas, 0);
 
-  // A degenerate base must not poison the published noise floor.
+  // A degenerate base must not poison the published percentiles.
   const degenerate = buildComment(
     { Benchmarks: [
       { FullName: 'ABenchmark.A_Ok(ItemCount: 8)', Statistics: statsFor(102, 1) },
@@ -495,7 +529,7 @@ function selfTest() {
     ] },
     {},
   );
-  check('a non-finite delta is kept out of the noise profile', [degenerate.noise.n, degenerate.noise.p95.toFixed(1)], [1, '2.0']);
+  check('a non-finite delta is kept out of the spread profile', [degenerate.spread.n, degenerate.spread.p95.toFixed(1)], [1, '2.0']);
   // A zero base makes the ratio Infinity, a zero PR mean makes it 0. Both used to clear a
   // gate; neither may now be reported as a change.
   check('a broken comparison is never flagged', [degenerate.regressions, degenerate.improvements], [0, 0]);
@@ -517,7 +551,7 @@ function selfTest() {
   // An empty base (every shard's base run produced nothing) must still render.
   const noBase = buildComment(prReport, { Benchmarks: [] }, {});
   check('an empty base still renders', noBase.regressions, 0);
-  check('an empty base reports every row as new', (noBase.body.match(/🆕 new/g) || []).length, 5);
+  check('an empty base reports every row as new', (noBase.body.match(/🆕 new/g) || []).length, 6);
 
   // ---- settings ----
   // Unset means "use the calibrated default"; a value that would quietly change what a
@@ -608,14 +642,14 @@ function main() {
   console.log(`Wrote ${outPath}: ${result.entries.length} row(s), ${result.regressions} regression(s), ` +
     `${result.improvements} improvement(s), ${result.errored} errored ` +
     `(±${((result.thresholdRatio - 1) * 100).toFixed(0)}% and ${result.noiseSigmas}σ).`);
-  if (result.noise) {
-    console.log(`Measured noise floor over ${result.noise.n} paired row(s): p50 ` +
-      `${result.noise.p50.toFixed(1)}%, p90 ${result.noise.p90.toFixed(1)}%, p95 ${result.noise.p95.toFixed(1)}%.`);
+  if (result.spread) {
+    console.log(`Observed spread over ${result.spread.n} paired row(s): p50 ` +
+      `${result.spread.p50.toFixed(1)}%, p90 ${result.spread.p90.toFixed(1)}%, p95 ${result.spread.p95.toFixed(1)}%.`);
   }
 }
 
 if (require.main === module) {
   main();
 }
 
-module.exports = { buildComment, classifyDelta, formatNs, isComparable, noiseProfile, readSetting, COMMENT_MARKER };
+module.exports = { buildComment, classifyDelta, formatNs, isComparable, spreadProfile, readSetting, COMMENT_MARKER };