Observation
The PR benchmark comment's flag rule — a row is flagged when it moves past ±10% and past the combined standard deviation of the two measurements — produced 13 flagged rows (8 regressions, 5 improvements) on a diff whose library IL is byte-identical to main.
Measured on #350 (run 31139942655). That PR changes only .github/workflows/, scripts/, documentation, and src/Celerity.Benchmarks/Program.cs — and the Program.cs change touches shard selection only, never a measured code path. Nothing in Celerity, Celerity.Hashing, Celerity.Primitives or Celerity.Sorting was touched at all.
So every one of these deltas is measurement noise:
| Benchmark |
This PR |
StdDev |
main |
Δ |
SmallDictionaryBenchmark.Dictionary_Remove(ItemCount: 64) |
6.47 μs |
219.1 ns |
4.86 μs |
+33.1% ⚠️ |
TrieBenchmark.Trie_PrefixMatch(ItemCount: 100000) |
8.33 ms |
1.17 ms |
6.12 ms |
+36.2% ⚠️ |
CompressedIntSetBenchmark.HashSet_Union(ItemCount: 1000) |
34.16 μs |
4.39 μs |
27.52 μs |
+24.1% ⚠️ |
CelerityMultiMapBenchmark.Dictionary_Remove(ItemCount: 1000) |
20.91 μs |
1.44 μs |
32.88 μs |
-36.4% ✅ |
SmallDictionaryBenchmark.SmallDictionary_Remove(ItemCount: 8) |
1.11 μs |
112.8 ns |
1.52 μs |
-26.7% ✅ |
SwissDictionaryBenchmark.Dictionary_Lookup(ItemCount: 1000) |
4.73 μs |
118.0 ns |
6.18 μs |
-23.5% ✅ |
Four of these are BCL baseline arms — Dictionary_Remove, Dictionary_Lookup, HashSet_Union. The .NET Dictionary<,> implementation is identical on both sides by definition, so a flagged ±33% on it is unambiguously the harness, not the code.
Why the std-dev guard does not catch it
ALERT_THRESHOLD_RATIO is gated on Math.abs(prMean - baseMean) > (prStdDev + baseStdDev). That compares a between-run difference against within-run dispersion. BenchmarkDotNet's StandardDeviation describes the spread of iterations inside one measurement — on a warm, settled process that is genuinely small — while the thing that actually moves between the head slice and the base slice is the process/runner level: JIT and GC state, page cache, and the noisy-neighbour effects benchmarks.yml's own header calls out as 20–50%.
So the guard is systematically too permissive: a row with a tight intra-run stddev and a shifted mean clears it every time. SmallDictionaryBenchmark.Dictionary_Remove above has a 219 ns stddev against a 1.61 μs shift.
Why it matters
The comment is the per-PR regression signal, and Guiding Principle #4 makes the published numbers the project's contract. At ~13 flagged rows per zero-change PR, the default reading of a flag has to be "probably noise", which is the same as having no signal — and it makes a real regression harder to see, not easier, because it arrives in a list of a dozen false ones.
This is the third concern in #335 ("it may quietly corrupt the measurements") showing up on the reporting side rather than the scheduling side. #350 fixes the scheduling half — runs no longer overlap or accumulate, so contention is much reduced — but this run had the workflow essentially to itself and still produced 13 flags, so reduced contention alone does not close it.
Approaches to consider
Not proposing one over the others:
- Raise the ratio threshold to something like ±25%, matching the documented hosted-runner variance. Cheapest; blunt, and hides real regressions smaller than that.
- Replace the std-dev guard with a between-run estimate. BenchmarkDotNet emits per-measurement data in the full JSON; a comparison against the spread of launches rather than iterations would be measuring the right quantity.
Job.Default already uses 2 launches, so the data is present.
- Require the flag to reproduce. Flag only when a row moves past threshold on two consecutive runs, or re-measure only the flagged rows a second time and drop the ones that do not repeat. Costs a short second pass over a handful of cases rather than the suite.
- Report a confidence interval instead of a point delta, and flag only when the intervals are disjoint.
- Quantify the floor first. Before tuning anything, run the workflow on a no-op commit two or three times and record how many rows flag; that gives the actual false-positive rate to design against. This run is one such sample already (13).
(5) is worth doing regardless of which of (1)–(4) is chosen — right now the threshold is set against an assumed noise level rather than a measured one.
Observation
The PR benchmark comment's flag rule — a row is flagged when it moves past ±10% and past the combined standard deviation of the two measurements — produced 13 flagged rows (8 regressions, 5 improvements) on a diff whose library IL is byte-identical to
main.Measured on #350 (run 31139942655). That PR changes only
.github/workflows/,scripts/, documentation, andsrc/Celerity.Benchmarks/Program.cs— and theProgram.cschange touches shard selection only, never a measured code path. Nothing inCelerity,Celerity.Hashing,Celerity.PrimitivesorCelerity.Sortingwas touched at all.So every one of these deltas is measurement noise:
SmallDictionaryBenchmark.Dictionary_Remove(ItemCount: 64)TrieBenchmark.Trie_PrefixMatch(ItemCount: 100000)CompressedIntSetBenchmark.HashSet_Union(ItemCount: 1000)CelerityMultiMapBenchmark.Dictionary_Remove(ItemCount: 1000)SmallDictionaryBenchmark.SmallDictionary_Remove(ItemCount: 8)SwissDictionaryBenchmark.Dictionary_Lookup(ItemCount: 1000)Four of these are BCL baseline arms —
Dictionary_Remove,Dictionary_Lookup,HashSet_Union. The .NETDictionary<,>implementation is identical on both sides by definition, so a flagged ±33% on it is unambiguously the harness, not the code.Why the std-dev guard does not catch it
ALERT_THRESHOLD_RATIOis gated onMath.abs(prMean - baseMean) > (prStdDev + baseStdDev). That compares a between-run difference against within-run dispersion. BenchmarkDotNet'sStandardDeviationdescribes the spread of iterations inside one measurement — on a warm, settled process that is genuinely small — while the thing that actually moves between the head slice and the base slice is the process/runner level: JIT and GC state, page cache, and the noisy-neighbour effectsbenchmarks.yml's own header calls out as 20–50%.So the guard is systematically too permissive: a row with a tight intra-run stddev and a shifted mean clears it every time.
SmallDictionaryBenchmark.Dictionary_Removeabove has a 219 ns stddev against a 1.61 μs shift.Why it matters
The comment is the per-PR regression signal, and Guiding Principle #4 makes the published numbers the project's contract. At ~13 flagged rows per zero-change PR, the default reading of a flag has to be "probably noise", which is the same as having no signal — and it makes a real regression harder to see, not easier, because it arrives in a list of a dozen false ones.
This is the third concern in #335 ("it may quietly corrupt the measurements") showing up on the reporting side rather than the scheduling side. #350 fixes the scheduling half — runs no longer overlap or accumulate, so contention is much reduced — but this run had the workflow essentially to itself and still produced 13 flags, so reduced contention alone does not close it.
Approaches to consider
Not proposing one over the others:
Job.Defaultalready uses 2 launches, so the data is present.(5) is worth doing regardless of which of (1)–(4) is chosen — right now the threshold is set against an assumed noise level rather than a measured one.