Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ 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

Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
110 changes: 72 additions & 38 deletions scripts/benchmark_comment.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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>`
);
}

Expand Down Expand Up @@ -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 ------------------------------------------------------------------------
Expand Down Expand Up @@ -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 = {
Expand All @@ -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) },
],
};
Expand All @@ -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' });
Expand All @@ -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) },
Expand All @@ -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]);
Expand All @@ -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
Expand Down Expand Up @@ -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 };
Loading