diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0b1910c6..72be0299b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -126,6 +126,85 @@ jobs: CI: true NX_TASKS_RUNNER_DYNAMIC_OUTPUT: false + initial-bytes-ratchet: + name: Initial bytes ratchet + runs-on: ubuntu-latest + timeout-minutes: 30 + + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Install pnpm + uses: pnpm/action-setup@v6.0.10 + + - name: Setup Node.js + uses: actions/setup-node@v7 + with: + node-version-file: '.nvmrc' + cache: 'pnpm' + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + # The ratchet below compares a measurement with the baselines file + # of the same commit, so it cannot see a change that grows the + # payload and raises the baseline to match. Compare the file with + # the revision this one is measured against instead: the target + # branch for a pull request, the previous head for a master push, + # master for a manual dispatch. Any raised limit, widened tolerance + # or removed entry fails. + - name: Refuse baseline increases against the previous revision + env: + EVENT_NAME: ${{ github.event_name }} + BASE_REF: ${{ github.base_ref }} + BEFORE_SHA: ${{ github.event.before }} + run: | + set -euo pipefail + case "$EVENT_NAME" in + pull_request) + git fetch --no-tags --depth=1 origin "$BASE_REF" + ;; + push) + if [ -z "$BEFORE_SHA" ] || [ "$BEFORE_SHA" = "0000000000000000000000000000000000000000" ]; then + echo "No previous revision for this push; nothing to compare against." + exit 0 + fi + git fetch --no-tags --depth=1 origin "$BEFORE_SHA" + ;; + *) + git fetch --no-tags --depth=1 origin master + ;; + esac + git show "FETCH_HEAD:tools/performance/journey-baselines.json" > /tmp/base-journey-baselines.json 2>/dev/null || + rm -f /tmp/base-journey-baselines.json + node tools/performance/check-baseline-direction.mjs \ + --base /tmp/base-journey-baselines.json \ + --head tools/performance/journey-baselines.json + + # The production configuration is what users download; measuring + # any other build would ratchet a number nobody ships. + - name: Build web app (production) + run: pnpm nx build web --skip-nx-cache + env: + CI: true + NX_TASKS_RUNNER_DYNAMIC_OUTPUT: false + + # Fails when renderer.initialBytes exceeds the value committed in + # tools/performance/journey-baselines.json. Baselines only move + # down, with the printed measurement as evidence; the contract is + # docs/architecture/performance-journeys.md. + - name: Check renderer.initialBytes against the baseline + run: pnpm run perf:initial-bytes:check + + - name: Upload journey summary + if: always() + uses: actions/upload-artifact@v7 + with: + name: performance-journey-summary + path: dist/performance/ + retention-days: 14 + unit-and-typecheck: name: Unit Tests and Typechecks runs-on: ubuntu-latest diff --git a/README.md b/README.md index 9e43bd5bc..b325071ca 100644 --- a/README.md +++ b/README.md @@ -377,7 +377,7 @@ $ pnpm run serve:frontend ``` To see how many bytes the built web app puts on the initial load path (the -number the performance ratchet guards), build it and run the measurement: +number the CI ratchet guards), build it and run the measurement: ``` $ pnpm nx build web diff --git a/docs/architecture/performance-journeys.md b/docs/architecture/performance-journeys.md index e32387ce2..07fea1a88 100644 --- a/docs/architecture/performance-journeys.md +++ b/docs/architecture/performance-journeys.md @@ -2,12 +2,10 @@ IPTVnator measures performance through a small set of everyday user journeys. Each journey has deterministic counters that are asserted exactly, and -wall-clock timings that are recorded as evidence. Counters are meant to be -ratcheted in CI: a committed baseline that may only be lowered, and only with -the measured output as evidence. This document is the contract for that loop; -`tools/performance/` holds the scripts. The measurement script lands first; -the baseline file and the CI job follow in their own PRs (#1693, #1694), so -until they merge the reported number is informational, not enforced. +wall-clock timings that are recorded as evidence. Counters are ratcheted in CI: +a committed baseline may only be lowered, and only with the measured output as +evidence. This document is the contract for that loop; `tools/performance/` +holds the scripts. ## Journeys @@ -104,6 +102,31 @@ pnpm run perf:initial-bytes:check # measure dist/apps/web into dist/performanc pnpm run perf:ratchet:check # check every baseline against dist/performance/journey-summary.json ``` +CI runs `perf:initial-bytes:check` in the `Initial bytes ratchet` job of +`.github/workflows/ci.yml` after a production build of `apps/web`, and uploads +`dist/performance/` as the `performance-journey-summary` artifact. Like the +rest of that workflow it runs for pull requests that target `master` and for +pushes to `master`; a stacked PR that targets another branch gets no run until +it is retargeted, so dispatch one with `gh workflow run ci.yml --ref ` +when you need the number. A PR that grows the counter fails that job. + +That runner is the canonical measurer: take baseline values from its output, +not from a local build. A local macOS build of the code before #1695 is 2 +bytes smaller in `main.js` (the eager locale imports); since #1695 the two +have been byte-identical. (An apparent 556-byte platform difference during +the first measurements was otherwise `package.json` text embedded in +`main.js`, which moved with every script edit; #1692 fixed that by importing +only the version.) + +The job also refuses a weakened baselines file: +`tools/performance/check-baseline-direction.mjs` compares +`journey-baselines.json` with the revision the change is measured against +(the target branch of a pull request, the previous head of a `master` push, +`master` for a manual dispatch) and fails when any +entry's enforced limit (`value × toleranceRatio`) went up, a tolerance widened +or an entry disappeared, so a PR cannot grow the payload and raise the +baseline to match. Lowered limits and new entries pass. + Baselines only move down. Lower `value` in the same PR as the change that earned it, set `updatedAt` and `evidencePr`, and paste the measurement output into the PR. Never raise a value to make a PR pass: if growth is a deliberate diff --git a/docs/architecture/validation-map.md b/docs/architecture/validation-map.md index 94bc34017..a12e05f5f 100644 --- a/docs/architecture/validation-map.md +++ b/docs/architecture/validation-map.md @@ -165,8 +165,10 @@ pnpm nx test performance-tools `perf:initial-bytes` reads the built `dist/apps/web/index.html` and sums the bytes on the initial path (the J1 counter `renderer.initialBytes`). `perf:initial-bytes:check` then fails if the value exceeds -`tools/performance/journey-baselines.json`; baselines only move down. The -contract, what counts and how to add a counter are in the +`tools/performance/journey-baselines.json`; baselines only move down. CI runs +the same check in the `Initial bytes ratchet` job of `ci.yml` for PRs that +target `master` and for `master` pushes (dispatch it with +`gh workflow run ci.yml --ref ` for a stacked branch). The contract, what counts and how to add a counter are in the [performance journeys](performance-journeys.md) document. ## Logging diff --git a/package.json b/package.json index aadc8eb5d..067d236e3 100644 --- a/package.json +++ b/package.json @@ -74,7 +74,7 @@ "perf:initial-bytes": "node tools/performance/measure-initial-bytes.mjs", "perf:initial-bytes:check": "node tools/performance/measure-initial-bytes.mjs --summary dist/performance/initial-bytes.summary.json && node tools/performance/check-journey-ratchet.mjs --summary dist/performance/initial-bytes.summary.json --only launch/renderer.initialBytes", "perf:ratchet:check": "node tools/performance/check-journey-ratchet.mjs --summary dist/performance/journey-summary.json", - "perf:tools:test": "node --test tools/performance/measure-initial-bytes.test.mjs tools/performance/check-journey-ratchet.test.mjs", + "perf:tools:test": "node --test tools/performance/measure-initial-bytes.test.mjs tools/performance/check-journey-ratchet.test.mjs tools/performance/check-baseline-direction.test.mjs", "agents:validate": "node tools/skills/validate-agent-guidance.mjs", "skills:validate": "node tools/skills/validate-repository-skills.mjs", "release:artwork:dry-run": "tsx --tsconfig tsconfig.base.json tools/release/generate-marketing-artwork.ts --dry-run", diff --git a/tools/performance/check-baseline-direction.mjs b/tools/performance/check-baseline-direction.mjs new file mode 100644 index 000000000..332559edc --- /dev/null +++ b/tools/performance/check-baseline-direction.mjs @@ -0,0 +1,175 @@ +/** + * Refuses a change that weakens tools/performance/journey-baselines.json. + * + * The ratchet job compares a measurement with the baselines file of the same + * commit, so on its own it cannot tell a genuine payload reduction from a PR + * that grows the payload and raises the baseline by the same amount. This + * check closes that gap: given the baselines file of the target branch and + * the one of the PR, any entry whose enforced limit (`value × toleranceRatio`) + * went up, whose tolerance widened, or that disappeared, is a failure. New + * entries and lowered limits pass. + * + * Usage: + * node tools/performance/check-baseline-direction.mjs \ + * --base \ + * --head tools/performance/journey-baselines.json + * + * A missing --base file means the target branch has no baselines yet, so + * there is nothing that could have been weakened. + */ +import { existsSync } from 'node:fs'; +import { readFile } from 'node:fs/promises'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { validateBaselines } from './check-journey-ratchet.mjs'; + +export const DEFAULT_HEAD_PATH = 'tools/performance/journey-baselines.json'; + +function entries(baselines) { + const flat = new Map(); + for (const [journey, counters] of Object.entries(baselines.journeys)) { + for (const [name, entry] of Object.entries(counters)) { + flat.set(`${journey}/${name}`, entry); + } + } + return flat; +} + +function formatNumber(value) { + return value.toLocaleString('en-US', { maximumFractionDigits: 2 }); +} + +/** + * What the ratchet actually enforces: `value × toleranceRatio` for a + * wall-clock entry, the bare value for a counter. Comparing values alone would + * let a PR lower a value while widening the tolerance. + */ +function effectiveLimit(entry) { + return entry.value * (entry.toleranceRatio ?? 1); +} + +/** Pure comparison; `failures` non-empty means the change weakens the ratchet. */ +export function compareBaselineDirection({ base, head }) { + validateBaselines(base); + validateBaselines(head); + const result = { failures: [], lowered: [], unchanged: [], added: [] }; + const headEntries = entries(head); + + for (const [label, baseEntry] of entries(base)) { + const headEntry = headEntries.get(label); + const unit = baseEntry.unit ? ` ${baseEntry.unit}` : ''; + if (!headEntry) { + result.failures.push( + `${label}: baseline ${formatNumber(baseEntry.value)}${unit} was removed. Baselines are retired only by a maintainer decision recorded in the PR, not by deleting the entry.` + ); + continue; + } + const baseTolerance = baseEntry.toleranceRatio ?? 1; + const headTolerance = headEntry.toleranceRatio ?? 1; + const baseLimit = effectiveLimit(baseEntry); + const headLimit = effectiveLimit(headEntry); + if (headTolerance > baseTolerance) { + result.failures.push( + `${label}: toleranceRatio widened from ${baseTolerance} to ${headTolerance}. Tolerances are a maintainer decision; a PR may only narrow them.` + ); + } else if (headLimit > baseLimit) { + result.failures.push( + `${label}: baseline raised from ${formatNumber(baseLimit)} to ${formatNumber(headLimit)}${unit}. Baselines only move down; bring the measurement back under ${formatNumber(baseLimit)} or make the case for the increase in the PR.` + ); + } else if (headLimit < baseLimit) { + result.lowered.push( + `${label}: ${formatNumber(baseLimit)} -> ${formatNumber(headLimit)}${unit}.` + ); + } else { + result.unchanged.push(label); + } + } + for (const label of headEntries.keys()) { + if (!entries(base).has(label)) result.added.push(label); + } + return result; +} + +export function formatDirectionResult(result) { + const lines = []; + for (const line of result.lowered) lines.push(`lowered ${line}`); + for (const label of result.added) lines.push(`added ${label}`); + for (const line of result.failures) lines.push(`FAIL ${line}`); + lines.push( + result.failures.length > 0 + ? `Baseline direction check failed: ${result.failures.length} entries raised or removed.` + : `Baseline direction OK: ${result.unchanged.length} unchanged, ${result.lowered.length} lowered, ${result.added.length} added.` + ); + return lines.join('\n'); +} + +export function parseArgs(argv) { + const options = { base: null, head: DEFAULT_HEAD_PATH }; + for (let index = 0; index < argv.length; index += 1) { + const argument = argv[index]; + if (argument === '--') continue; + if (argument === '--base') { + options.base = argv[++index]; + } else if (argument.startsWith('--base=')) { + options.base = argument.slice('--base='.length); + } else if (argument === '--head') { + options.head = argv[++index]; + } else if (argument.startsWith('--head=')) { + options.head = argument.slice('--head='.length); + } else { + throw new Error(`Unknown argument: ${argument}`); + } + if (options.base === undefined || options.head === undefined) { + throw new Error(`Missing value for ${argument}`); + } + } + if (!options.base) { + throw new Error( + '--base is required.' + ); + } + return options; +} + +async function readJson(filePath, description) { + try { + return JSON.parse(await readFile(filePath, 'utf8')); + } catch (error) { + throw new Error( + `Cannot read ${description} at ${filePath}: ${error.message}` + ); + } +} + +const isMain = + process.argv[1] && + path.resolve(process.argv[1]) === + path.resolve(fileURLToPath(import.meta.url)); + +if (isMain) { + try { + const options = parseArgs(process.argv.slice(2)); + const basePath = path.resolve(options.base); + const base = existsSync(basePath) + ? await readJson(basePath, 'target-branch baselines') + : { version: 1, journeys: {} }; + if (!existsSync(basePath)) { + console.log( + `No baselines file at ${options.base} on the target branch; nothing to weaken.` + ); + } + const head = await readJson(path.resolve(options.head), 'baselines'); + const result = compareBaselineDirection({ base, head }); + const output = formatDirectionResult(result); + if (result.failures.length > 0) { + console.error(output); + process.exitCode = 1; + } else { + console.log(output); + } + } catch (error) { + console.error(`check-baseline-direction: ${error.message}`); + process.exitCode = 1; + } +} diff --git a/tools/performance/check-baseline-direction.test.mjs b/tools/performance/check-baseline-direction.test.mjs new file mode 100644 index 000000000..b3ec4e4c0 --- /dev/null +++ b/tools/performance/check-baseline-direction.test.mjs @@ -0,0 +1,221 @@ +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { after, before, test } from 'node:test'; + +import { + DEFAULT_HEAD_PATH, + compareBaselineDirection, + formatDirectionResult, + parseArgs, +} from './check-baseline-direction.mjs'; + +const scriptPath = fileURLToPath( + new URL('./check-baseline-direction.mjs', import.meta.url) +); + +const file = (initialBytes, extra = {}) => ({ + version: 1, + journeys: { + launch: { + 'renderer.initialBytes': { value: initialBytes, unit: 'bytes' }, + ...extra, + }, + }, +}); + +let workDir; +before(async () => { + workDir = await mkdtemp( + path.join(os.tmpdir(), 'check-baseline-direction-') + ); +}); +after(async () => { + await rm(workDir, { recursive: true, force: true }); +}); + +test('an unchanged or lowered baseline passes', () => { + const same = compareBaselineDirection({ base: file(100), head: file(100) }); + assert.deepEqual(same.failures, []); + assert.deepEqual(same.unchanged, ['launch/renderer.initialBytes']); + + const lowered = compareBaselineDirection({ + base: file(100), + head: file(90), + }); + assert.deepEqual(lowered.failures, []); + assert.match(lowered.lowered[0], /100 -> 90 bytes/); +}); + +test('a raised baseline fails with both values', () => { + const result = compareBaselineDirection({ + base: file(100), + head: file(101), + }); + assert.equal(result.failures.length, 1); + assert.match( + result.failures[0], + /raised from 100 to 101 bytes\. Baselines only move down/ + ); +}); + +test('a removed baseline fails; a new one is reported and passes', () => { + const removed = compareBaselineDirection({ + base: file(100, { cdTicks: { value: 5 } }), + head: file(100), + }); + assert.equal(removed.failures.length, 1); + assert.match( + removed.failures[0], + /launch\/cdTicks: baseline 5 was removed/ + ); + + const added = compareBaselineDirection({ + base: file(100), + head: file(100, { cdTicks: { value: 5 } }), + }); + assert.deepEqual(added.failures, []); + assert.deepEqual(added.added, ['launch/cdTicks']); +}); + +test('a widened or newly added tolerance fails even when the value went down', () => { + const widened = compareBaselineDirection({ + base: file(100, { + firstCardMs: { value: 100, unit: 'ms', toleranceRatio: 1.1 }, + }), + head: file(100, { + firstCardMs: { value: 99, unit: 'ms', toleranceRatio: 2 }, + }), + }); + assert.equal(widened.failures.length, 1); + assert.match( + widened.failures[0], + /firstCardMs: toleranceRatio widened from 1\.1 to 2/ + ); + + const added = compareBaselineDirection({ + base: file(100), + head: file(100, undefined) && { + version: 1, + journeys: { + launch: { + 'renderer.initialBytes': { + value: 100, + unit: 'bytes', + toleranceRatio: 1.25, + }, + }, + }, + }, + }); + assert.equal(added.failures.length, 1); + assert.match(added.failures[0], /toleranceRatio widened from 1 to 1\.25/); +}); + +test('the enforced limit is what is compared for wall-clock entries', () => { + const narrowed = compareBaselineDirection({ + base: file(100, { + firstCardMs: { value: 100, unit: 'ms', toleranceRatio: 1.25 }, + }), + head: file(100, { + firstCardMs: { value: 110, unit: 'ms', toleranceRatio: 1 }, + }), + }); + assert.deepEqual(narrowed.failures, []); + assert.match(narrowed.lowered[0], /firstCardMs: 125 -> 110 ms/); + + const raisedLimit = compareBaselineDirection({ + base: file(100, { + firstCardMs: { value: 100, unit: 'ms', toleranceRatio: 1.25 }, + }), + head: file(100, { + firstCardMs: { value: 130, unit: 'ms', toleranceRatio: 1.25 }, + }), + }); + assert.equal(raisedLimit.failures.length, 1); + assert.match( + raisedLimit.failures[0], + /firstCardMs: baseline raised from 125 to 162\.5 ms/ + ); +}); + +test('an empty target-branch file cannot be weakened', () => { + const result = compareBaselineDirection({ + base: { journeys: {} }, + head: file(100), + }); + assert.deepEqual(result.failures, []); + assert.deepEqual(result.added, ['launch/renderer.initialBytes']); +}); + +test('formats the outcome', () => { + assert.match( + formatDirectionResult( + compareBaselineDirection({ base: file(100), head: file(90) }) + ), + /^lowered {2}launch\/renderer\.initialBytes: 100 -> 90 bytes\.\nBaseline direction OK: 0 unchanged, 1 lowered, 0 added\.$/ + ); + assert.match( + formatDirectionResult( + compareBaselineDirection({ base: file(100), head: file(200) }) + ), + /^FAIL {5}launch.*\nBaseline direction check failed: 1 entries raised or removed\.$/ + ); +}); + +test('parses arguments and requires --base', () => { + assert.deepEqual(parseArgs(['--base', 'b.json']), { + base: 'b.json', + head: DEFAULT_HEAD_PATH, + }); + assert.deepEqual(parseArgs(['--', '--base=b.json', '--head=h.json']), { + base: 'b.json', + head: 'h.json', + }); + assert.throws( + () => parseArgs([]), + /--base is required/ + ); + assert.throws(() => parseArgs(['--base']), /Missing value for --base/); + assert.throws( + () => parseArgs(['--base', 'b', '--force']), + /Unknown argument: --force/ + ); +}); + +async function runCli(base, head) { + const basePath = + base === null + ? path.join(workDir, 'absent.json') + : path.join(workDir, `base-${Math.random()}.json`); + const headPath = path.join(workDir, `head-${Math.random()}.json`); + if (base !== null) await writeFile(basePath, JSON.stringify(base)); + await writeFile(headPath, JSON.stringify(head)); + return spawnSync( + process.execPath, + [scriptPath, '--base', basePath, '--head', headPath], + { + encoding: 'utf8', + } + ); +} + +test('CLI exits 0 for a lowered baseline, 1 for a raised one, 0 when the target branch has no file', async () => { + const lowered = await runCli(file(100), file(90)); + assert.equal(lowered.status, 0, lowered.stderr); + assert.match(lowered.stdout, /Baseline direction OK/); + + const raised = await runCli(file(100), file(101)); + assert.equal(raised.status, 1); + assert.match( + raised.stderr, + /FAIL {5}launch\/renderer\.initialBytes: baseline raised/ + ); + + const noBase = await runCli(null, file(100)); + assert.equal(noBase.status, 0, noBase.stderr); + assert.match(noBase.stdout, /nothing to weaken/); +}); diff --git a/tools/performance/project.json b/tools/performance/project.json index ca364bde7..ac5ff1f95 100644 --- a/tools/performance/project.json +++ b/tools/performance/project.json @@ -14,7 +14,7 @@ { "externalDependencies": ["parse5"] } ], "options": { - "command": "node --test tools/performance/measure-initial-bytes.test.mjs tools/performance/check-journey-ratchet.test.mjs", + "command": "node --test tools/performance/measure-initial-bytes.test.mjs tools/performance/check-journey-ratchet.test.mjs tools/performance/check-baseline-direction.test.mjs", "cwd": "{workspaceRoot}" } },