From 6fa20f467639ac3d195e0feedf8324d784905682 Mon Sep 17 00:00:00 2001 From: tadelesh Date: Tue, 14 Jul 2026 14:46:56 +0800 Subject: [PATCH] fix(benchmark): compare PR-comment baseline at the flat-label level The PR comment reconstructed the baseline RuntimeStats from the flat history metrics via expandRuntimeMetrics, which split each emit/ label on every slash and mis-parsed scoped packages: emit/@azure-tools/typespec-client-generator-core became a fake @azure-tools emitter with the package name as a step. This produced phantom @azure-tools / @typespec emitter rows in the benchmark PR comments and a wrong baseline for the real emitters. Compare current vs baseline directly at the flat-label level instead: labels are opaque keys (compareFlatMetrics), the current runtime is flattened with flattenRuntime, and no reconstruction/parsing of emitter names happens. Removes expandRuntimeMetrics entirely. The history/latest.json storage format is unchanged, so existing benchmark-data is unaffected; only the compare CLI keeps its structural path (which never had this bug). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/benchmark/src/compare.ts | 19 +++ packages/benchmark/src/generate-history.ts | 2 +- packages/benchmark/src/upload-pr-comment.ts | 153 +++++++------------ packages/benchmark/test/compare-flat.test.ts | 74 +++++++++ 4 files changed, 153 insertions(+), 95 deletions(-) create mode 100644 packages/benchmark/test/compare-flat.test.ts diff --git a/packages/benchmark/src/compare.ts b/packages/benchmark/src/compare.ts index bb95e2ed62..a873f97cad 100644 --- a/packages/benchmark/src/compare.ts +++ b/packages/benchmark/src/compare.ts @@ -22,6 +22,25 @@ export function isNotableMetricChange( return Math.abs(metric.percentChange) >= threshold && Math.abs(metric.change) >= minChangeMs; } +/** + * Compare two flat metric maps (label → ms). Labels are treated as opaque keys, + * so scoped names like `emit/@azure-tools/pkg` are compared as-is without being + * parsed into emitter/step segments. Ordering follows `current` (the logical + * order produced by flattenRuntime), with baseline-only labels appended. + */ +export function compareFlatMetrics( + baseline: Record, + current: Record, +): MetricComparison[] { + const labels = [...Object.keys(current)]; + for (const label of Object.keys(baseline)) { + if (!(label in current)) { + labels.push(label); + } + } + return labels.map((label) => createMetric(label, baseline[label] ?? 0, current[label] ?? 0)); +} + function extractRuntimeMetrics( baselineRuntime: RuntimeStats, currentRuntime: RuntimeStats, diff --git a/packages/benchmark/src/generate-history.ts b/packages/benchmark/src/generate-history.ts index 8d3867cfae..3605ec570d 100644 --- a/packages/benchmark/src/generate-history.ts +++ b/packages/benchmark/src/generate-history.ts @@ -25,7 +25,7 @@ export interface HistoryData { } /** Flatten RuntimeStats into a flat record of label → ms. */ -function flattenRuntime(rt: RuntimeStats): Record { +export function flattenRuntime(rt: RuntimeStats): Record { const flat: Record = {}; flat["total"] = rt.total ?? 0; flat["loader"] = rt.loader ?? 0; diff --git a/packages/benchmark/src/upload-pr-comment.ts b/packages/benchmark/src/upload-pr-comment.ts index 298ade6a4f..5095551b8c 100644 --- a/packages/benchmark/src/upload-pr-comment.ts +++ b/packages/benchmark/src/upload-pr-comment.ts @@ -3,14 +3,14 @@ import { execSync } from "node:child_process"; import { appendFileSync, existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { join, resolve } from "node:path"; import { aggregateDurations } from "./aggregate.js"; -import { compareBenchmarks, hasNotableChanges } from "./compare.js"; +import { compareFlatMetrics, hasNotableChanges } from "./compare.js"; import { formatComparisonSummary, formatConsoleSummary, formatPrComment, } from "./format-comment.js"; -import type { HistoryData } from "./generate-history.js"; -import type { BenchmarkResult, RuntimeStats, SpecBenchmarkResult } from "./types.js"; +import { flattenRuntime, type HistoryData } from "./generate-history.js"; +import type { BenchmarkResult, ComparisonResult } from "./types.js"; import { DEFAULT_BRANCH } from "./utils.js"; export interface UploadPrCommentOptions { @@ -34,58 +34,19 @@ export interface UploadPrCommentOptions { commentFile?: string; } -interface BaselineResult { - baseline: BenchmarkResult; +/** Baseline metrics per spec (spec name → flat label → ms) plus provenance. */ +type FlatMetrics = Record; +interface FlatBaseline { + specs: Record; + commit: string; label: string; } -function expandRuntimeMetrics(flat: Record): RuntimeStats { - const runtime: RuntimeStats = { - total: flat["total"] ?? 0, - loader: flat["loader"] ?? 0, - resolver: flat["resolver"] ?? 0, - checker: flat["checker"] ?? 0, - validation: { total: flat["validation"] ?? 0, validators: {} }, - linter: { total: flat["linter"] ?? 0, rules: {} }, - emit: { total: flat["emit"] ?? 0, emitters: {} }, - }; - - for (const [label, value] of Object.entries(flat)) { - if (label.startsWith("validation/")) { - runtime.validation.validators[label.replace("validation/", "")] = value; - continue; - } - if (label.startsWith("linter/")) { - runtime.linter.rules[label.replace("linter/", "")] = value; - continue; - } - if (!label.startsWith("emit/")) { - continue; - } - - const parts = label.split("/"); - if (parts.length < 2) { - continue; - } - - const emitterName = parts[1]; - runtime.emit.emitters[emitterName] ??= { total: 0, steps: {} }; - if (parts.length === 2) { - runtime.emit.emitters[emitterName].total = value; - } else if (parts.length > 2) { - const stepName = parts.slice(2).join("/"); - runtime.emit.emitters[emitterName].steps[stepName] = value; - } - } - - return runtime; -} - +/** Aggregate a spec's flat metrics across history entries (label → ms). */ function aggregateSpecFromHistory( specName: string, entries: HistoryData["entries"], - currentSpec: SpecBenchmarkResult, -): SpecBenchmarkResult | undefined { +): FlatMetrics | undefined { const samplesByMetric = new Map(); for (const entry of entries) { const metrics = entry.specMetrics[specName]; @@ -104,38 +65,30 @@ function aggregateSpecFromHistory( return undefined; } - const aggregated: Record = {}; + const aggregated: FlatMetrics = {}; for (const [label, samples] of samplesByMetric) { aggregated[label] = aggregateDurations(samples); } - - return { - ...currentSpec, - stats: { - ...currentSpec.stats, - runtime: expandRuntimeMetrics(aggregated), - }, - }; + return aggregated; } function buildRollingBaseline( history: HistoryData, current: BenchmarkResult, baselineWindow: number, -): BaselineResult | undefined { +): FlatBaseline | undefined { const window = Math.max(1, baselineWindow); const entries = history.entries.slice(-window); if (entries.length === 0) { return undefined; } - const specs: Record = {}; - for (const [specName, currentSpec] of Object.entries(current.specs)) { - const rollingSpec = aggregateSpecFromHistory(specName, entries, currentSpec); - if (!rollingSpec) { - continue; + const specs: Record = {}; + for (const specName of Object.keys(current.specs)) { + const aggregated = aggregateSpecFromHistory(specName, entries); + if (aggregated) { + specs[specName] = aggregated; } - specs[specName] = rollingSpec; } if (Object.keys(specs).length === 0) { @@ -145,12 +98,8 @@ function buildRollingBaseline( const firstCommit = entries[0]?.commit.slice(0, 7) ?? "unknown"; const lastCommit = entries[entries.length - 1]?.commit.slice(0, 7) ?? "unknown"; return { - baseline: { - ...current, - commit: `rolling-baseline-${firstCommit}-${lastCommit}`, - timestamp: new Date().toISOString(), - specs, - }, + specs, + commit: `rolling-baseline-${firstCommit}-${lastCommit}`, label: `rolling baseline (${entries.length} main run${entries.length > 1 ? "s" : ""})`, }; } @@ -160,7 +109,7 @@ function fetchBaseline( resultsDir: string, current: BenchmarkResult, baselineWindow: number, -): BaselineResult | undefined { +): FlatBaseline | undefined { try { const hasRemote = (() => { try { @@ -194,10 +143,12 @@ function fetchBaseline( encoding: "utf-8", maxBuffer: 50_000_000, }); - return { - baseline: JSON.parse(latestContent) as BenchmarkResult, - label: "latest main benchmark", - }; + const latest = JSON.parse(latestContent) as BenchmarkResult; + const specs: Record = {}; + for (const [specName, spec] of Object.entries(latest.specs)) { + specs[specName] = flattenRuntime(spec.stats.runtime); + } + return { specs, commit: latest.commit, label: "latest main benchmark" }; } catch { return undefined; } @@ -227,31 +178,45 @@ export function uploadPrComment(options: UploadPrCommentOptions): void { } const current = JSON.parse(readFileSync(resolve(resultsFile), "utf-8")) as BenchmarkResult; - const baselineResult = fetchBaseline(branch, resultsDir, current, baselineWindow); + const baseline = fetchBaseline(branch, resultsDir, current, baselineWindow); mkdirSync(outputDir, { recursive: true }); let commentMarkdown: string; let githubSummary: string | undefined; - if (baselineResult) { - const { baseline, label } = baselineResult; - const comparisons = compareBenchmarks(baseline, current, { threshold }); - commentMarkdown = formatPrComment( - comparisons, - `${baseline.commit} (${label})`, - current.commit, - { - threshold, - title, - }, - ); - githubSummary = formatComparisonSummary( - comparisons, - `${baseline.commit} (${label})`, - current.commit, + if (baseline) { + // Compare current vs baseline at the flat-label level: labels are opaque + // keys, so scoped emitter/rule names are compared as-is (no parsing). + const comparisons: ComparisonResult[] = []; + for (const specName of Object.keys(current.specs).sort()) { + const baselineFlat = baseline.specs[specName]; + if (!baselineFlat) { + continue; + } + const currentSpec = current.specs[specName]; + comparisons.push({ + specName, + metrics: compareFlatMetrics(baselineFlat, flattenRuntime(currentSpec.stats.runtime)), + complexity: { + createdTypes: { + baseline: currentSpec.stats.complexity.createdTypes, + current: currentSpec.stats.complexity.createdTypes, + }, + finishedTypes: { + baseline: currentSpec.stats.complexity.finishedTypes, + current: currentSpec.stats.complexity.finishedTypes, + }, + }, + }); + } + + const baselineLabel = `${baseline.commit} (${baseline.label})`; + commentMarkdown = formatPrComment(comparisons, baselineLabel, current.commit, { threshold, - ); + title, + }); + githubSummary = formatComparisonSummary(comparisons, baselineLabel, current.commit, threshold); // Also print console summary console.log(formatConsoleSummary(comparisons, threshold)); diff --git a/packages/benchmark/test/compare-flat.test.ts b/packages/benchmark/test/compare-flat.test.ts new file mode 100644 index 0000000000..e29e0698ac --- /dev/null +++ b/packages/benchmark/test/compare-flat.test.ts @@ -0,0 +1,74 @@ +import { expect, it } from "vitest"; +import { compareFlatMetrics } from "../src/compare.js"; +import { flattenRuntime } from "../src/generate-history.js"; +import type { RuntimeStats } from "../src/types.js"; + +function emptyRuntime(): RuntimeStats { + return { + total: 0, + loader: 0, + resolver: 0, + checker: 0, + validation: { total: 0, validators: {} }, + linter: { total: 0, rules: {} }, + emit: { total: 0, emitters: {} }, + }; +} + +it("compareFlatMetrics treats scoped labels as opaque keys", () => { + // Scoped package names contain a slash; they must be compared as whole labels, + // never split into a fake `@azure-tools` / `@typespec` metric. + const baseline = { + "emit/@azure-tools/typespec-client-generator-core": 100, + "emit/@typespec/openapi3": 50, + "emit/@typespec/openapi3/write": 10, + }; + const current = { + "emit/@azure-tools/typespec-client-generator-core": 110, + "emit/@typespec/openapi3": 50, + "emit/@typespec/openapi3/write": 12, + }; + + const metrics = compareFlatMetrics(baseline, current); + const byLabel = Object.fromEntries(metrics.map((m) => [m.label, m])); + + expect(Object.keys(byLabel).sort()).toEqual([ + "emit/@azure-tools/typespec-client-generator-core", + "emit/@typespec/openapi3", + "emit/@typespec/openapi3/write", + ]); + // No phantom "@azure-tools" / "@typespec" labels. + expect(metrics.some((m) => m.label === "emit/@azure-tools")).toBe(false); + expect(metrics.some((m) => m.label === "emit/@typespec")).toBe(false); + + const tcgc = byLabel["emit/@azure-tools/typespec-client-generator-core"]; + expect(tcgc.baseline).toBe(100); + expect(tcgc.current).toBe(110); +}); + +it("compareFlatMetrics defaults missing values to 0 on either side", () => { + const metrics = compareFlatMetrics({ checker: 100 }, { loader: 20 }); + const byLabel = Object.fromEntries(metrics.map((m) => [m.label, m])); + expect(byLabel["loader"]).toMatchObject({ baseline: 0, current: 20 }); + expect(byLabel["checker"]).toMatchObject({ baseline: 100, current: 0 }); +}); + +it("flattenRuntime keeps scoped emitter names intact and reversible via compare", () => { + const runtime = emptyRuntime(); + runtime.emit.emitters = { + "@azure-tools/typespec-client-generator-core": { total: 100, steps: {} }, + "@typespec/openapi3": { total: 50, steps: { compute: 40, write: 10 } }, + "local-emitter": { total: 5, steps: {} }, + }; + + const flat = flattenRuntime(runtime); + + expect(flat["emit/@azure-tools/typespec-client-generator-core"]).toBe(100); + expect(flat["emit/@typespec/openapi3"]).toBe(50); + expect(flat["emit/@typespec/openapi3/write"]).toBe(10); + expect(flat["emit/local-emitter"]).toBe(5); + + // Comparing a flat map against itself yields zero change for every label. + const metrics = compareFlatMetrics(flat, flat); + expect(metrics.every((m) => m.change === 0)).toBe(true); +});