From 8915c3969209465336a463b718e6d740b5686d65 Mon Sep 17 00:00:00 2001 From: Seiya KONNO Date: Sun, 6 Sep 2026 16:40:45 +0900 Subject: [PATCH] fix(bench): exclude sub-timer-resolution cases from the comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #201's benchmark comment reported implausible improvements (GameplayRecorder +2733%, createBeatResolver +1500%, 205 more cases at +90% or higher) for a PR that only changes how scripts are invoked, not any benchmarked function. Root cause (issue #202): several exports-benchmark cases (closure- allocating ones especially) run in tens to a few hundred nanoseconds per call under CI's timing budget (--time 50 --warmup-time 25) — below what tinybench's timer can resolve (p50Ms == 0). A percent computed from two independent runs of code this cheap is measurement noise: the same unmodified code, benchmarked twice back to back, can swing 50%+ with zero changes (verified: chart.eventToBeat went 60.5M -> 24.4M ops/s across two back-to-back runs of the identical build). CI's base/head split (separate worktrees/processes) is exactly the setup where that noise reads as a "regression" or "improvement". Added a reliability floor: compareSnapshots now excludes any case whose base or head median per-call latency (p50Ms) is at or below 0.001ms from the regression/improvement/unchanged classification entirely (not folded into "unchanged" -- a percent computed from noise isn't a real "no change" either). Excluded cases get their own count (ComparisonSummary.unreliableCaseCount) and a labeled section in the markdown report so they stay visible instead of silently vanishing. countUnreliableCases is exported separately from compareSnapshots so existing callers of compareSnapshots (and its existing tests) keep returning ComparedRow[] unchanged. --- scripts/bench/compare-results.test.ts | 37 +++++++++++ scripts/bench/compare-results.ts | 90 ++++++++++++++++++++++++++- 2 files changed, 124 insertions(+), 3 deletions(-) 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(