From 881cb159400a788d8285a82468f2e03b557d376e Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 23 Sep 2026 16:05:46 +1000 Subject: [PATCH 1/3] feat(perf): store bundle size Measurements for every main commit The perf-review Routine reads Measurements from refs/notes/perf. Bundle byte counts carry a per-Benchmark thresholdPercent, so normal feature growth is not a Regression. --- .github/routines.yml | 10 ++ .github/workflows/perf-comment.yml | 182 +++++++++++++++++++++ .github/workflows/perf-pull-request.yml | 120 ++++++++++++++ .github/workflows/perf.yml | 142 +++++++++++++++++ perf/benchmarks.json | 68 ++++++++ scripts/perf/cases/client-chunk-count.mjs | 5 + scripts/perf/cases/client-css-bytes.mjs | 4 + scripts/perf/cases/client-entry-bytes.mjs | 4 + scripts/perf/cases/client-js-bytes.mjs | 4 + scripts/perf/cases/server-bytes.mjs | 4 + scripts/perf/cases/server-gzip-bytes.mjs | 7 + scripts/perf/output.mjs | 92 +++++++++++ scripts/perf/report.mjs | 154 ++++++++++++++++++ scripts/perf/run.mjs | 185 ++++++++++++++++++++++ test/unit/perf-report.test.ts | 50 ++++++ 15 files changed, 1031 insertions(+) create mode 100644 .github/workflows/perf-comment.yml create mode 100644 .github/workflows/perf-pull-request.yml create mode 100644 .github/workflows/perf.yml create mode 100644 perf/benchmarks.json create mode 100644 scripts/perf/cases/client-chunk-count.mjs create mode 100644 scripts/perf/cases/client-css-bytes.mjs create mode 100644 scripts/perf/cases/client-entry-bytes.mjs create mode 100644 scripts/perf/cases/client-js-bytes.mjs create mode 100644 scripts/perf/cases/server-bytes.mjs create mode 100644 scripts/perf/cases/server-gzip-bytes.mjs create mode 100644 scripts/perf/output.mjs create mode 100644 scripts/perf/report.mjs create mode 100644 scripts/perf/run.mjs create mode 100644 test/unit/perf-report.test.ts diff --git a/.github/routines.yml b/.github/routines.yml index 2953ff92..80cad472 100644 --- a/.github/routines.yml +++ b/.github/routines.yml @@ -15,3 +15,13 @@ routines: timezone: Australia/Sydney mode: propose enabled: true + # Reads the Measurements .github/workflows/perf.yml stores on refs/notes/perf. + # It judges nothing until a Benchmark has ten usable Measurements, so the + # first several runs will report coverage and file nothing. + - name: perf-review + on: + schedule: + - cron: '0 11 * * 1' + timezone: Australia/Sydney + mode: report + enabled: true diff --git a/.github/workflows/perf-comment.yml b/.github/workflows/perf-comment.yml new file mode 100644 index 00000000..f6de846e --- /dev/null +++ b/.github/workflows/perf-comment.yml @@ -0,0 +1,182 @@ +name: Perf Comment + +# Posts the report Perf Pull Request produced. It runs after that workflow, so +# it holds the write token and the pull request's own code never does. Every +# value it trusts is checked against the workflow run before it is used. +on: + workflow_run: + workflows: + - Perf Pull Request + types: + - completed + +concurrency: + group: ${{ github.workflow }}-${{ github.event.workflow_run.head_repository.id }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: true + +permissions: {} + +jobs: + comment: + if: >- + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' + permissions: + actions: read + pull-requests: write + runs-on: [self-hosted, linux, x64, harlan-desktop-ci] + steps: + - name: Resolve the pull request + id: pull-request + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const run = context.payload.workflow_run + const headOwner = run.head_repository?.owner?.login + if (!headOwner || !run.head_branch) { + core.setFailed('The workflow run is missing its head repository or branch') + return + } + + const { data: pullRequests } = await github.rest.pulls.list({ + owner: context.repo.owner, + repo: context.repo.repo, + state: 'open', + head: `${headOwner}:${run.head_branch}`, + per_page: 100, + }) + const matches = pullRequests.filter(pullRequest => + pullRequest.state === 'open' + && pullRequest.head.sha === run.head_sha + && pullRequest.head.repo?.full_name === run.head_repository.full_name + && pullRequest.base.repo.full_name === `${context.repo.owner}/${context.repo.repo}`, + ) + + if (matches.length !== 1) { + core.setFailed(`Expected one open pull request for ${run.head_sha}, found ${matches.length}`) + return + } + + core.setOutput('number', matches[0].number) + + - name: Download the report + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: perf-report + path: ${{ runner.temp }}/perf-report + github-token: ${{ github.token }} + run-id: ${{ github.event.workflow_run.id }} + + - name: Check the report before trusting it + env: + COMMENT_PATH: ${{ runner.temp }}/perf-comment.md + EXPECTED_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + EXPECTED_PR_NUMBER: ${{ steps.pull-request.outputs.number }} + REPORT_DIR: ${{ runner.temp }}/perf-report + run: | + set -euo pipefail + report="$REPORT_DIR/report.md" + pr_number="$REPORT_DIR/pr-number.txt" + head_sha="$REPORT_DIR/head-sha.txt" + + for file in "$report" "$pr_number" "$head_sha"; do + if [ ! -f "$file" ] || [ -L "$file" ]; then + echo "::error::Missing or invalid report file: $file" + exit 1 + fi + done + + if [ "$(wc -c < "$pr_number")" -gt 16 ] || [ "$(wc -c < "$head_sha")" -gt 64 ]; then + echo "::error::Report metadata is too large" + exit 1 + fi + + if ! grep -Eq '^[0-9]+$' "$pr_number" \ + || [ "$(tr -d '\r\n' < "$pr_number")" != "$EXPECTED_PR_NUMBER" ]; then + echo "::error::The report names a different pull request than the workflow run" + exit 1 + fi + + if [ "$(tr -d '\r\n' < "$head_sha")" != "$EXPECTED_HEAD_SHA" ]; then + echo "::error::The report names a different head commit than the workflow run" + exit 1 + fi + + size=$(wc -c < "$report") + if [ "$size" -lt 1 ] || [ "$size" -gt 131072 ]; then + echo "::error::The report must be between 1 byte and 128 KiB" + exit 1 + fi + + if [ "$(head -n 1 "$report")" != '### ⚡ Perf' ]; then + echo "::error::The report has an unexpected format" + exit 1 + fi + + if [ -e "$COMMENT_PATH" ] || [ -L "$COMMENT_PATH" ]; then + echo "::error::Refusing to overwrite the checked comment path" + exit 1 + fi + # A benchmark id is repository content, so it could carry a mention. + sed 's/@/@/g' "$report" > "$COMMENT_PATH" + + - name: Post the comment + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + COMMENT_PATH: ${{ runner.temp }}/perf-comment.md + PR_NUMBER: ${{ steps.pull-request.outputs.number }} + with: + script: | + const { readFileSync } = require('node:fs') + + // GitHub returns transient 5xx, 429, and network errors. Retry + // those. Anything else fails the step at once. + async function withRetry(label, call) { + for (let attempt = 1; ; attempt++) { + try { + return await call() + } + catch (error) { + const status = error.status + const transient = status === undefined || status === 429 || status >= 500 + if (!transient || attempt >= 5) + throw error + const wait = Math.min(2000 * 2 ** (attempt - 1), 30000) + core.warning(`${label} hit a transient error (${error.message}). Retrying in ${wait}ms.`) + await new Promise(resolve => setTimeout(resolve, wait)) + } + } + } + + const { owner, repo } = context.repo + const issueNumber = Number(process.env.PR_NUMBER) + const marker = '' + const body = `${readFileSync(process.env.COMMENT_PATH, 'utf8')}\n${marker}` + + const comments = await withRetry('issues.listComments', () => + github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: issueNumber, + per_page: 100, + }), + ) + const existing = comments.find(comment => comment.body?.includes(marker)) + + if (!existing) { + await withRetry('issues.createComment', () => + github.rest.issues.createComment({ owner, repo, issue_number: issueNumber, body }), + ) + core.info(`Posted the perf comment on #${issueNumber}`) + return + } + + if (existing.body === body) { + core.info('The perf comment is unchanged. Leaving it alone.') + return + } + + await withRetry('issues.updateComment', () => + github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }), + ) + core.info(`Updated the perf comment on #${issueNumber}`) diff --git a/.github/workflows/perf-pull-request.yml b/.github/workflows/perf-pull-request.yml new file mode 100644 index 00000000..ae3c706b --- /dev/null +++ b/.github/workflows/perf-pull-request.yml @@ -0,0 +1,120 @@ +name: Perf Pull Request + +# Measures a pull request against its base, in one job, on one machine. +# It holds no write access and posts nothing. Perf Comment does that, from the +# artifact this job uploads. +# It is not a required check and it never blocks a merge. +on: + pull_request: + paths-ignore: + - '**/*.md' + - 'docs/**' + - LICENSE + +concurrency: + group: ${{ github.workflow }}-${{ github.event.number }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + measure: + # Safe on the workstation pool because the repository is private; a fork + # pull request cannot reach this runner. + runs-on: [self-hosted, linux, x64, harlan-desktop-ci] + timeout-minutes: 45 + steps: + - name: Check out the pull request + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + persist-credentials: false + + # Installs this revision only for the heartbeat wrapper. Each measured + # side installs from its own lockfile below. + - uses: harlan-zw/harlan-nuxt/.github/actions/harlan-desktop-setup@main + + # Both sides are exported from git into fresh directories. Nothing is + # restored into them: a restored skew-protection cache would copy the + # chunks of earlier deploys into the output and count them too. + - name: Export both commits + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + mkdir "$RUNNER_TEMP/head" "$RUNNER_TEMP/base" + git archive HEAD | tar -x -C "$RUNNER_TEMP/head" + git archive "$BASE_SHA" | tar -x -C "$RUNNER_TEMP/base" + + # The desktop runner sends SIGHUP after 45 seconds without output. + # Nitro can bundle longer than that, so the wrapper emits a heartbeat. + - name: Build the pull request + working-directory: ${{ runner.temp }}/head + env: + NODE_OPTIONS: --max-old-space-size=8192 + run: | + pnpm install --frozen-lockfile + "$GITHUB_WORKSPACE/node_modules/.bin/tsx" "$GITHUB_WORKSPACE/scripts/run-with-heartbeat.ts" pnpm build + + - name: Build the base commit + working-directory: ${{ runner.temp }}/base + env: + NODE_OPTIONS: --max-old-space-size=8192 + run: | + pnpm install --frozen-lockfile + "$GITHUB_WORKSPACE/node_modules/.bin/tsx" "$GITHUB_WORKSPACE/scripts/run-with-heartbeat.ts" pnpm build + + - name: Note whether the dependencies moved + id: deps + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + # Each side installs from its own lockfile, so this does not affect + # whether the comparison holds. It tells a reader where a delta may + # have come from. + if git diff --quiet "$BASE_SHA" HEAD -- pnpm-lock.yaml; then + echo "dependencies_changed=false" >> "$GITHUB_OUTPUT" + else + echo "dependencies_changed=true" >> "$GITHUB_OUTPUT" + fi + echo "label=$(git log -1 --format='%h · %cs' "$BASE_SHA")" >> "$GITHUB_OUTPUT" + + - name: Measure both builds + # Every value a pull request can influence arrives as an environment + # variable. Nothing from the event is pasted into this script. + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + DEPENDENCIES_CHANGED: ${{ steps.deps.outputs.dependencies_changed }} + run: | + node scripts/perf/run.mjs \ + --head "$RUNNER_TEMP/head" \ + --parent "$RUNNER_TEMP/base" \ + --commit "$HEAD_SHA" \ + --parent-commit "$BASE_SHA" \ + --dependencies-changed "$DEPENDENCIES_CHANGED" \ + --out "$RUNNER_TEMP/perf-report/measurement.json" + + - name: Write the report + env: + BASE_LABEL: ${{ github.base_ref }} @ ${{ steps.deps.outputs.label }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + MEASUREMENT: ${{ runner.temp }}/perf-report/measurement.json + PR_NUMBER: ${{ github.event.pull_request.number }} + REPORT_DIR: ${{ runner.temp }}/perf-report + run: | + set -euo pipefail + node scripts/perf/report.mjs | tee "$REPORT_DIR/report.md" + cat "$REPORT_DIR/report.md" >> "$GITHUB_STEP_SUMMARY" + printf '%s\n' "$PR_NUMBER" > "$REPORT_DIR/pr-number.txt" + printf '%s\n' "$HEAD_SHA" > "$REPORT_DIR/head-sha.txt" + + - name: Upload the report + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: perf-report + path: ${{ runner.temp }}/perf-report + if-no-files-found: error + retention-days: 14 diff --git a/.github/workflows/perf.yml b/.github/workflows/perf.yml new file mode 100644 index 00000000..cef690af --- /dev/null +++ b/.github/workflows/perf.yml @@ -0,0 +1,142 @@ +name: Perf + +# Measures every merged commit against its parent, in one job, on one machine. +# It runs after merge and never on a pull request, so it adds no pull request +# latency and gates nothing. The numbers land on refs/notes/perf. +# +# The harness, the case files, and perf/benchmarks.json always come from the +# revision of this workflow, not from the commit it measures. So a backfill of a +# commit older than the harness measures it with today's Benchmarks. +on: + push: + branches: [main] + paths-ignore: + - '**/*.md' + - 'docs/**' + - LICENSE + workflow_dispatch: + inputs: + commit: + description: A main commit to measure against its parent. Leave empty to measure the tip. + required: false + type: string + +# One group for each measured commit. A single shared group keeps only one +# pending run, so a backfill of several commits would cancel all but two. +concurrency: + group: perf-${{ inputs.commit || github.sha }} + cancel-in-progress: false + +permissions: + contents: write + +jobs: + measure: + runs-on: [self-hosted, linux, x64, harlan-desktop-ci] + timeout-minutes: 45 + steps: + - name: Check out the harness + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + + # Installs this revision only for the heartbeat wrapper. Each measured + # side installs from its own lockfile below. + - uses: harlan-zw/harlan-nuxt/.github/actions/harlan-desktop-setup@main + + - name: Name both commits + id: commits + env: + REQUESTED: ${{ inputs.commit }} + run: | + set -euo pipefail + commit="$(git rev-parse --verify "${REQUESTED:-$GITHUB_SHA}^{commit}")" + if ! git merge-base --is-ancestor "$commit" origin/main; then + echo "::error::$commit is not on main. Measure main commits only." + exit 1 + fi + parent="$(git rev-parse "$commit~1")" + echo "sha=$commit" >> "$GITHUB_OUTPUT" + echo "parent=$parent" >> "$GITHUB_OUTPUT" + # Each side installs from its own lockfile, so this does not affect + # whether the comparison holds. It tells a reader where a delta may + # have come from. + if git diff --quiet "$parent" "$commit" -- pnpm-lock.yaml; then + echo "dependencies_changed=false" >> "$GITHUB_OUTPUT" + else + echo "dependencies_changed=true" >> "$GITHUB_OUTPUT" + fi + + # Both sides are exported from git into fresh directories. Nothing is + # restored into them: a restored skew-protection cache would copy the + # chunks of earlier deploys into the output and count them too. + - name: Export both commits + env: + HEAD_SHA: ${{ steps.commits.outputs.sha }} + PARENT_SHA: ${{ steps.commits.outputs.parent }} + run: | + set -euo pipefail + mkdir "$RUNNER_TEMP/head" "$RUNNER_TEMP/parent" + git archive "$HEAD_SHA" | tar -x -C "$RUNNER_TEMP/head" + git archive "$PARENT_SHA" | tar -x -C "$RUNNER_TEMP/parent" + + # The desktop runner sends SIGHUP after 45 seconds without output. + # Nitro can bundle longer than that, so the wrapper emits a heartbeat. + - name: Build this commit + working-directory: ${{ runner.temp }}/head + env: + NODE_OPTIONS: --max-old-space-size=8192 + run: | + pnpm install --frozen-lockfile + "$GITHUB_WORKSPACE/node_modules/.bin/tsx" "$GITHUB_WORKSPACE/scripts/run-with-heartbeat.ts" pnpm build + + - name: Build the parent commit + working-directory: ${{ runner.temp }}/parent + env: + NODE_OPTIONS: --max-old-space-size=8192 + run: | + pnpm install --frozen-lockfile + "$GITHUB_WORKSPACE/node_modules/.bin/tsx" "$GITHUB_WORKSPACE/scripts/run-with-heartbeat.ts" pnpm build + + - name: Measure both builds + env: + DEPENDENCIES_CHANGED: ${{ steps.commits.outputs.dependencies_changed }} + HEAD_SHA: ${{ steps.commits.outputs.sha }} + PARENT_SHA: ${{ steps.commits.outputs.parent }} + run: | + node scripts/perf/run.mjs \ + --head "$RUNNER_TEMP/head" \ + --parent "$RUNNER_TEMP/parent" \ + --commit "$HEAD_SHA" \ + --parent-commit "$PARENT_SHA" \ + --dependencies-changed "$DEPENDENCIES_CHANGED" \ + --out "$RUNNER_TEMP/measurement.json" + + - name: Store the measurement + env: + HEAD_SHA: ${{ steps.commits.outputs.sha }} + run: | + set -euo pipefail + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + # Another run can land a note between the fetch and the push, so + # take the remote copy and reapply on top of it. + for attempt in 1 2 3; do + git fetch origin "refs/notes/perf:refs/notes/perf" || true + git notes --ref=perf add -f -F "$RUNNER_TEMP/measurement.json" "$HEAD_SHA" + if git push origin refs/notes/perf; then + exit 0 + fi + git update-ref -d refs/notes/perf || true + sleep $((attempt * 5)) + done + echo "Could not store the measurement after three attempts." + exit 1 + + - name: Keep the measurement when the note cannot be stored + if: failure() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: measurement + path: ${{ runner.temp }}/measurement.json + if-no-files-found: warn diff --git a/perf/benchmarks.json b/perf/benchmarks.json new file mode 100644 index 00000000..f589a4dc --- /dev/null +++ b/perf/benchmarks.json @@ -0,0 +1,68 @@ +{ + "harness": 1, + "repeats": 1, + "warmups": 0, + "build": "pnpm build", + "benchmarks": [ + { + "id": "server/bytes", + "kind": "count", + "unit": "bytes", + "case": "scripts/perf/cases/server-bytes.mjs", + "prepare": false, + "thresholdPercent": 2, + "source": "nuxt.config.ts", + "measures": "total bytes of the Worker bundle in .output/server, source maps left out" + }, + { + "id": "server/gzip-bytes", + "kind": "count", + "unit": "bytes", + "case": "scripts/perf/cases/server-gzip-bytes.mjs", + "prepare": false, + "thresholdPercent": 2, + "source": "nuxt.config.ts", + "measures": "gzip bytes of the Worker bundle, the size Cloudflare limits" + }, + { + "id": "client/js-bytes", + "kind": "count", + "unit": "bytes", + "case": "scripts/perf/cases/client-js-bytes.mjs", + "prepare": false, + "thresholdPercent": 2, + "source": "nuxt.config.ts", + "measures": "total bytes of the client JavaScript this build produced" + }, + { + "id": "client/css-bytes", + "kind": "count", + "unit": "bytes", + "case": "scripts/perf/cases/client-css-bytes.mjs", + "prepare": false, + "thresholdPercent": 3, + "source": "app/assets/css/main.css", + "measures": "total bytes of the client CSS this build produced" + }, + { + "id": "client/chunk-count", + "kind": "count", + "unit": "chunks", + "case": "scripts/perf/cases/client-chunk-count.mjs", + "prepare": false, + "thresholdPercent": 5, + "source": "nuxt.config.ts", + "measures": "number of client JavaScript chunks" + }, + { + "id": "client/entry-bytes", + "kind": "count", + "unit": "bytes", + "case": "scripts/perf/cases/client-entry-bytes.mjs", + "prepare": false, + "thresholdPercent": 1, + "source": "app/app.vue", + "measures": "bytes of the client entry chunk every page loads before it hydrates" + } + ] +} diff --git a/scripts/perf/cases/client-chunk-count.mjs b/scripts/perf/cases/client-chunk-count.mjs new file mode 100644 index 00000000..f4f4f589 --- /dev/null +++ b/scripts/perf/cases/client-chunk-count.mjs @@ -0,0 +1,5 @@ +import { clientFiles, countCase } from '../output.mjs' + +// Number of client JavaScript chunks. A lost code split moves this down, a +// page that splits into many small requests moves it up. +countCase(root => clientFiles(root).filter(path => path.endsWith('.js')).length) diff --git a/scripts/perf/cases/client-css-bytes.mjs b/scripts/perf/cases/client-css-bytes.mjs new file mode 100644 index 00000000..66a610f3 --- /dev/null +++ b/scripts/perf/cases/client-css-bytes.mjs @@ -0,0 +1,4 @@ +import { bytes, clientFiles, countCase } from '../output.mjs' + +// Total bytes of the client CSS this build produced. +countCase(root => bytes(clientFiles(root).filter(path => path.endsWith('.css')))) diff --git a/scripts/perf/cases/client-entry-bytes.mjs b/scripts/perf/cases/client-entry-bytes.mjs new file mode 100644 index 00000000..ae460faf --- /dev/null +++ b/scripts/perf/cases/client-entry-bytes.mjs @@ -0,0 +1,4 @@ +import { bytes, countCase, entryFile } from '../output.mjs' + +// Bytes of the client entry chunk. Every page loads it before it can hydrate. +countCase(root => bytes([entryFile(root)])) diff --git a/scripts/perf/cases/client-js-bytes.mjs b/scripts/perf/cases/client-js-bytes.mjs new file mode 100644 index 00000000..456e67ad --- /dev/null +++ b/scripts/perf/cases/client-js-bytes.mjs @@ -0,0 +1,4 @@ +import { bytes, clientFiles, countCase } from '../output.mjs' + +// Total bytes of the client JavaScript this build produced. +countCase(root => bytes(clientFiles(root).filter(path => path.endsWith('.js')))) diff --git a/scripts/perf/cases/server-bytes.mjs b/scripts/perf/cases/server-bytes.mjs new file mode 100644 index 00000000..e4654f8f --- /dev/null +++ b/scripts/perf/cases/server-bytes.mjs @@ -0,0 +1,4 @@ +import { bytes, countCase, serverFiles } from '../output.mjs' + +// Total bytes of the Worker bundle in .output/server, source maps left out. +countCase(root => bytes(serverFiles(root))) diff --git a/scripts/perf/cases/server-gzip-bytes.mjs b/scripts/perf/cases/server-gzip-bytes.mjs new file mode 100644 index 00000000..5b388631 --- /dev/null +++ b/scripts/perf/cases/server-gzip-bytes.mjs @@ -0,0 +1,7 @@ +import { countCase, gzipBytes, serverFiles } from '../output.mjs' + +// Gzip bytes of the Worker bundle. Cloudflare applies its Worker size limit to +// the compressed upload, so this is the number that can block a deploy. The +// entry file itself is a 2 KB shim that imports every chunk, so it counts all +// of them. +countCase(root => gzipBytes(serverFiles(root))) diff --git a/scripts/perf/output.mjs b/scripts/perf/output.mjs new file mode 100644 index 00000000..b01cd395 --- /dev/null +++ b/scripts/perf/output.mjs @@ -0,0 +1,92 @@ +import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs' +import { join, relative, sep } from 'node:path' +import process from 'node:process' +import { gzipSync } from 'node:zlib' + +// Shared by the case files. Each case counts one thing in a built `.output`, +// so it carries no measurement noise: the same build gives the same number. + +const BUILD_ASSETS = 'public/_nuxt/v2' + +function argument(name) { + const index = process.argv.indexOf(`--${name}`) + const value = index === -1 ? undefined : process.argv[index + 1] + if (value === undefined) + throw new Error(`Pass --${name}`) + return value +} + +function files(directory) { + return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { + const path = join(directory, entry.name) + return entry.isDirectory() ? files(path) : [path] + }) +} + +/** The `.output` directory of the checkout the harness names with `--root`. */ +export function outputRoot() { + const root = join(argument('root'), '.output') + if (!existsSync(join(root, 'nitro.json'))) + throw new Error(`${root} holds no Nitro build. Build this checkout first.`) + return root +} + +/** Every server file except source maps. This is what wrangler uploads. */ +export function serverFiles(root) { + return files(join(root, 'server')).filter(path => !path.endsWith('.map')) +} + +/** + * Client chunks that this build produced. + * + * nuxt-skew-protection copies the chunks of earlier deploys into the output, + * so old sessions still load. A build that restored that cache would count + * every earlier deploy as well. `latest.json` names those retained chunks, so + * they are left out here. + */ +export function clientFiles(root) { + const assets = join(root, BUILD_ASSETS) + const latest = join(assets, 'builds/latest.json') + const retained = new Set() + if (existsSync(latest)) { + const versions = JSON.parse(readFileSync(latest, 'utf8')).skewProtection?.versions ?? {} + for (const version of Object.values(versions)) { + for (const chunk of version.deletedChunks ?? []) + retained.add(chunk) + } + } + const publicRoot = join(root, 'public') + return files(assets) + .filter(path => !path.endsWith('.map')) + .filter(path => !retained.has(relative(publicRoot, path).split(sep).join('/'))) +} + +/** + * The client entry chunk, read from the server's precomputed render manifest. + * + * Exactly one entry must match. Anything else means Nuxt changed the manifest + * shape, and a guess would store a wrong number. + */ +export function entryFile(root) { + const manifest = readFileSync(join(root, 'server/chunks/virtual/precomputed.mjs'), 'utf8') + const entries = new Set([...manifest.matchAll(/file:"([^"]+\.js)",name:"entry",src:"[^"]*",isEntry:!0/g)].map(match => match[1])) + if (entries.size !== 1) + throw new Error(`Expected one client entry in the render manifest, found ${entries.size}.`) + return join(root, BUILD_ASSETS, [...entries][0]) +} + +export const bytes = paths => paths.reduce((total, path) => total + statSync(path).size, 0) + +/** Default level, the same one a Worker upload reports. */ +export const gzipBytes = paths => paths.reduce((total, path) => total + gzipSync(readFileSync(path)).length, 0) + +/** Answers the harness protocol for a case that counts. */ +export function countCase(count) { + const command = process.argv[2] + if (command === 'prepare') + console.log(JSON.stringify({ value: 0 })) + else if (command === 'sample') + console.log(JSON.stringify({ value: count(outputRoot()), checksum: 'count' })) + else + throw new Error(`Unknown command: ${command}`) +} diff --git a/scripts/perf/report.mjs b/scripts/perf/report.mjs new file mode 100644 index 00000000..2c72c346 --- /dev/null +++ b/scripts/perf/report.mjs @@ -0,0 +1,154 @@ +#!/usr/bin/env node +import { existsSync, readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import process from 'node:process' +import { fileURLToPath } from 'node:url' + +// Renders one measurement as the pull request comment. +// +// The gate is measured, not assumed. Every benchmark carries the delta the +// same build produced against itself in the same job, so a reading counts only +// when it beats that noise by a margin. A count benchmark has no noise at all. +// It counts only above the `thresholdPercent` perf/benchmarks.json declares for +// it, because a site's bundle grows a little with most feature commits. +const TIME_FLOOR_PERCENT = 5 +const NOISE_MARGIN = 2 + +function formatBytes(bytes) { + const absolute = Math.abs(bytes) + if (absolute >= 1024 * 1024) + return `${(bytes / 1024 ** 2).toFixed(2)} MiB` + if (absolute >= 1024) + return `${(bytes / 1024).toFixed(1)} KiB` + return `${bytes} B` +} + +function formatValue(benchmark, value) { + if (benchmark.unit === 'bytes') + return formatBytes(value) + if (benchmark.unit === 'ms') + return `${value.toFixed(2)} ms` + return `${value} ${benchmark.unit}` +} + +function formatPercent(percent) { + if (percent === null) + return '—' + return `${percent > 0 ? '+' : ''}${percent.toFixed(2)}%` +} + +function escapeCell(value) { + return String(value).replaceAll('|', '|').replace(/[\r\n]+/g, ' ') +} + +/** The delta a benchmark must beat before anyone calls it a change. */ +function gatePercent(benchmark, thresholds) { + if (benchmark.kind === 'count') + return thresholds[benchmark.id] ?? 0 + const noise = Math.abs(benchmark.controlPercent ?? 0) * NOISE_MARGIN + return Math.max(TIME_FLOOR_PERCENT, noise) +} + +function classify(benchmark, thresholds) { + const delta = benchmark.deltaPercent + if (delta === null || !benchmark.verified) + return { ...benchmark, _tag: 'Unusable' } + const gate = gatePercent(benchmark, thresholds) + if (Math.abs(delta) <= gate) + return { ...benchmark, _tag: 'Unchanged', gate } + return { ...benchmark, _tag: delta > 0 ? 'Slower' : 'Faster', gate } +} + +function subject(row) { + const amount = formatValue(row, Math.abs(row.head.min - row.parent.min)) + return `\`${escapeCell(row.id)}\` ${row._tag === 'Slower' ? 'grew' : 'fell'} by ${amount} (${formatPercent(Math.abs(row.deltaPercent))})` +} + +function renderOutcome(rows) { + const unusable = rows.filter(row => row._tag === 'Unusable') + const slower = rows.filter(row => row._tag === 'Slower') + const faster = rows.filter(row => row._tag === 'Faster') + if (unusable.length) + return `⚠️ **${unusable.length} benchmark${unusable.length === 1 ? '' : 's'} produced no usable reading.** The two sides disagreed on their output, or the case failed.` + if (slower.length === 1 && faster.length === 0) + return `🔴 **${subject(slower[0])}.**` + if (faster.length === 1 && slower.length === 0) + return `🟢 **${subject(faster[0])}.**` + if (slower.length) + return `⚠️ **${slower.length} slower and ${faster.length} faster, past the noise this run measured.**` + if (faster.length) + return `🟢 **${faster.length} benchmark${faster.length === 1 ? '' : 's'} improved past the noise this run measured.**` + return '✅ **No clear performance change.**' +} + +function renderTable(rows) { + const output = [ + '| Benchmark | Base | This PR | Change | Noise |', + '|---|---:|---:|---:|---:|', + ] + for (const row of rows) { + const mark = row._tag === 'Slower' ? '🔴 ' : row._tag === 'Faster' ? '🟢 ' : '' + const change = row._tag === 'Unusable' ? '—' : `${mark}${formatPercent(row.deltaPercent)}` + output.push(`| \`${escapeCell(row.id)}\` | ${formatValue(row, row.parent.min)} | ${formatValue(row, row.head.min)} | ${change} | ${formatPercent(row.controlPercent)} |`) + } + return output +} + +function renderHowToRead(rows) { + const counts = rows.filter(row => row.kind === 'count').length + return [ + '
How to read this', + '', + 'Both revisions are built and measured in one job, on one machine, alternating within seconds. Machine class, CPU model, and noisy neighbours hit both sides equally, so they cancel in the change column.', + '', + '**Noise** is this run marking its own homework. It is the same build measured a second time against itself, so it should be zero and never is. A change counts only when it clears twice the noise, or 5%, whichever is larger.', + '', + counts > 0 + ? `A \`count\` benchmark carries no noise at all, so any movement there is real. It counts as a change only above the threshold \`perf/benchmarks.json\` declares for it, because most feature work grows the bundle a little. ${counts} of ${rows.length} here ${counts === 1 ? 'is' : 'are'} a count.` + : 'Every benchmark here is timed, so every reading carries noise.', + '', + 'Values are the fastest of the repeats. The minimum leads, because noise only ever adds.', + '', + '
', + ] +} + +/** + * Reads each count Benchmark's `thresholdPercent` from the manifest. + * + * A Measurement does not carry it. The manifest of the revision under review + * decides, the same one whose cases took the samples. + */ +export function readThresholds(manifest) { + const thresholds = {} + for (const benchmark of manifest.benchmarks ?? []) { + if (benchmark.thresholdPercent === undefined) + continue + if (typeof benchmark.thresholdPercent !== 'number' || !(benchmark.thresholdPercent >= 0)) + throw new TypeError(`${benchmark.id}: thresholdPercent must be a number of 0 or more.`) + thresholds[benchmark.id] = benchmark.thresholdPercent + } + return thresholds +} + +export function renderReport(measurement, baseLabel = '', thresholds = {}) { + const rows = (measurement.benchmarks ?? []).map(benchmark => classify(benchmark, thresholds)) + const output = ['### ⚡ Perf', '', renderOutcome(rows), ''] + if (rows.length) + output.push(...renderTable(rows), '', ...renderHowToRead(rows)) + else + output.push('No benchmark produced a reading.') + if (measurement.dependenciesChanged === true) + output.push('', '> ℹ️ This pull request changes `pnpm-lock.yaml`. Each side still builds from its own lockfile, so the comparison holds. A change above may belong to a dependency rather than to this repository.') + if (baseLabel) + output.push('', `Base: ${escapeCell(baseLabel)}. Both sides ran on the same runner, ${measurement.repeats ?? rows[0]?.repeats ?? 'several'} repeats each.`) + return `${output.join('\n')}\n` +} + +if (process.argv[1] && fileURLToPath(import.meta.url) === resolve(process.argv[1])) { + const path = process.env.MEASUREMENT + if (!path || !existsSync(path)) + throw new TypeError('MEASUREMENT must point to a measurement produced by scripts/perf/run.mjs.') + const manifest = JSON.parse(readFileSync(resolve(process.env.MANIFEST ?? 'perf/benchmarks.json'), 'utf8')) + process.stdout.write(renderReport(JSON.parse(readFileSync(path, 'utf8')), process.env.BASE_LABEL, readThresholds(manifest))) +} diff --git a/scripts/perf/run.mjs b/scripts/perf/run.mjs new file mode 100644 index 00000000..0747bb72 --- /dev/null +++ b/scripts/perf/run.mjs @@ -0,0 +1,185 @@ +import { spawnSync } from 'node:child_process' +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { cpus, tmpdir } from 'node:os' +import { dirname, join, resolve } from 'node:path' +import process from 'node:process' + +// Measures two revisions of the same code against each other in one process +// tree, on one machine, within seconds. Machine class, CPU model, thermal +// state, and noisy neighbours hit both sides equally, so they cancel in the +// delta. A number compared against another machine's number is not fixable by +// statistics, so this harness never compares across runs. +// +// node scripts/perf/run.mjs --head --parent --out perf.json +// +// Each side's is a full checkout whose packages are already built. +// Case files always come from this revision, so both sides run identical +// measurement code against different builds. + +function argument(name, fallback) { + const index = process.argv.indexOf(`--${name}`) + if (index === -1) { + if (fallback === undefined) + throw new Error(`Pass --${name}`) + return fallback + } + const value = process.argv[index + 1] + if (value === undefined || value.startsWith('--')) + throw new Error(`--${name} needs a value`) + return value +} + +/** + * Reads a true or false flag, and refuses anything else. + * + * A caller that passes an empty value means something went wrong on its side. + * Reading that as false would hide the fault and store a wrong measurement, so + * it fails here instead. + */ +function flag(name) { + const value = argument(name, 'false') + if (value !== 'true' && value !== 'false') + throw new Error(`--${name} takes true or false, not ${JSON.stringify(value)}`) + return value === 'true' +} + +/** Runs one case file in a fresh process and reads its single JSON line. */ +function sample(caseFile, command, root, fixtures) { + const result = spawnSync(process.execPath, [ + '--expose-gc', + '--max-old-space-size=2048', + caseFile, + command, + '--root', + root, + '--fixtures', + fixtures, + ], { encoding: 'utf8', maxBuffer: 8 * 1024 * 1024 }) + if (result.error) + throw result.error + if (result.status !== 0) + throw new Error(`${caseFile} ${command} failed: ${result.stderr}`) + const line = result.stdout.trim().split('\n').at(-1) + const parsed = JSON.parse(line) + if (typeof parsed.value !== 'number' || !Number.isFinite(parsed.value)) + throw new Error(`${caseFile} reported no numeric value`) + return parsed +} + +function median(values) { + const sorted = [...values].sort((left, right) => left - right) + const middle = Math.floor(sorted.length / 2) + return sorted.length % 2 === 0 ? (sorted[middle - 1] + sorted[middle]) / 2 : sorted[middle] +} + +function round(value) { + return Math.round(value * 1000) / 1000 +} + +const headRoot = resolve(argument('head')) +const parentRoot = resolve(argument('parent')) +const outPath = resolve(argument('out', 'perf.json')) +const manifestPath = resolve(argument('manifest', 'perf/benchmarks.json')) +const only = process.argv.includes('--only') ? argument('only') : null + +const manifest = JSON.parse(readFileSync(manifestPath, 'utf8')) +const fixtures = mkdtempSync(join(tmpdir(), 'skilld-perf-')) +const benchmarks = [] + +try { + for (const benchmark of manifest.benchmarks) { + if (only !== null && benchmark.id !== only) + continue + const caseFile = resolve(benchmark.case) + // A multiple of three, so each of the three sides occupies each position + // in the rotation the same number of times. An uneven count hands one + // side more first places than the others, and first place is measurably + // different from the rest. + const repeats = benchmark.repeats ?? manifest.repeats ?? 9 + const warmups = benchmark.warmups ?? manifest.warmups ?? 2 + + // Fixtures are built once, by this revision, and both sides read the same + // bytes. A side that generated its own fixtures would measure its own + // encoder as well as the case under test. + if (benchmark.prepare !== false) + sample(caseFile, 'prepare', headRoot, fixtures) + + for (let index = 0; index < warmups; index++) { + sample(caseFile, 'sample', headRoot, fixtures) + sample(caseFile, 'sample', parentRoot, fixtures) + } + + // Three sides, not two. The control is the head build measured a second + // time, so its delta against head is pure measurement noise. It answers + // the only question that matters for a threshold: how big is a difference + // this machine invents on its own, today, for this case? + const sides = { head: [], parent: [], control: [] } + const roots = { head: headRoot, parent: parentRoot, control: headRoot } + const names = ['head', 'parent', 'control'] + const checksums = new Set() + for (let index = 0; index < repeats; index++) { + // Rotating the order stops one side from always paying for whatever the + // machine does first in a group. + for (let offset = 0; offset < names.length; offset++) { + const name = names[(index + offset) % names.length] + const taken = sample(caseFile, 'sample', roots[name], fixtures) + sides[name].push(taken.value) + checksums.add(taken.checksum ?? null) + } + } + + const headMin = Math.min(...sides.head) + const parentMin = Math.min(...sides.parent) + const controlMin = Math.min(...sides.control) + // The minimum leads, because noise only ever adds. A count kind has no + // noise to remove, so both statistics agree there. + const percent = (value, against) => against === 0 ? null : round((value - against) / against * 100) + const deltaPercent = percent(headMin, parentMin) + const controlPercent = percent(controlMin, headMin) + benchmarks.push({ + id: benchmark.id, + kind: benchmark.kind, + unit: benchmark.unit, + repeats, + head: { min: round(headMin), median: round(median(sides.head)) }, + parent: { min: round(parentMin), median: round(median(sides.parent)) }, + control: { min: round(controlMin), median: round(median(sides.control)) }, + deltaPercent, + // How far the same build moved against itself in this same job. A + // reader must clear this before it calls a delta a slowdown. One + // control is one sample of the noise, so the rule belongs to the + // reader, not here: this harness reports, it does not judge. + controlPercent, + // Both sides must agree on what they produced. A faster revision that + // returns different bytes is a defect, never a win. + verified: checksums.size === 1, + }) + } +} +finally { + rmSync(fixtures, { force: true, recursive: true }) +} + +const measurement = { + harness: manifest.harness, + commit: argument('commit', 'unknown'), + parentCommit: argument('parent-commit', 'unknown'), + // Each side installs from its own lockfile, so a dependency change does not + // invalidate the comparison. It does explain one: a delta here may belong to + // the dependency rather than to this repository's code. + dependenciesChanged: flag('dependencies-changed'), + runner: process.env.RUNNER_NAME === undefined ? 'local' : process.env.ImageOS ?? 'unknown', + node: process.version, + cpu: cpus()[0]?.model ?? 'unknown', + measuredAt: new Date().toISOString(), + benchmarks, +} + +// The caller names a path, not a directory it has to remember to create. +mkdirSync(dirname(outPath), { recursive: true }) +writeFileSync(outPath, `${JSON.stringify(measurement, null, 2)}\n`) +const signed = value => value === null ? 'n/a' : `${value > 0 ? '+' : ''}${value}%` +for (const benchmark of benchmarks) { + const verified = benchmark.verified ? '' : ' UNVERIFIED: the sides disagree on their output' + console.log(`${benchmark.id} head ${benchmark.head.min} ${benchmark.unit} parent ${benchmark.parent.min} ${benchmark.unit} delta ${signed(benchmark.deltaPercent)} noise ${signed(benchmark.controlPercent)}${verified}`) +} diff --git a/test/unit/perf-report.test.ts b/test/unit/perf-report.test.ts new file mode 100644 index 00000000..c5acad67 --- /dev/null +++ b/test/unit/perf-report.test.ts @@ -0,0 +1,50 @@ +// @vitest-environment node +import { describe, expect, it } from 'vitest' +// @ts-expect-error the perf harness is plain JavaScript, with no types +import { readThresholds, renderReport } from '../../scripts/perf/report.mjs' + +function measurement(headMin: number, parentMin: number) { + return { + harness: 1, + benchmarks: [{ + id: 'client/js-bytes', + kind: 'count', + unit: 'bytes', + repeats: 1, + head: { min: headMin, median: headMin }, + parent: { min: parentMin, median: parentMin }, + control: { min: headMin, median: headMin }, + deltaPercent: (headMin - parentMin) / parentMin * 100, + controlPercent: 0, + verified: true, + }], + } +} + +function manifest(thresholdPercent?: unknown) { + return { + benchmarks: [{ id: 'client/js-bytes', kind: 'count', ...(thresholdPercent === undefined ? {} : { thresholdPercent }) }], + } +} + +describe('perf report thresholds', () => { + it('calls count growth below its threshold no change, because feature work grows the bundle', () => { + const report = renderReport(measurement(101_500, 100_000), '', readThresholds(manifest(2))) + expect(report).toContain('✅ **No clear performance change.**') + }) + + it('calls count growth above its threshold a regression', () => { + const report = renderReport(measurement(102_500, 100_000), '', readThresholds(manifest(2))) + expect(report).toContain('🔴 **`client/js-bytes` grew by') + }) + + it('treats any count movement as a change when the manifest sets no threshold', () => { + const report = renderReport(measurement(100_001, 100_000), '', readThresholds(manifest())) + expect(report).toContain('🔴') + }) + + it('refuses a threshold that is not a non-negative number', () => { + expect(() => readThresholds(manifest('2'))).toThrow('thresholdPercent') + expect(() => readThresholds(manifest(-1))).toThrow('thresholdPercent') + }) +}) From a4304aba6cbfbade69453cb560739128ef00be2e Mon Sep 17 00:00:00 2001 From: Harlan GitHub Agent Date: Wed, 23 Sep 2026 17:37:53 +1000 Subject: [PATCH 2/3] fix(perf): mark ambiguous count moves changed, disable perf-review --- .github/routines.yml | 7 +++- perf/benchmarks.json | 1 + scripts/perf/report.mjs | 61 ++++++++++++++++++++++++++--------- test/unit/perf-report.test.ts | 47 +++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 17 deletions(-) diff --git a/.github/routines.yml b/.github/routines.yml index 80cad472..d1c19be3 100644 --- a/.github/routines.yml +++ b/.github/routines.yml @@ -18,10 +18,15 @@ routines: # Reads the Measurements .github/workflows/perf.yml stores on refs/notes/perf. # It judges nothing until a Benchmark has ten usable Measurements, so the # first several runs will report coverage and file nothing. + # + # Disabled until harlan-zw/harlan-agent-kit#355 merges. The deployed Skill + # treats any count movement as a Regression without it, so it would judge + # every count Benchmark against semantics perf/benchmarks.json contradicts. + # Flip enabled back to true once that pull request lands. - name: perf-review on: schedule: - cron: '0 11 * * 1' timezone: Australia/Sydney mode: report - enabled: true + enabled: false diff --git a/perf/benchmarks.json b/perf/benchmarks.json index f589a4dc..abb13518 100644 --- a/perf/benchmarks.json +++ b/perf/benchmarks.json @@ -51,6 +51,7 @@ "case": "scripts/perf/cases/client-chunk-count.mjs", "prepare": false, "thresholdPercent": 5, + "direction": "either", "source": "nuxt.config.ts", "measures": "number of client JavaScript chunks" }, diff --git a/scripts/perf/report.mjs b/scripts/perf/report.mjs index 2c72c346..f53476f8 100644 --- a/scripts/perf/report.mjs +++ b/scripts/perf/report.mjs @@ -49,27 +49,46 @@ function gatePercent(benchmark, thresholds) { return Math.max(TIME_FLOOR_PERCENT, noise) } -function classify(benchmark, thresholds) { +function classify(benchmark, rules) { const delta = benchmark.deltaPercent if (delta === null || !benchmark.verified) return { ...benchmark, _tag: 'Unusable' } - const gate = gatePercent(benchmark, thresholds) + const gate = gatePercent(benchmark, rules.thresholds) if (Math.abs(delta) <= gate) return { ...benchmark, _tag: 'Unchanged', gate } + // A count the manifest marks `direction: either` is suspect both ways: a + // lost code split moves a chunk count down, a page of tiny requests moves + // it up. The report cannot grade it, so it flags it for a read instead. + if (rules.directions[benchmark.id] === 'either') + return { ...benchmark, _tag: 'Changed', gate } return { ...benchmark, _tag: delta > 0 ? 'Slower' : 'Faster', gate } } function subject(row) { const amount = formatValue(row, Math.abs(row.head.min - row.parent.min)) - return `\`${escapeCell(row.id)}\` ${row._tag === 'Slower' ? 'grew' : 'fell'} by ${amount} (${formatPercent(Math.abs(row.deltaPercent))})` + const verb = row.deltaPercent > 0 ? 'grew' : 'fell' + return `\`${escapeCell(row.id)}\` ${verb} by ${amount} (${formatPercent(Math.abs(row.deltaPercent))})` } function renderOutcome(rows) { const unusable = rows.filter(row => row._tag === 'Unusable') + const changed = rows.filter(row => row._tag === 'Changed') const slower = rows.filter(row => row._tag === 'Slower') const faster = rows.filter(row => row._tag === 'Faster') if (unusable.length) return `⚠️ **${unusable.length} benchmark${unusable.length === 1 ? '' : 's'} produced no usable reading.** The two sides disagreed on their output, or the case failed.` + if (changed.length) { + const head = changed.length === 1 + ? subject(changed[0]) + : `${changed.length} benchmarks moved past their threshold` + const why = changed.length === 1 + ? 'The manifest marks this count as suspect in both directions, so read the build before you grade the movement.' + : `The manifest marks these ${changed.length} counts as suspect in both directions, so read the build before you grade the movement.` + const rest = slower.length || faster.length + ? ` The same run marks ${slower.length} slower and ${faster.length} faster, past the noise this run measured.` + : '' + return `⚠️ **${head}. ${why}${rest}**` + } if (slower.length === 1 && faster.length === 0) return `🔴 **${subject(slower[0])}.**` if (faster.length === 1 && slower.length === 0) @@ -87,7 +106,7 @@ function renderTable(rows) { '|---|---:|---:|---:|---:|', ] for (const row of rows) { - const mark = row._tag === 'Slower' ? '🔴 ' : row._tag === 'Faster' ? '🟢 ' : '' + const mark = row._tag === 'Slower' ? '🔴 ' : row._tag === 'Faster' ? '🟢 ' : row._tag === 'Changed' ? '⚠️ ' : '' const change = row._tag === 'Unusable' ? '—' : `${mark}${formatPercent(row.deltaPercent)}` output.push(`| \`${escapeCell(row.id)}\` | ${formatValue(row, row.parent.min)} | ${formatValue(row, row.head.min)} | ${change} | ${formatPercent(row.controlPercent)} |`) } @@ -104,7 +123,7 @@ function renderHowToRead(rows) { '**Noise** is this run marking its own homework. It is the same build measured a second time against itself, so it should be zero and never is. A change counts only when it clears twice the noise, or 5%, whichever is larger.', '', counts > 0 - ? `A \`count\` benchmark carries no noise at all, so any movement there is real. It counts as a change only above the threshold \`perf/benchmarks.json\` declares for it, because most feature work grows the bundle a little. ${counts} of ${rows.length} here ${counts === 1 ? 'is' : 'are'} a count.` + ? `A \`count\` benchmark carries no noise at all, so any movement there is real. It counts as a change only above the threshold \`perf/benchmarks.json\` declares for it, because most feature work grows the bundle a little. A count marked \`direction: either\` is suspect in both directions, so ⚠️ flags it for a read. ${counts} of ${rows.length} here ${counts === 1 ? 'is' : 'are'} a count.` : 'Every benchmark here is timed, so every reading carries noise.', '', 'Values are the fastest of the repeats. The minimum leads, because noise only ever adds.', @@ -114,25 +133,35 @@ function renderHowToRead(rows) { } /** - * Reads each count Benchmark's `thresholdPercent` from the manifest. + * Reads each count Benchmark's rules from the manifest: the `thresholdPercent` + * a change must clear, and the `direction` when movement in both directions + * is suspect. A Measurement carries neither. The manifest of the revision + * under review decides, the same one whose cases took the samples. * - * A Measurement does not carry it. The manifest of the revision under review - * decides, the same one whose cases took the samples. + * A count without `direction: either` keeps the default grading, where a fall + * past the gate is an improvement and growth past it is a regression. */ export function readThresholds(manifest) { const thresholds = {} + const directions = {} for (const benchmark of manifest.benchmarks ?? []) { - if (benchmark.thresholdPercent === undefined) - continue - if (typeof benchmark.thresholdPercent !== 'number' || !(benchmark.thresholdPercent >= 0)) - throw new TypeError(`${benchmark.id}: thresholdPercent must be a number of 0 or more.`) - thresholds[benchmark.id] = benchmark.thresholdPercent + if (benchmark.thresholdPercent !== undefined) { + if (typeof benchmark.thresholdPercent !== 'number' || !(benchmark.thresholdPercent >= 0)) + throw new TypeError(`${benchmark.id}: thresholdPercent must be a number of 0 or more.`) + thresholds[benchmark.id] = benchmark.thresholdPercent + } + if (benchmark.direction !== undefined) { + if (benchmark.direction !== 'either') + throw new TypeError(`${benchmark.id}: direction must be "either", or left out for the default grading.`) + directions[benchmark.id] = benchmark.direction + } } - return thresholds + return { thresholds, directions } } -export function renderReport(measurement, baseLabel = '', thresholds = {}) { - const rows = (measurement.benchmarks ?? []).map(benchmark => classify(benchmark, thresholds)) +export function renderReport(measurement, baseLabel = '', rules = {}) { + const { thresholds = {}, directions = {} } = rules + const rows = (measurement.benchmarks ?? []).map(benchmark => classify(benchmark, { thresholds, directions })) const output = ['### ⚡ Perf', '', renderOutcome(rows), ''] if (rows.length) output.push(...renderTable(rows), '', ...renderHowToRead(rows)) diff --git a/test/unit/perf-report.test.ts b/test/unit/perf-report.test.ts index c5acad67..1d472470 100644 --- a/test/unit/perf-report.test.ts +++ b/test/unit/perf-report.test.ts @@ -27,6 +27,28 @@ function manifest(thresholdPercent?: unknown) { } } +function countMeasurement(id: string, unit: string, headMin: number, parentMin: number) { + return { + harness: 1, + benchmarks: [{ + id, + kind: 'count', + unit, + repeats: 1, + head: { min: headMin, median: headMin }, + parent: { min: parentMin, median: parentMin }, + control: { min: headMin, median: headMin }, + deltaPercent: (headMin - parentMin) / parentMin * 100, + controlPercent: 0, + verified: true, + }], + } +} + +function countManifest(benchmark: { id: string, thresholdPercent?: number, direction?: string }) { + return { benchmarks: [{ kind: 'count', ...benchmark }] } +} + describe('perf report thresholds', () => { it('calls count growth below its threshold no change, because feature work grows the bundle', () => { const report = renderReport(measurement(101_500, 100_000), '', readThresholds(manifest(2))) @@ -48,3 +70,28 @@ describe('perf report thresholds', () => { expect(() => readThresholds(manifest(-1))).toThrow('thresholdPercent') }) }) + +describe('perf report count direction', () => { + it('does not present a count fall past its threshold as an improvement', () => { + const rules = readThresholds(countManifest({ id: 'client/chunk-count', thresholdPercent: 5, direction: 'either' })) + const report = renderReport(countMeasurement('client/chunk-count', 'chunks', 78, 156), '', rules) + expect(report).not.toContain('🟢') + expect(report).toContain('⚠️ **`client/chunk-count` fell by 78 chunks (+50.00%)') + }) + + it('flags count growth the same way when the manifest marks both directions', () => { + const rules = readThresholds(countManifest({ id: 'client/chunk-count', thresholdPercent: 5, direction: 'either' })) + const report = renderReport(countMeasurement('client/chunk-count', 'chunks', 312, 156), '', rules) + expect(report).not.toContain('🔴') + expect(report).toContain('⚠️ **`client/chunk-count` grew by 156 chunks (+100.00%)') + }) + + it('still grades a byte fall past its threshold as an improvement', () => { + const report = renderReport(countMeasurement('client/js-bytes', 'bytes', 50_000, 100_000), '', readThresholds(manifest(2))) + expect(report).toContain('🟢 **`client/js-bytes` fell by') + }) + + it('refuses a direction the report does not know', () => { + expect(() => readThresholds(countManifest({ id: 'client/chunk-count', direction: 'up' }))).toThrow('direction') + }) +}) From 6a50c37a9031dd53b5182f82575a9d26ae94ac34 Mon Sep 17 00:00:00 2001 From: Harlan GitHub Agent Date: Wed, 23 Sep 2026 20:32:10 +1000 Subject: [PATCH 3/3] docs(perf): scope the stale-cache guard claim to js counts The retained-chunk guard reads deletedChunks, which nuxt-skew-protection records for JavaScript only. A warm local cache can still inflate css counts; CI's cold exports keep stored --- scripts/perf/cases/client-css-bytes.mjs | 4 ++++ scripts/perf/output.mjs | 11 ++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/scripts/perf/cases/client-css-bytes.mjs b/scripts/perf/cases/client-css-bytes.mjs index 66a610f3..93eecdca 100644 --- a/scripts/perf/cases/client-css-bytes.mjs +++ b/scripts/perf/cases/client-css-bytes.mjs @@ -1,4 +1,8 @@ import { bytes, clientFiles, countCase } from '../output.mjs' // Total bytes of the client CSS this build produced. +// +// Exact on a cold build, which is what CI exports before it measures. A warm +// local skew-protection cache restores old hashed CSS that no build metadata +// names, so a count read on a warm checkout can include earlier deploys. countCase(root => bytes(clientFiles(root).filter(path => path.endsWith('.css')))) diff --git a/scripts/perf/output.mjs b/scripts/perf/output.mjs index b01cd395..c53f70ca 100644 --- a/scripts/perf/output.mjs +++ b/scripts/perf/output.mjs @@ -37,12 +37,17 @@ export function serverFiles(root) { } /** - * Client chunks that this build produced. + * The client files this build produced. * * nuxt-skew-protection copies the chunks of earlier deploys into the output, * so old sessions still load. A build that restored that cache would count - * every earlier deploy as well. `latest.json` names those retained chunks, so - * they are left out here. + * every earlier deploy as well. `latest.json` names the retained JavaScript + * chunks, so those are left out here. The metadata records deleted `.js` + * only, and no metadata names the other restored asset types. A warm local + * cache can therefore leave stale `.css` files in the output. The + * non-JavaScript cases that read this list then count them. Stored + * Measurements stay exact: CI measures cold git-archive exports, so nothing + * is restored there. */ export function clientFiles(root) { const assets = join(root, BUILD_ASSETS)