diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 04fc7075..14d8b0bf 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -1,17 +1,24 @@ name: Bench -# CodSpeed simulation bench (the blocking perf regression gate), split out of +# Both CodSpeed instruments, in the order they must run. Split out of # pr-checks.yml so the workflow that talks to the self-hosted bench runner is # small and changes rarely. It reuses the dist artifacts built by the # "PR checks" run for the same commit (the `wait` step below), so nothing is # compiled twice. # -# Two jobs: +# Three jobs: # gate — hosted VM, metadata only (never checks out or executes PR code). # Decides whether the bench should run and what to bench, then waits # for the PR checks build artifacts. -# codspeed-bench — the bench itself on the self-hosted box (see the comments -# on the job). +# codspeed-bench — the simulation gate, the blocking perf regression check, +# on the self-hosted box (see the comments on the job). +# codspeed-walltime — the advisory real-time instrument, on a CodSpeed macro +# runner. It lives HERE, after codspeed-bench, rather than in +# pr-checks.yml beside it, because the CodSpeed app reports whichever +# instrument uploads FIRST. See the comment on that job. +# +# Only codspeed-bench runs on the shared nashua box. The macro-runner job is in +# this file for the `needs:` ordering, and touches nothing that box provides. # # PRs that modify CI-defining files (workflows, tools/ci/, root manifests) are # only benched pre-merge when they come from a branch in THIS repo; from a fork @@ -476,3 +483,178 @@ jobs: # under contention, so the first main run after merge is the new # baseline -- expect one round of large apparent deltas there. run: bash tools/ci/with-nashua-lock.sh pnpm --workspace-concurrency=1 $SCOPE_FLAGS run bench + + codspeed-walltime: + # Moved here from pr-checks.yml, where it ran BESIDE codspeed-bench rather + # than after it. That ordering decided which instrument GitHub reported, + # because the CodSpeed app computes its "CodSpeed Performance Analysis" + # check from the FIRST upload a commit produces and never re-evaluates it. + # Walltime finished 2-7 minutes earlier every time, so the ADVISORY + # instrument always won the race and the simulation gate never spoke: + # + # commit walltime check codspeed-bench verdict + # ab49563 18:36:45 18:37:16 18:39:29 failure -32.65% + # bac71dd 17:57:33 17:57:46 18:00:03 failure -19.77% + # 4ce83c9 17:52:55 17:53:20 17:56:29 failure + # 073884c 19:44:51 19:45:17 19:47:24 failure + # 5bfa7ff 21:27:58 21:28:22 21:34:56 success + # + # `continue-on-error: true` below does NOT prevent this: it sets this JOB's + # conclusion, while the CodSpeed app posts an independent check run that no + # job setting can mark advisory. Ordering is the only lever in the repo. + # + # It also mixed the two instruments within one series. On c9ffa62 the + # release commit cancelled this job (see pr-checks.yml's concurrency + # comment), simulation won the race by default, and the check compared + # bac71dd's walltime number against c9ffa62's simulation number: + # `JPEG XL Lossless (.110)` 158.5 ms -> 991.8 ms, a 6.3x ratio that sits + # inside the documented 5-15x simulation-inflation band for wasm decode. + # + # `needs: codspeed-bench` is what fixes both: simulation always uploads + # first, so it always decides the check, and this job is what it was + # documented to be. The cost is that a failed or cancelled bench takes + # walltime with it -- accepted, because an out-of-order walltime run is + # worse than a missing one. + needs: [gate, codspeed-bench] + # Deliberately NOT `always()`. Waiting for the bench is the entire point, + # and `always()` would let this job run when codspeed-bench was cancelled + # or skipped -- exactly the case that produced the 991.8 ms comparison. + # + # Gated behind a repository variable, as it was in pr-checks.yml: a + # `runs-on: codspeed-macro` job queues forever (up to 24h) when macro + # runners aren't provisioned for the org, and GitHub's job timeout only + # covers execution, not queue time. Enable macro runners for the org on + # app.codspeed.io first (the repo is public -- also make sure the runner + # group allows public repositories), then: + # gh variable set CODSPEED_MACRO_ENABLED --body true + # Turning this variable off leaves simulation as the ONLY instrument, which + # is a baseline re-seed event in both directions: the stored numbers change + # instrument, so expect one round of large apparent deltas either way. + if: needs.gate.outputs.ready == 'true' && vars.CODSPEED_MACRO_ENABLED == 'true' + # Advisory instrument: real wall-clock numbers (V8 JIT active, real + # cache/branch behavior) that complement the simulation gate above -- + # simulation catches small algorithmic slips deterministically, + # walltime keeps the numbers honest on real hardware and covers the + # pure-JS packages where the no-JIT simulation model is furthest from + # production. Failures here must not block the PR while this beds in. + continue-on-error: true + timeout-minutes: 30 + permissions: + contents: read + actions: read # cross-run artifact download from the PR checks run + pull-requests: write # CodSpeed action posts a sticky PR comment + id-token: write # OIDC token used by CodSpeedHQ/action for auth + # CodSpeed-managed 16-core ARM64 bare-metal machine, tuned for + # low-noise walltime measurement. Requires the CodSpeed GitHub app on + # an organization account. NOTE: ARM64 -- anything cached must be + # arch-qualified (see the cache key below). + # + # These runners are METERED, unlike the self-hosted bench box. Ordering via + # `needs:` rather than a poll inside this job is what keeps that cheap: + # GitHub allocates the runner when the job STARTS, so the wait for the + # bench costs nothing. A wait step here would hold a billed macro runner + # idle for the whole 7-12 minute bench. + runs-on: codspeed-macro + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + # Pinned for the same reason as the simulation job above: V8 changes + # between patch releases move the numbers, and walltime is if anything + # more sensitive than instruction counts. Keep this in step with the + # codspeed-bench pin so the two instruments stay comparable. + node-version: '24.20.0' + - name: Provide pnpm via Corepack + run: | + corepack enable pnpm + corepack prepare --activate + pnpm --version + - name: Download all built dists + uses: actions/download-artifact@v4 + with: + pattern: dist-* + path: tmp/ + # Cross-run download, new since this job moved out of pr-checks.yml: + # the artifacts live on the PR checks run the gate waited for, not on + # this run. Same three arguments codspeed-bench uses above. + run-id: ${{ needs.gate.outputs.run_id }} + github-token: ${{ github.token }} + - name: Replay dists into packages//dist + run: | + set -e + for d in tmp/dist-*; do + [ -d "$d" ] || continue + pkg=$(basename "$d" | sed 's/^dist-//') + mkdir -p "packages/$pkg/dist" + shopt -s dotglob nullglob + cp -r "$d"/* "packages/$pkg/dist/" 2>/dev/null || true + done + - name: Restore node_modules cache + id: modules-cache + uses: actions/cache@v4 + with: + path: | + node_modules + packages/*/node_modules + # runner.arch matters: this job runs on ARM64 while every other + # job is x64; sharing a key would restore x64 native binaries + # (esbuild/rollup) and break vitest. + # Manifests + workspace config in the key -- see pr-checks.yml's build + # job cache step for why the lockfile alone is not enough. + key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }} + - name: Install dependencies + if: steps.modules-cache.outputs.cache-hit != 'true' + run: pnpm install --frozen-lockfile + - name: Log CPU info + run: lscpu | grep -E "Model name|Cache|Flags" | head -5 || true + - name: Compute bench scope + # Reads the gate's scope, as codspeed-bench does, rather than + # detect-changes' `bench` output in the other workflow. The two lists + # are kept in sync by hand (see the gate job), so taking this one keeps + # both instruments measuring exactly the same package set. + id: scope + env: + BENCH: ${{ needs.gate.outputs.bench }} + run: | + set -euo pipefail + flags="" + for pkg in $(echo "$BENCH" | jq -r '.[]'); do + # Untrusted: fork PRs control this value (it comes from the PR's own + # packages//package.json). Validate against npm's name grammar + # before it reaches GITHUB_OUTPUT or any command line. Must be a + # whole-string check: keep it in node rather than a line-based tool. + # Keep in step with the same check in the codspeed-bench job above. + name=$(node -e ' + const pkg = process.argv[1]; + const { name } = require(`./packages/${pkg}/package.json`); + if (typeof name !== "string" || + !/^(@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/.test(name)) { + console.error(`::error::Rejected package name for ${pkg}`); + process.exit(1); + } + process.stdout.write(name); + ' "$pkg") + flags="$flags --filter $name" + done + echo "Bench scope flags:$flags" + echo "flags=$flags" >> "$GITHUB_OUTPUT" + - name: Run CodSpeed benchmarks (walltime) + # Walltime measures actual elapsed time, so parallel benchmark + # processes would contend for cores and add noise -- run packages + # sequentially (--workspace-concurrency=1). The simulation job above + # now does the same: it was left parallel on the premise that + # instruction counting is immune to contention, and #76 showed it is + # not. See the comment on that job's run step. + # + # No nashua lock here: this runs on a CodSpeed macro runner, not on the + # shared box, so there is nothing to serialise against. + uses: CodSpeedHQ/action@4e969336ab9acd4f6f8d025fdd793292b0835df0 # v4.18.2 + env: + # Keep this in env, NOT `${{ }}` in the run: below -- an env value is + # expanded by the shell after the command line is parsed, so it stays + # data. Unquoted below on purpose: the flags must word-split into + # repeated `--filter ` pairs. + SCOPE_FLAGS: ${{ steps.scope.outputs.flags }} + with: + mode: walltime + run: pnpm --workspace-concurrency=1 $SCOPE_FLAGS run bench diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 192a9adf..e2af0554 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -2,10 +2,10 @@ name: PR checks # Pipeline: builds run as a per-package parallel matrix (wasm compiles are # the slow part), then a single test job runs the whole vitest workspace -# against the built dists. The CodSpeed simulation bench lives in bench.yml, +# against the built dists. BOTH CodSpeed instruments now live in bench.yml, # which waits for THIS workflow's dist artifacts and benches the packages -# the PR touched; the advisory walltime bench stays here (codspeed-walltime -# below). +# the PR touched: the simulation gate first, then the advisory walltime job +# after it. # # Toolchain bumps (emsdk image tag below, root package.json/pnpm-lock.yaml, # this workflow itself) force the FULL pipeline including a full bench sweep. @@ -39,12 +39,49 @@ on: # dashboard (to seed initial perf data after the repo is connected). workflow_dispatch: -# Cancel in-flight runs when a new push lands on the same PR / branch. +# Cancel in-flight runs when a new push lands on the same PR. +# +# Keyed per-commit off a PR, the same way bench.yml is, and because of the same +# incident. On a push `head_ref` is empty, so `github.head_ref || github.ref` +# put every main push into the single group +# `pr-checks-PR checks-refs/heads/main` with cancel-in-progress: true. The +# release workflow's version commit -- pushed about five minutes after the +# merge that triggered it -- entered that group and CANCELLED the merge +# commit's run, then skipped its own jobs through the detect-changes guard +# below. GitHub applies concurrency when a run is QUEUED, before it evaluates +# any `if:`, so a run that goes on to do nothing still cancels its +# predecessor: +# +# 18:33 c9ffa62 fix(release): preflight the registry... cancelled +# 18:39 c44693e chore(release): publish skipped +# +# On c9ffa62 that killed `codspeed-walltime` at 18:39:11 with every other job +# already green. The cost is not one lost job: the CodSpeed check is computed +# from the FIRST upload a commit produces and is never re-evaluated, so +# cancelling walltime hands the check to the simulation bench instead. The two +# instruments differ by 5-15x on wasm decode (see BENCHMARKING.md), so +# c9ffa62 stored a simulation-sourced number for a benchmark whose predecessor +# had stored a walltime-sourced one, and the check reported `JPEG XL Lossless +# (.110)` as 158.5 ms -> 991.8 ms on a diff that touches only tools/release/ +# and workflows. +# +# c9ffa62 fixed this in bench.yml but not here, and the bench.yml fix is not +# sufficient on its own: bench.yml's gate waits on THIS workflow's build +# artifacts, so a run cancelled here starves the bench there. +# +# Keying non-PR runs by github.sha gives every main commit its own group, so no +# main push can cancel another. cancel-in-progress stays true for +# pull_request, where superseding an in-flight run with a newer push is +# exactly what is wanted. See bench.yml's concurrency comment for why +# per-commit keying is needed rather than cancel-in-progress: false alone -- a +# group holds one running run and at most one PENDING run, and queueing a +# third evicts the pending one whatever cancel-in-progress says. concurrency: - group: pr-checks-${{ github.workflow }}-${{ github.head_ref || github.ref }} - cancel-in-progress: true + group: pr-checks-${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} -# Job-level permissions where a job needs more (codspeed-walltime). +# Job-level permissions where a job needs more (detect-changes reads the PR's +# changed files). permissions: contents: read @@ -74,10 +111,12 @@ jobs: # bench — only the packages that actually changed. CodSpeed benches # run under valgrind (the slowest thing in CI), and a PR only needs # deltas for what it touched; main re-benches everything to keep - # full baselines. Consumed by codspeed-walltime below; the - # simulation bench in bench.yml computes the same scope from the - # PR's changed-file list — keep TOOLCHAIN_PATHS below in sync with - # the path lists in bench.yml's gate job. + # full baselines. No job in THIS workflow consumes it any more — + # both bench jobs live in bench.yml, whose gate job computes the + # same scope from the PR's changed-file list. It is kept because + # the two lists must agree: keep TOOLCHAIN_PATHS below in sync with + # the path lists in bench.yml's gate job, and this output is what + # makes a divergence visible in this run's own log. # # Bounded like every other job here: without an explicit timeout a job that # wedges runs to GitHub's 6-hour default. 13s typical, so 10 is ~45x @@ -482,120 +521,6 @@ jobs: - name: Browser smoke decode run: node tools/browser-smoke/run.js - codspeed-walltime: - needs: [detect-changes, build] - # Gated behind a repository variable: a `runs-on: codspeed-macro` job - # queues forever (up to 24h) when macro runners aren't provisioned for - # the org, and GitHub's job timeout only covers execution, not queue - # time. Enable macro runners for the org on app.codspeed.io first - # (the repo is public — also make sure the runner group allows public - # repositories), then: gh variable set CODSPEED_MACRO_ENABLED --body true - if: needs.detect-changes.outputs.any == 'true' && vars.CODSPEED_MACRO_ENABLED == 'true' - # Advisory instrument: real wall-clock numbers (V8 JIT active, real - # cache/branch behavior) that complement the simulation gate (bench.yml) — - # simulation catches small algorithmic slips deterministically, - # walltime keeps the numbers honest on real hardware and covers the - # pure-JS packages where the no-JIT simulation model is furthest from - # production. Failures here must not block the PR while this beds in. - continue-on-error: true - timeout-minutes: 30 - permissions: - contents: read - pull-requests: write # CodSpeed action posts a sticky PR comment - id-token: write # OIDC token used by CodSpeedHQ/action for auth - # CodSpeed-managed 16-core ARM64 bare-metal machine, tuned for - # low-noise walltime measurement. Requires the CodSpeed GitHub app on - # an organization account. NOTE: ARM64 — anything cached must be - # arch-qualified (see the cache key below). - runs-on: codspeed-macro - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - # Pinned for the same reason as the simulation job above: V8 changes - # between patch releases move the numbers, and walltime is if anything - # more sensitive than instruction counts. Keep this in step with the - # codspeed-bench pin so the two instruments stay comparable. - node-version: '24.20.0' - - name: Provide pnpm via Corepack - run: | - corepack enable pnpm - corepack prepare --activate - pnpm --version - - name: Download all built dists - uses: actions/download-artifact@v4 - with: - pattern: dist-* - path: tmp/ - - name: Replay dists into packages//dist - run: | - set -e - for d in tmp/dist-*; do - [ -d "$d" ] || continue - pkg=$(basename "$d" | sed 's/^dist-//') - mkdir -p "packages/$pkg/dist" - shopt -s dotglob nullglob - cp -r "$d"/* "packages/$pkg/dist/" 2>/dev/null || true - done - - name: Restore node_modules cache - id: modules-cache - uses: actions/cache@v4 - with: - path: | - node_modules - packages/*/node_modules - # runner.arch matters: this job runs on ARM64 while every other - # job is x64; sharing a key would restore x64 native binaries - # (esbuild/rollup) and break vitest. - # Manifests + workspace config in the key — see the build job's cache - # step for why the lockfile alone is not enough. - key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }} - - name: Install dependencies - if: steps.modules-cache.outputs.cache-hit != 'true' - run: pnpm install --frozen-lockfile - - name: Log CPU info - run: lscpu | grep -E "Model name|Cache|Flags" | head -5 || true - - name: Compute bench scope - id: scope - env: - BENCH: ${{ needs.detect-changes.outputs.bench }} - run: | - set -euo pipefail - flags="" - for pkg in $(echo "$BENCH" | jq -r '.[]'); do - # Untrusted: fork PRs control this value (it comes from the PR's own - # packages//package.json). Validate against npm's name grammar - # before it reaches GITHUB_OUTPUT or any command line. Must be a - # whole-string check: keep it in node rather than a line-based tool. - # Keep in step with the same check in bench.yml. - name=$(node -e ' - const pkg = process.argv[1]; - const { name } = require(`./packages/${pkg}/package.json`); - if (typeof name !== "string" || - !/^(@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/.test(name)) { - console.error(`::error::Rejected package name for ${pkg}`); - process.exit(1); - } - process.stdout.write(name); - ' "$pkg") - flags="$flags --filter $name" - done - echo "Bench scope flags:$flags" - echo "flags=$flags" >> "$GITHUB_OUTPUT" - - name: Run CodSpeed benchmarks (walltime) - # Walltime measures actual elapsed time, so parallel benchmark - # processes would contend for cores and add noise — run packages - # sequentially (--workspace-concurrency=1). The simulation job in - # bench.yml now does the same: it was left parallel on the premise that - # instruction counting is immune to contention, and #76 showed it is - # not. See the comment on that job's run step. - uses: CodSpeedHQ/action@4e969336ab9acd4f6f8d025fdd793292b0835df0 # v4.18.2 - env: - # Keep this in env, NOT `${{ }}` in the run: below — an env value is - # expanded by the shell after the command line is parsed, so it stays - # data. Unquoted below on purpose: the flags must word-split into - # repeated `--filter ` pairs. - SCOPE_FLAGS: ${{ steps.scope.outputs.flags }} - with: - mode: walltime - run: pnpm --workspace-concurrency=1 $SCOPE_FLAGS run bench + # codspeed-walltime lived here until it was moved to bench.yml, so that it + # runs AFTER the simulation bench rather than beside it. See that job's + # comment for why the ordering decides which instrument GitHub reports. diff --git a/BENCHMARKING.md b/BENCHMARKING.md index 6753dd6f..e418ae2d 100644 --- a/BENCHMARKING.md +++ b/BENCHMARKING.md @@ -5,9 +5,9 @@ what the numbers mean, why they don't match real wall-clock time, and how to read warnings from the CodSpeed dashboard. Bench files live under `packages/*/bench/*.bench.js` and are driven by -`vitest bench` + `@codspeed/vitest-plugin@^5`. The full pipeline is in -`.github/workflows/pr-checks.yml` (jobs: `codspeed-bench` and -`codspeed-walltime`). +`vitest bench` + `@codspeed/vitest-plugin@^5`. Both bench jobs are in +`.github/workflows/bench.yml` (`codspeed-bench`, then `codspeed-walltime`); +`.github/workflows/pr-checks.yml` builds the dists they measure. ## TL;DR @@ -48,11 +48,29 @@ bare-metal machines. It covers simulation's two blind spots: real-time effects (branch prediction, actual caches) that instruction counting models away, and the pure-JS packages (`little-endian`/`big-endian`) where the no-JIT simulation model is furthest from production V8. -Walltime benches run packages sequentially (`--concurrency 1`) because -parallel processes contend for cores and add noise; simulation is immune -to contention so it keeps `--parallel`. Note the macro runners are ARM64: -walltime numbers are real milliseconds, but on different silicon than -most x86 production traffic. +Note the macro runners are ARM64: walltime numbers are real milliseconds, +but on different silicon than most x86 production traffic. + +**Both jobs run packages sequentially** (`--workspace-concurrency=1`). +Walltime measures elapsed time, so parallel processes contend for cores +and add noise. Simulation was left parallel on the premise that +instruction counting is immune to contention, and #76 disproved that: a +charls bench was reported as a 19.8 ms → 37.9 ms regression on a commit +whose only change was one vitest file. See the comment on that job's +`run:` step in `bench.yml`. + +**Ordering matters, and it is why both jobs live in `bench.yml`.** The +CodSpeed app computes its single `CodSpeed Performance Analysis` check +from the **first** upload a commit produces, and never re-evaluates it. +While walltime ran beside the bench in `pr-checks.yml` it finished 2–7 +minutes earlier every time, so the advisory instrument decided the check +on every commit and the gate never spoke. `continue-on-error: true` does +not help: it sets the job's conclusion, while the app posts an +independent check run that no job setting can mark advisory. +`codspeed-walltime` therefore `needs: codspeed-bench`, which also stops +the two instruments landing in one series — they differ by 5–15x on wasm +decode, so a commit measured by one and its predecessor by the other +produces a meaningless delta (see the next section). ## How the numbers get inflated @@ -168,6 +186,25 @@ its own — see the comment on `concurrency:` in [.github/workflows/bench.yml](.github/workflows/bench.yml). Merges of #70 and #73 produced no baseline at all. +There was a second cause, fixed at the same time as this note was written. +`pr-checks.yml` kept a branch-level push concurrency group after `bench.yml` +got a per-commit one, so the release commit still cancelled the merge +commit's **`pr-checks`** run — which is where `codspeed-walltime` used to +live. On `c9ffa62` that killed walltime with every other job already green, +simulation won the first-upload race by default, and the check compared +`bac71dd`'s walltime number against `c9ffa62`'s simulation number: +`JPEG XL Lossless (.110)` 158.5 ms → 991.8 ms. That 6.3x ratio is not a +regression, it is the 5–15x simulation-inflation band in the table above. +Two symptoms identify this case: the ratio sits near that band, and the +commit's `codspeed-walltime` job is `cancelled` rather than `success`. + +Drift also plays a part that a fresh baseline does not remove. `JPEG XL +Lossless colour (.110)` read 182.1 ms, then 193.4 ms, then 298.5 ms across +three commits whose `libjxl` was byte-identical, with a good walltime +baseline on both sides of each comparison. That is well outside the 1–3% +figure in the table, so treat walltime deltas on the `dicom-codec` dispatch +benches as advisory even when the baseline is sound. + **What to do**: confirm the diff cannot affect the measured path, then let the next successful **push to main** re-seed the baseline for all live benchmarks — that happens on its own and needs no dashboard access. diff --git a/packages/charls/extern/charls b/packages/charls/extern/charls index 38d95d00..9930a2a2 160000 --- a/packages/charls/extern/charls +++ b/packages/charls/extern/charls @@ -1 +1 @@ -Subproject commit 38d95d00671f4cddfa61f3f51eaf81b8bac34543 +Subproject commit 9930a2a2fa75f516c4a08708180c9907fa501a97