feat(bench): gate on a bench smoke check, add local ratio history (#86 phase 3) - #170
Merged
Conversation
…phase 3) Closes #86. Closes #157. Phase 3 started as "run the benchmarks in CI", but working through what that would actually be good for changed the design. If runner numbers are the least trustworthy this project produces -- a VM with invisible neighbours, and the ~1.15x noise floor was measured locally, not there -- then they should not gate anything and are not worth keeping forever. What is left is not a measurement. So CI gets the thing that IS worth gating, and measurement stays manual. THE GATE (CI.yml, every push and PR): - `npm run bench:smoke` loads every bench file, runs its module-level and describe bodies, and skips the timed callbacks. It answers "do the benchmarks still work?", never "how fast are they" -- nothing is measured, so runner noise is irrelevant and it can safely fail the build. Verified it does: a planted throwing describe body exits 1, a healthy tree exits 0. Takes ~12s. It earns its place because bench/ was untouched by CI: npm test includes only tests/**. Every real breakage this suite has had lived in collection-time code -- undersized pools, a failed count assertion, a leaked Math.random stub, a comparator that made qsort not sort. - `npm run typecheck` uses a new tsconfig.check.json covering src/ AND bench/. tsconfig.json stays narrow because it drives vite-plugin-dts and the published types/ output, so widening it there risks leaking bench-only types into the public surface. That gap is #157, and it is how a genuine type error in bench/optical_flow_lk.bench.ts survived several PRs while tsc reported clean. tests/** is deliberately NOT included yet: it has 32 pre-existing type errors (mostly matrix_t vs Float32Array in the parity helpers). Fixing those is a separate job; wiring up the check should not be blocked on it. THE MEASUREMENT (bench.yml, dispatch only): Kept, but rescoped honestly: it exists to look at ratios without running them locally. Not a regression check, not the record. Never gates. A concurrency group stops two runs contending for the resource being measured. THE HISTORY (bench/history.jsonl): Append-only, one line per run, never rewritten. Ratios only -- bench/README.md forbids committing a baseline because it invites cross-machine comparison, but explicitly contemplates storing ratios. Written by local runs only (`npm run bench:ratios`); CI does not append, since its numbers are exactly the ones not worth keeping, and appending from CI would need write permissions on a dispatchable workflow. Design borrowed from OpenCV's cvbenchmark, which commits results per release with the CPU in the filename rather than pretending numbers transfer: - the environment is recorded alongside the numbers, because the A/B ratio cancels most machine differences but demonstrably not all -- yape06 measured ~1.3x on one checkout and ~1.04x on another; - cases store the SIGNED speedup (>1 means jsfeatNext is faster) so records aggregate without a separate direction field; - geomean summarises a run, geometric because these are ratios and 2x faster plus 2x slower must cancel to 1.0. Documented as a coarse indicator only: it can hide a per-case regression behind a healthy mean. scripts/bench-ratios.mjs reduces vitest's --outputJson to ratios. Written against the real JSON shape, matching "jsfeatNext" before "jsfeat" since the former also starts with the latter, and reporting rather than dropping a group that fails to pair up -- that would be a bug in the bench file. Always exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kalwalt
added
documentation
kalwalt
force-pushed
the
feat/86-bench-ci
branch
from
August 25, 2026 11:29
b0a8d19 to
1d096dd
Compare
4 tasks
kalwalt
added a commit
that referenced
this pull request
Aug 25, 2026
…arning The manual bench workflow (#170) surfaced this while dispatched post-0.14.0: (!) Your Vite config uses features that are unsupported by `configLoader: 'native'`, which is planned to become the default in a future major version of Vite: - ESM syntax in a file loaded as CommonJS (vitest.config.ts:1:1). package.json has no "type" field, so a plain .ts is ambiguous to the native config loader despite using ESM import/export syntax. vite.config.mts already sidesteps this the same way; vitest.config.ts was the one file left using the ambiguous extension. Verified: the warning is gone from a bench run, and npm test / bench:smoke / typecheck / format-check all still pass -- Vitest resolves the renamed config automatically, nothing referenced it by literal path except one comment in CI.yml, updated here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #86. Closes #157.
How the design changed while building it
Phase 3 started as "run the benchmarks in CI". Working through what that would actually be good for changed the shape: if runner numbers are the least trustworthy this project produces — a VM with invisible neighbours, and the ~1.15x noise floor was measured locally, not there — then they should not gate anything and are not worth keeping forever.
So CI gets the thing that is worth gating, and measurement stays where it is trustworthy.
1. The gate (
CI.yml, every push/PR)npm run bench:smoke— collection-only: loads every bench file, runs its module-level anddescribebodies, skips the timed callbacks. It answers "do the benchmarks still work?", never "how fast are they" — nothing is measured, so runner noise is irrelevant and it can safely fail the build. Verified empirically: a planted throwingdescribebody exits 1, a healthy tree exits 0. ~12s.It earns its place because
bench/was untouched by CI (npm testincludes onlytests/**), and every real breakage this suite has had lived in collection-time code: undersized pools, a failed count assertion, a leakedMath.randomstub, a comparator that madeqsortnot sort.npm run typecheck— a newtsconfig.check.jsoncoveringsrc/andbench/. This closes #157:tsconfig.jsonstays narrow because it drivesvite-plugin-dtsand the publishedtypes/output, so a plaintsc -p tsconfig.jsonreported clean while never looking at a bench file — which is how a genuine type error survived several PRs.tests/**is deliberately not included yet: widening the scope surfaced 32 pre-existing type errors there (mostlymatrix_tvsFloat32Arrayin the parity helpers). Fixing those is a separate job — I'd suggest a follow-up issue; wiring up the check should not be blocked on it.2. The measurement (
bench.yml, dispatch-only)Kept, but rescoped honestly: it exists to look at ratios without running them locally. Not a regression check, not the record. Never gates, cannot fail a build; a concurrency group stops two runs contending for the resource being measured. Results go to a 90-day artifact; the job summary carries the caveats (local noise floor doesn't transfer; never open a finding from one CI run).
3. The history (
bench/history.jsonl)The durable record #86 asked for ("chart trends across releases"). Append-only JSONL, one line per run, never rewritten. Ratios only —
bench/README.mdforbids committing a baseline of rawhzbut explicitly contemplates storing ratios.Design borrowed from OpenCV's cvbenchmark, which commits results per release with the CPU in the filename rather than pretending numbers transfer between machines:
yape06measured ~1.3x on one checkout, ~1.04x on another);casesstore the signed speedup (>1 = jsfeatNext faster) so records aggregate without a direction field;geomeansummarises a run — geometric, because 2x faster and 2x slower must cancel to 1.0. Documented as a coarse indicator only.Only local runs feed the file (
npm run bench:ratios). CI does not append: its numbers are exactly the ones not worth keeping, and appending from CI would need write permissions on a dispatchable workflow.Also
scripts/bench-ratios.mjs: reduces Vitest's--outputJsonto ratios; written against the real JSON shape; matchesjsfeatNextbeforejsfeat(prefix collision); reports rather than drops a group that fails to pair. Always exits 0.bench-results/gitignored; main README gains the Benchmarks badge + section;bench/README.mddocuments the gate/measurement/history split.Verification
npm test265 passed ·npm run typecheckclean (src+bench) ·bench:smokeexit 0 · prettier clean · license-check 93 files · both workflow YAMLs validated.Refs #86, #157.
🤖 Generated with Claude Code