Skip to content

Commit 70030e0

Browse files
sedghijboccewayfarer3130claude
authored
chore(charls): update CharLS submodule to upstream 2.4.4 (#77)
Had a previous approval by jbocce on the main fix, the additional work was a publish fix/race conditon on release that needs a build test. * chore(charls): update extern/charls submodule to upstream CharLS 2.4.4 Advances from 38d95d0 (~2.4.1-era, Jan 2023) to upstream 2.4.4 (2026-06-08). No custom fork patches (clean version advance). Fork PR: cornerstonejs/charls#1. CI is the first build/validation of 2.4.4 against our glue. * ci: run the walltime bench after the simulation gate; give pr-checks a per-commit push group The CodSpeed app computes its single "CodSpeed Performance Analysis" check from the FIRST upload a commit produces, and never re-evaluates it. While codspeed-walltime ran beside codspeed-bench in pr-checks.yml it finished 2-7 minutes earlier on every commit measured, so the advisory instrument decided the check 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 does not prevent this. It sets that JOB's conclusion, while the app posts an independent check run that no job setting can mark advisory. Ordering is the only lever available in the repo, so codspeed-walltime moves to bench.yml with needs: codspeed-bench. Ordering via needs: rather than a poll inside the job also keeps the metered macro runner unallocated while it waits. pr-checks.yml kept a branch-level push concurrency group after #93 gave bench.yml a per-commit one. On a push head_ref is empty, so every main push shared one group with cancel-in-progress: true, and the release workflow's version commit cancelled the merge commit's run before being skipped itself by the detect-changes guard -- GitHub applies concurrency when a run is queued, before it evaluates any if:. 18:33 c9ffa62 fix(release): preflight the registry... cancelled 18:39 c44693e chore(release): publish skipped The two faults compounded. Cancelling pr-checks killed walltime, so 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) as 158.5 ms -> 991.8 ms. That 6.3x ratio is the documented 5-15x simulation-inflation band for wasm decode, not a regression. BENCHMARKING.md records both faults, corrects the job locations, and drops the stale claim that simulation "keeps --parallel" (#89 serialised it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Joe Boccanfuso <joe.boccanfuso@radicalimaging.com> Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c44693e commit 70030e0

4 files changed

Lines changed: 285 additions & 141 deletions

File tree

‎.github/workflows/bench.yml‎

Lines changed: 186 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,24 @@
11
name: Bench
22

3-
# CodSpeed simulation bench (the blocking perf regression gate), split out of
3+
# Both CodSpeed instruments, in the order they must run. Split out of
44
# pr-checks.yml so the workflow that talks to the self-hosted bench runner is
55
# small and changes rarely. It reuses the dist artifacts built by the
66
# "PR checks" run for the same commit (the `wait` step below), so nothing is
77
# compiled twice.
88
#
9-
# Two jobs:
9+
# Three jobs:
1010
# gate — hosted VM, metadata only (never checks out or executes PR code).
1111
# Decides whether the bench should run and what to bench, then waits
1212
# for the PR checks build artifacts.
13-
# codspeed-bench — the bench itself on the self-hosted box (see the comments
14-
# on the job).
13+
# codspeed-bench — the simulation gate, the blocking perf regression check,
14+
# on the self-hosted box (see the comments on the job).
15+
# codspeed-walltime — the advisory real-time instrument, on a CodSpeed macro
16+
# runner. It lives HERE, after codspeed-bench, rather than in
17+
# pr-checks.yml beside it, because the CodSpeed app reports whichever
18+
# instrument uploads FIRST. See the comment on that job.
19+
#
20+
# Only codspeed-bench runs on the shared nashua box. The macro-runner job is in
21+
# this file for the `needs:` ordering, and touches nothing that box provides.
1522
#
1623
# PRs that modify CI-defining files (workflows, tools/ci/, root manifests) are
1724
# only benched pre-merge when they come from a branch in THIS repo; from a fork
@@ -476,3 +483,178 @@ jobs:
476483
# under contention, so the first main run after merge is the new
477484
# baseline -- expect one round of large apparent deltas there.
478485
run: bash tools/ci/with-nashua-lock.sh pnpm --workspace-concurrency=1 $SCOPE_FLAGS run bench
486+
487+
codspeed-walltime:
488+
# Moved here from pr-checks.yml, where it ran BESIDE codspeed-bench rather
489+
# than after it. That ordering decided which instrument GitHub reported,
490+
# because the CodSpeed app computes its "CodSpeed Performance Analysis"
491+
# check from the FIRST upload a commit produces and never re-evaluates it.
492+
# Walltime finished 2-7 minutes earlier every time, so the ADVISORY
493+
# instrument always won the race and the simulation gate never spoke:
494+
#
495+
# commit walltime check codspeed-bench verdict
496+
# ab49563 18:36:45 18:37:16 18:39:29 failure -32.65%
497+
# bac71dd 17:57:33 17:57:46 18:00:03 failure -19.77%
498+
# 4ce83c9 17:52:55 17:53:20 17:56:29 failure
499+
# 073884c 19:44:51 19:45:17 19:47:24 failure
500+
# 5bfa7ff 21:27:58 21:28:22 21:34:56 success
501+
#
502+
# `continue-on-error: true` below does NOT prevent this: it sets this JOB's
503+
# conclusion, while the CodSpeed app posts an independent check run that no
504+
# job setting can mark advisory. Ordering is the only lever in the repo.
505+
#
506+
# It also mixed the two instruments within one series. On c9ffa62 the
507+
# release commit cancelled this job (see pr-checks.yml's concurrency
508+
# comment), simulation won the race by default, and the check compared
509+
# bac71dd's walltime number against c9ffa62's simulation number:
510+
# `JPEG XL Lossless (.110)` 158.5 ms -> 991.8 ms, a 6.3x ratio that sits
511+
# inside the documented 5-15x simulation-inflation band for wasm decode.
512+
#
513+
# `needs: codspeed-bench` is what fixes both: simulation always uploads
514+
# first, so it always decides the check, and this job is what it was
515+
# documented to be. The cost is that a failed or cancelled bench takes
516+
# walltime with it -- accepted, because an out-of-order walltime run is
517+
# worse than a missing one.
518+
needs: [gate, codspeed-bench]
519+
# Deliberately NOT `always()`. Waiting for the bench is the entire point,
520+
# and `always()` would let this job run when codspeed-bench was cancelled
521+
# or skipped -- exactly the case that produced the 991.8 ms comparison.
522+
#
523+
# Gated behind a repository variable, as it was in pr-checks.yml: a
524+
# `runs-on: codspeed-macro` job queues forever (up to 24h) when macro
525+
# runners aren't provisioned for the org, and GitHub's job timeout only
526+
# covers execution, not queue time. Enable macro runners for the org on
527+
# app.codspeed.io first (the repo is public -- also make sure the runner
528+
# group allows public repositories), then:
529+
# gh variable set CODSPEED_MACRO_ENABLED --body true
530+
# Turning this variable off leaves simulation as the ONLY instrument, which
531+
# is a baseline re-seed event in both directions: the stored numbers change
532+
# instrument, so expect one round of large apparent deltas either way.
533+
if: needs.gate.outputs.ready == 'true' && vars.CODSPEED_MACRO_ENABLED == 'true'
534+
# Advisory instrument: real wall-clock numbers (V8 JIT active, real
535+
# cache/branch behavior) that complement the simulation gate above --
536+
# simulation catches small algorithmic slips deterministically,
537+
# walltime keeps the numbers honest on real hardware and covers the
538+
# pure-JS packages where the no-JIT simulation model is furthest from
539+
# production. Failures here must not block the PR while this beds in.
540+
continue-on-error: true
541+
timeout-minutes: 30
542+
permissions:
543+
contents: read
544+
actions: read # cross-run artifact download from the PR checks run
545+
pull-requests: write # CodSpeed action posts a sticky PR comment
546+
id-token: write # OIDC token used by CodSpeedHQ/action for auth
547+
# CodSpeed-managed 16-core ARM64 bare-metal machine, tuned for
548+
# low-noise walltime measurement. Requires the CodSpeed GitHub app on
549+
# an organization account. NOTE: ARM64 -- anything cached must be
550+
# arch-qualified (see the cache key below).
551+
#
552+
# These runners are METERED, unlike the self-hosted bench box. Ordering via
553+
# `needs:` rather than a poll inside this job is what keeps that cheap:
554+
# GitHub allocates the runner when the job STARTS, so the wait for the
555+
# bench costs nothing. A wait step here would hold a billed macro runner
556+
# idle for the whole 7-12 minute bench.
557+
runs-on: codspeed-macro
558+
steps:
559+
- uses: actions/checkout@v4
560+
- uses: actions/setup-node@v4
561+
with:
562+
# Pinned for the same reason as the simulation job above: V8 changes
563+
# between patch releases move the numbers, and walltime is if anything
564+
# more sensitive than instruction counts. Keep this in step with the
565+
# codspeed-bench pin so the two instruments stay comparable.
566+
node-version: '24.20.0'
567+
- name: Provide pnpm via Corepack
568+
run: |
569+
corepack enable pnpm
570+
corepack prepare --activate
571+
pnpm --version
572+
- name: Download all built dists
573+
uses: actions/download-artifact@v4
574+
with:
575+
pattern: dist-*
576+
path: tmp/
577+
# Cross-run download, new since this job moved out of pr-checks.yml:
578+
# the artifacts live on the PR checks run the gate waited for, not on
579+
# this run. Same three arguments codspeed-bench uses above.
580+
run-id: ${{ needs.gate.outputs.run_id }}
581+
github-token: ${{ github.token }}
582+
- name: Replay dists into packages/<pkg>/dist
583+
run: |
584+
set -e
585+
for d in tmp/dist-*; do
586+
[ -d "$d" ] || continue
587+
pkg=$(basename "$d" | sed 's/^dist-//')
588+
mkdir -p "packages/$pkg/dist"
589+
shopt -s dotglob nullglob
590+
cp -r "$d"/* "packages/$pkg/dist/" 2>/dev/null || true
591+
done
592+
- name: Restore node_modules cache
593+
id: modules-cache
594+
uses: actions/cache@v4
595+
with:
596+
path: |
597+
node_modules
598+
packages/*/node_modules
599+
# runner.arch matters: this job runs on ARM64 while every other
600+
# job is x64; sharing a key would restore x64 native binaries
601+
# (esbuild/rollup) and break vitest.
602+
# Manifests + workspace config in the key -- see pr-checks.yml's build
603+
# job cache step for why the lockfile alone is not enough.
604+
key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
605+
- name: Install dependencies
606+
if: steps.modules-cache.outputs.cache-hit != 'true'
607+
run: pnpm install --frozen-lockfile
608+
- name: Log CPU info
609+
run: lscpu | grep -E "Model name|Cache|Flags" | head -5 || true
610+
- name: Compute bench scope
611+
# Reads the gate's scope, as codspeed-bench does, rather than
612+
# detect-changes' `bench` output in the other workflow. The two lists
613+
# are kept in sync by hand (see the gate job), so taking this one keeps
614+
# both instruments measuring exactly the same package set.
615+
id: scope
616+
env:
617+
BENCH: ${{ needs.gate.outputs.bench }}
618+
run: |
619+
set -euo pipefail
620+
flags=""
621+
for pkg in $(echo "$BENCH" | jq -r '.[]'); do
622+
# Untrusted: fork PRs control this value (it comes from the PR's own
623+
# packages/<pkg>/package.json). Validate against npm's name grammar
624+
# before it reaches GITHUB_OUTPUT or any command line. Must be a
625+
# whole-string check: keep it in node rather than a line-based tool.
626+
# Keep in step with the same check in the codspeed-bench job above.
627+
name=$(node -e '
628+
const pkg = process.argv[1];
629+
const { name } = require(`./packages/${pkg}/package.json`);
630+
if (typeof name !== "string" ||
631+
!/^(@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/.test(name)) {
632+
console.error(`::error::Rejected package name for ${pkg}`);
633+
process.exit(1);
634+
}
635+
process.stdout.write(name);
636+
' "$pkg")
637+
flags="$flags --filter $name"
638+
done
639+
echo "Bench scope flags:$flags"
640+
echo "flags=$flags" >> "$GITHUB_OUTPUT"
641+
- name: Run CodSpeed benchmarks (walltime)
642+
# Walltime measures actual elapsed time, so parallel benchmark
643+
# processes would contend for cores and add noise -- run packages
644+
# sequentially (--workspace-concurrency=1). The simulation job above
645+
# now does the same: it was left parallel on the premise that
646+
# instruction counting is immune to contention, and #76 showed it is
647+
# not. See the comment on that job's run step.
648+
#
649+
# No nashua lock here: this runs on a CodSpeed macro runner, not on the
650+
# shared box, so there is nothing to serialise against.
651+
uses: CodSpeedHQ/action@4e969336ab9acd4f6f8d025fdd793292b0835df0 # v4.18.2
652+
env:
653+
# Keep this in env, NOT `${{ }}` in the run: below -- an env value is
654+
# expanded by the shell after the command line is parsed, so it stays
655+
# data. Unquoted below on purpose: the flags must word-split into
656+
# repeated `--filter <name>` pairs.
657+
SCOPE_FLAGS: ${{ steps.scope.outputs.flags }}
658+
with:
659+
mode: walltime
660+
run: pnpm --workspace-concurrency=1 $SCOPE_FLAGS run bench

0 commit comments

Comments
 (0)