Skip to content

Commit bf5d1eb

Browse files
refactor(ci): name the spread profile after what it measures
Review follow-up: the footer stopped calling the published percentiles a noise floor, but the code still said noiseProfile/noise, which is how the wording gets reintroduced on the next edit. Renamed to spreadProfile/spread, including the buildComment result key. noiseSigmas and ALERT_NOISE_SIGMAS keep their names deliberately, and the comment now says why: that guard scales the two measurements' own standard deviations, which really are per-row measurement noise - a different quantity from the run-wide distribution, and one the word describes correctly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent fbca824 commit bf5d1eb

1 file changed

Lines changed: 29 additions & 23 deletions

File tree

scripts/benchmark_comment.js

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -127,13 +127,19 @@ function percentile(sorted, p) {
127127
// request — a handful of the ~750 tracked cases touched — this is a noise floor: almost
128128
// every row in it is the runner's own drift between the head slice and the base slice.
129129
//
130-
// It is not *only* that, which is why nothing here calls it a noise floor. A change to a
131-
// shared primitive or to the benchmark configuration moves many rows at once, and those
132-
// real effects sit in the same sample. Deriving the floor from a control group instead was
133-
// considered and rejected: the obvious controls are the BCL baseline arms, but a layout
134-
// shift moves those too, so they would understate the tail while looking authoritative.
135-
// Reporting the distribution and saying what is in it is the honest version.
136-
function noiseProfile(deltaPercents) {
130+
// It is not *only* that, which is why neither this function nor the footer it feeds calls
131+
// it a noise floor. A change to a shared primitive or to the benchmark configuration moves
132+
// many rows at once, and those real effects sit in the same sample. Deriving the floor from
133+
// a control group instead was considered and rejected: the obvious controls are the BCL
134+
// baseline arms, but a layout shift moves those too, so they would understate the tail
135+
// while looking authoritative. Reporting the distribution and saying what is in it is the
136+
// honest version.
137+
//
138+
// `noiseSigmas` and `ALERT_NOISE_SIGMAS` keep their names on purpose. That guard is scaled
139+
// by the two measurements' own standard deviations, which really are per-row measurement
140+
// noise — a different quantity from the run-wide distribution measured here, and one the
141+
// word describes correctly.
142+
function spreadProfile(deltaPercents) {
137143
if (deltaPercents.length === 0) return null;
138144
const sorted = [...deltaPercents].sort((a, b) => a - b);
139145
return {
@@ -279,17 +285,17 @@ function buildComment(prReport, baseReport, options = {}) {
279285
`measurements' combined standard deviation; ✅ = correspondingly faster.</sub>`,
280286
];
281287

282-
const noise = noiseProfile(deltaPercents);
283-
if (noise) {
288+
const spread = spreadProfile(deltaPercents);
289+
if (spread) {
284290
// Named for what it is rather than for what it usually is. On a narrow change this
285291
// reads as the run's noise floor; on a broad one it moves with the change itself, and
286292
// a reviewer told "this is drift" would talk themselves out of a real regression.
287293
// Reported as percentiles, not as a mean and a "median": each figure is the |Δ| of an
288294
// actual row (see `percentile`), and naming them p50/p90/p95 says so.
289295
footer.push('');
290296
footer.push(
291-
`<sub>**Observed spread of this run** — |Δ| across all ${noise.n} paired rows: p50 ` +
292-
`${noise.p50.toFixed(1)}%, p90 ${noise.p90.toFixed(1)}%, p95 ${noise.p95.toFixed(1)}% ` +
297+
`<sub>**Observed spread of this run** — |Δ| across all ${spread.n} paired rows: p50 ` +
298+
`${spread.p50.toFixed(1)}%, p90 ${spread.p90.toFixed(1)}%, p95 ${spread.p95.toFixed(1)}% ` +
293299
`(nearest-rank). A pull request usually touches a handful of these, so this is mostly the ` +
294300
`runner's own drift between the two slices — but a change to a shared primitive moves many ` +
295301
`rows at once and would raise these figures itself, so read it as the run's spread rather ` +
@@ -338,7 +344,7 @@ function buildComment(prReport, baseReport, options = {}) {
338344
...footer,
339345
].join('\n');
340346

341-
return { body, entries, regressions, improvements, errored, noise, missingShards, thresholdRatio, noiseSigmas };
347+
return { body, entries, regressions, improvements, errored, spread, missingShards, thresholdRatio, noiseSigmas };
342348
}
343349

344350
// ---- self-test ------------------------------------------------------------------------
@@ -436,13 +442,13 @@ function selfTest() {
436442
// 117.8) = 1614.7 against a 665.3 gap.
437443
check('3σ clears PooledCelerityDictionary_Lookup', classifyDelta(5372.3, 525.2, 4707.0, 117.8, 1.10, 3), null);
438444

439-
// ---- noise profile ----
440-
const profile = noiseProfile([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
441-
check('noise profile', [profile.n, profile.p50, profile.p90, profile.p95], [10, 5, 9, 10]);
442-
check('noise profile of nothing', noiseProfile([]), null);
445+
// ---- spread profile ----
446+
const profile = spreadProfile([1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
447+
check('spread profile', [profile.n, profile.p50, profile.p90, profile.p95], [10, 5, 9, 10]);
448+
check('spread profile of nothing', spreadProfile([]), null);
443449
// Nearest-rank, so every reported figure is a value that appears in the sample. Pinned
444450
// because the footer names the statistics after this definition.
445-
check('percentiles are order statistics, not interpolations', noiseProfile([1, 2]).p50, 1);
451+
check('percentiles are order statistics, not interpolations', spreadProfile([1, 2]).p50, 1);
446452

447453
// ---- comment composition ----
448454
const prReport = {
@@ -485,7 +491,7 @@ function selfTest() {
485491
check('collections section excludes hashers', built.body.includes('<summary><b>Collections</b> (6)</summary>'), true);
486492
// The errored row has no delta, the base-errored row has no base statistics, and the new
487493
// row has no base at all. Four rows remain.
488-
check('noise profile counts only paired rows', built.noise.n, 4);
494+
check('spread profile counts only paired rows', built.spread.n, 4);
489495
check('the spread is published', built.body.includes('**Observed spread of this run**'), true);
490496
check('the spread is not called a floor', /noise floor/i.test(built.body), false);
491497
check('no incomplete banner by default', built.body.includes('[!WARNING]'), false);
@@ -523,7 +529,7 @@ function selfTest() {
523529
] },
524530
{},
525531
);
526-
check('a non-finite delta is kept out of the noise profile', [degenerate.noise.n, degenerate.noise.p95.toFixed(1)], [1, '2.0']);
532+
check('a non-finite delta is kept out of the spread profile', [degenerate.spread.n, degenerate.spread.p95.toFixed(1)], [1, '2.0']);
527533
// A zero base makes the ratio Infinity, a zero PR mean makes it 0. Both used to clear a
528534
// gate; neither may now be reported as a change.
529535
check('a broken comparison is never flagged', [degenerate.regressions, degenerate.improvements], [0, 0]);
@@ -636,14 +642,14 @@ function main() {
636642
console.log(`Wrote ${outPath}: ${result.entries.length} row(s), ${result.regressions} regression(s), ` +
637643
`${result.improvements} improvement(s), ${result.errored} errored ` +
638644
`(±${((result.thresholdRatio - 1) * 100).toFixed(0)}% and ${result.noiseSigmas}σ).`);
639-
if (result.noise) {
640-
console.log(`Observed spread over ${result.noise.n} paired row(s): p50 ` +
641-
`${result.noise.p50.toFixed(1)}%, p90 ${result.noise.p90.toFixed(1)}%, p95 ${result.noise.p95.toFixed(1)}%.`);
645+
if (result.spread) {
646+
console.log(`Observed spread over ${result.spread.n} paired row(s): p50 ` +
647+
`${result.spread.p50.toFixed(1)}%, p90 ${result.spread.p90.toFixed(1)}%, p95 ${result.spread.p95.toFixed(1)}%.`);
642648
}
643649
}
644650

645651
if (require.main === module) {
646652
main();
647653
}
648654

649-
module.exports = { buildComment, classifyDelta, formatNs, isComparable, noiseProfile, readSetting, COMMENT_MARKER };
655+
module.exports = { buildComment, classifyDelta, formatNs, isComparable, spreadProfile, readSetting, COMMENT_MARKER };

0 commit comments

Comments
 (0)