diff --git a/scripts/bench/compare-results.test.ts b/scripts/bench/compare-results.test.ts index 8edf93f3..f57d14c0 100644 --- a/scripts/bench/compare-results.test.ts +++ b/scripts/bench/compare-results.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { buildDiffMarkdown, compareSnapshots, + countUnreliableCases, resolveOverallVerdict, summarizeComparison, summarizeRows, @@ -126,6 +127,42 @@ describe('compareSnapshots', () => { expect(rows[0]?.headHz).toBe(110); expect(rows[0]?.deltaPercent).toBeCloseTo(10); }); + + it('excludes a case whose head p50 latency is at/below the reliability floor, however large its hz delta', () => { + // A case this cheap (p50 <= 0.001ms) is timer-resolution noise, not a real 10,000% speedup — issue #202. + const rows = compareSnapshots( + createSnapshot({ 'chart.createBeatResolver': 1_000_000 }), + createSnapshot({ 'chart.createBeatResolver': 100_000_000 }), + ); + + expect(rows).toHaveLength(0); + }); + + it('excludes a case whose base p50 latency is at/below the reliability floor', () => { + const rows = compareSnapshots( + createSnapshot({ 'chart.createBeatResolver': 100_000_000 }), + createSnapshot({ 'chart.createBeatResolver': 1_000_000 }), + ); + + expect(rows).toHaveLength(0); + }); + + it('keeps a case whose p50 latency sits just above the reliability floor', () => { + const rows = compareSnapshots(createSnapshot({ 'utils.stable': 1000 }), createSnapshot({ 'utils.stable': 1000 })); + + expect(rows).toHaveLength(1); + }); +}); + +describe('countUnreliableCases', () => { + it('counts only comparable keys excluded for sub-timer-resolution latency', () => { + const count = countUnreliableCases( + createSnapshot({ 'chart.createBeatResolver': 1_000_000, 'utils.stable': 1000, 'utils.baseOnly': 500 }), + createSnapshot({ 'chart.createBeatResolver': 100_000_000, 'utils.stable': 1000, 'utils.headOnly': 500 }), + ); + + expect(count).toBe(1); + }); }); describe('summarizeRows', () => { diff --git a/scripts/bench/compare-results.ts b/scripts/bench/compare-results.ts index 00afa627..6b800f4b 100644 --- a/scripts/bench/compare-results.ts +++ b/scripts/bench/compare-results.ts @@ -9,7 +9,7 @@ import { resolveCliValue, runCliMain, } from './cli-utils.ts'; -import type { ExportsBenchmarkSnapshot } from './exports.types.ts'; +import type { BenchmarkTaskStats, ExportsBenchmarkSnapshot } from './exports.types.ts'; import { resolveComparisonHz } from './task-stats.ts'; interface CliDefaults { @@ -46,12 +46,24 @@ export interface ComparisonSummary { meanDeltaPercent: number; thresholdPercent: number; overallVerdict: BenchmarkOverallVerdict; + /** Cases whose base or head p50 latency was at/below MIN_RELIABLE_LATENCY_MS — excluded, not just unchanged. */ + unreliableCaseCount: number; } const scriptDir = dirname(fileURLToPath(import.meta.url)); const repositoryDir = resolve(scriptDir, '../..'); const COMMENT_MARKER = ''; +/** + * Below this per-call latency (median, in ms), a case's timing is dominated by measurement noise rather than the + * benchmarked function's own cost — a single call is cheap enough that the system timer can't resolve it reliably. + * Percent deltas computed from two noise floors can legitimately swing by 10-30x between independent runs of + * *identical* code (see issue #202: verified the same ~15-250ns closure-allocation case reproduces this under both + * tsx and node, with no change to the benchmarked code). Such cases are excluded from the regression/improvement + * lists rather than folded into "unchanged", since a percent computed from noise isn't a real "no change" either. + */ +const MIN_RELIABLE_LATENCY_MS = 0.001; + const DEFAULTS: CliDefaults = { outputPath: resolve(repositoryDir, 'tmp/bench/exports-pr-comment.md'), thresholdPercent: 5, @@ -83,6 +95,7 @@ async function main(): Promise { meanDeltaPercent: 0, thresholdPercent: options.thresholdPercent, overallVerdict: 'unchanged', + unreliableCaseCount: 0, }; if (options.summaryPath) { @@ -111,7 +124,7 @@ export function buildDiffMarkdown( topCount: number, ): string { const rows = compareSnapshots(baseSnapshot, headSnapshot); - const summary = summarizeRows(rows, thresholdPercent); + const summary = { ...summarizeRows(rows, thresholdPercent), unreliableCaseCount: countUnreliableCases(baseSnapshot, headSnapshot) }; const regressions = rows .filter((row) => row.deltaPercent <= -thresholdPercent) @@ -145,6 +158,7 @@ export function buildDiffMarkdown( lines.push(`| Cases improved (>= threshold) | ${summary.improvedCount} |`); lines.push(`| Cases regressed (<= -threshold) | ${summary.regressedCount} |`); lines.push(`| Cases unchanged | ${summary.unchangedCount} |`); + lines.push(`| Cases excluded (sub-timer-resolution) | ${summary.unreliableCaseCount} |`); lines.push(`| Head benchmarked cases | ${headSnapshot.totals.benchmarked} |`); lines.push(`| Head skipped cases | ${headSnapshot.totals.skipped} |`); @@ -176,6 +190,22 @@ export function buildDiffMarkdown( } } + const unreliableKeys = resolveUnreliableCaseKeys(baseSnapshot, headSnapshot).slice(0, topCount); + if (unreliableKeys.length > 0) { + lines.push(''); + lines.push('### Excluded (sub-timer-resolution)'); + lines.push( + `Per-call latency at or below ${MIN_RELIABLE_LATENCY_MS}ms on at least one side — the reported time is measurement noise, not the case's real cost, so no percent change is shown.`, + ); + lines.push('| API | Base median ops/s | Head median ops/s |'); + lines.push('| --- | ---: | ---: |'); + for (const key of unreliableKeys) { + const baseResult = baseSnapshot.results[key]; + const headResult = headSnapshot.results[key]; + lines.push(`| \`${key}\` | ${formatOps(baseResult?.medianHz ?? 0)} | ${formatOps(headResult?.medianHz ?? 0)} |`); + } + } + const newlySkipped = resolveNewlySkippedKeys(baseSnapshot, headSnapshot).slice(0, topCount); if (newlySkipped.length > 0) { lines.push(''); @@ -224,6 +254,20 @@ function buildHeadOnlyMarkdown(headSnapshot: ExportsBenchmarkSnapshot, topCount: return lines.join('\n'); } +/** + * True when either side's per-call latency is at/below MIN_RELIABLE_LATENCY_MS — the case's timing is noise, not + * signal, so it must not be reported as a regression, an improvement, or "unchanged" (all three imply the percent + * means something). + */ +function isUnreliableCase( + baseResult: Pick, + headResult: Pick, +): boolean { + return ( + !(baseResult.p50Ms > MIN_RELIABLE_LATENCY_MS) || !(headResult.p50Ms > MIN_RELIABLE_LATENCY_MS) + ); +} + export function compareSnapshots( baseSnapshot: ExportsBenchmarkSnapshot, headSnapshot: ExportsBenchmarkSnapshot, @@ -234,6 +278,9 @@ export function compareSnapshots( if (!baseResult) { continue; } + if (isUnreliableCase(baseResult, headResult)) { + continue; + } const baseHz = resolveComparisonHz(baseResult); const headHz = resolveComparisonHz(headResult); if (baseHz <= 0 || headHz <= 0) { @@ -251,6 +298,24 @@ export function compareSnapshots( return rows; } +/** Count of comparable keys (present on both sides) excluded from `compareSnapshots` as sub-timer-resolution noise. */ +export function countUnreliableCases( + baseSnapshot: ExportsBenchmarkSnapshot, + headSnapshot: ExportsBenchmarkSnapshot, +): number { + let count = 0; + for (const [key, headResult] of Object.entries(headSnapshot.results)) { + const baseResult = baseSnapshot.results[key]; + if (!baseResult) { + continue; + } + if (isUnreliableCase(baseResult, headResult)) { + count += 1; + } + } + return count; +} + export function resolveOverallVerdict(medianDeltaPercent: number, thresholdPercent: number): BenchmarkOverallVerdict { if (medianDeltaPercent >= thresholdPercent) { return 'improved'; @@ -278,6 +343,7 @@ export function summarizeRows(rows: ComparedRow[], thresholdPercent: number): Co meanDeltaPercent, thresholdPercent, overallVerdict: resolveOverallVerdict(medianDeltaPercent, thresholdPercent), + unreliableCaseCount: 0, }; } @@ -287,7 +353,25 @@ export function summarizeComparison( thresholdPercent: number, ): ComparisonSummary { const rows = compareSnapshots(baseSnapshot, headSnapshot); - return summarizeRows(rows, thresholdPercent); + return { + ...summarizeRows(rows, thresholdPercent), + unreliableCaseCount: countUnreliableCases(baseSnapshot, headSnapshot), + }; +} + +function resolveUnreliableCaseKeys( + baseSnapshot: ExportsBenchmarkSnapshot, + headSnapshot: ExportsBenchmarkSnapshot, +): string[] { + const keys: string[] = []; + for (const [key, headResult] of Object.entries(headSnapshot.results)) { + const baseResult = baseSnapshot.results[key]; + if (baseResult && isUnreliableCase(baseResult, headResult)) { + keys.push(key); + } + } + keys.sort((left, right) => left.localeCompare(right)); + return keys; } function resolveNewlySkippedKeys(