test(perf): add a benchmark harness for the metric walk#1085
Merged
Conversation
Adds `big-code-analysis-bench`, a workspace member (not a default member, like `xtask`) with two bench targets. `--bench scaling` is a complexity-class gate. Eight probes pair a generated source shape with the metric selection that exercises one hot path; each is measured at three doubling nesting depths with the cells interleaved and rotated between rounds, and the reported figure is the slope of ln(time) against ln(depth). A probe fails when that exponent leaves the class it declares. Asserting the class rather than a duration is what makes the guard portable: the wall-clock assertion it replaces produced false failures on windows-latest, under llvm-cov, and twice on a loaded local host. `--bench metric_walk` runs criterion per metric over a deterministic slice of the corpus submodules. The slice prints its own composition before measuring, since a published number from the #1052 / #1062 work described a corpus it had not analysed. The wall-clock halves of `cognitive_deep_nesting_is_tractable` and `tokens_deep_nesting_is_tractable` move into the gate and the `BCA_ASSERT_SCALING` escape hatch is deleted. Both tests keep their value assertions and are renamed to say what they now cover. A per-walk time budget in the harness preserves the property those budgets existed for: a reintroduced quadratic walk is abandoned at its shallowest depth and reported, instead of running for hours. Out of band by design. `.github/workflows/benchmark.yml` runs quarterly and re-measures before filing an issue; `make bench-scaling` is the local entry point. The first run measured three walks as quadratic in nesting depth, all through `Node::parent` — filed as #1084 and pinned under a quadratic bound so a further degradation is still caught. The checkmake cap moves 100 -> 120: the `help` body had reached exactly 100, spending the headroom that file's own comment promises, so the next public target of any kind would have failed the gate. Fixes #1068
`run` pushed a cell into `pending` and only then compared its walk against `MAX_CELL_WALK`, so the cell the budget had just rejected was still walked once per round. A reintroduced quadratic walk — over two minutes at depth 2000 before #1052 — would have cost 13 or 21 rounds of that under the scheduled workflow and tripped the 90-minute timeout, which is the outcome the budget's own doc comment says it prevents. The check now precedes the push. `run` delegates to `run_with_budget(probes, rounds, max_cell_walk)` so the abandon path is reachable from a test: a zero budget abandons every probe at its shallowest depth in milliseconds, where exercising the real twenty seconds would need a walk that genuinely takes them. The new test fails against the previous ordering. Also in this pass: - `--rounds` rejects even values. `USAGE` said "must be odd" and `DEFAULT_ROUNDS` carries a compile-time parity assert, but the parser only rejected zero, so `--rounds 10` silently made every median an average of two samples. - The gate reports an abandoned probe by the budget it blew rather than by its exponent. That exponent is fitted over the cells that finished, so it reads as a pass ("0.00 > 1.50") for the worst regression the gate can see. - `bench_corpus` bails when `parse_slice` retains nothing. The slice selects by file extension, so a build without those languages compiled in would have timed empty loops and published them as walk numbers. - `Report::failures` and `SHAPE_BENCH_DEPTH` doc comments corrected; the latter is the shallowest linear depth, not the middle one. - AGENTS.md gains the crate-table row and a Benchmarking paragraph beside the mutation-testing one, so the workflow is discoverable from the file agents actually read.
Lesson 80 covers assertions that rot because they only fire on a rare trigger. #1068 did the opposite deliberately: it moved a wall-clock assertion out of the per-PR suite onto a quarterly bench, because a shared runner cannot produce an honest wall clock and the assertion had already redded CI four times on measurement artefacts. The sub-example records the three conditions that made that acceptable rather than a rot vector — every mechanical part of the guard mirrored per-PR, only the timing verdict left on the rare trigger, and a fail-fast budget so the rare gate reports instead of hanging — and the trap the first cut hit twice: measurement apparatus degrades toward a flattering number, so a truncated or empty measurement reads as an excellent one unless each partial path carries its own verdict. The closing paragraph gains the matching qualification; no lesson is renumbered.
`collect_candidates` recursed on `Path::is_dir`, which follows symlinks, so an aliased subtree was walked twice and every file beneath it entered the candidate list under two paths. The corpus ships `DeepSpeech/tensorflow/native_client -> ../native_client`, and it duplicated the whole Java bucket: `summary()` reported sixteen files where there were eight, each of them benched twice — in the one method whose entire purpose is reporting what was actually analysed. Directories are now deduplicated by canonical path through a set shared across roots, which also bounds the recursion: a symlink cycle previously descended until the stack overflowed, and a stack overflow aborts rather than unwinds. Both are covered by unix-gated fixture tests that fail against the previous walker. The real slice moves from 182 files / 886 KiB to 177 / 903 (Java's freed slots refill with distinct, larger files). Also from the same review pass: - An abandoned probe now contributes no cells at all, not merely no deeper ones. `passed()` ignores the exponent once `over_budget` is set, so the shallower cells changed no verdict while costing `rounds + WARMUP_ROUNDS` walks of an input already known to be pathological — about sixteen minutes for a pre-#1052-magnitude regression under the scheduled workflow. Cells now accumulate into a local vec that reaches the schedule only if the probe finished, so the invariant is structural rather than a cleanup to remember. The `MAX_CELL_WALK` and `LINEAR_DEPTHS` comments claimed a fail-fast behaviour that did not hold; both are corrected against the magnitudes on record. - The report header no longer prints an exponent for an abandoned probe. It is fitted over the cells that finished — none — so the line read `exponent 0.00 (bound 1.50) OVER BOUND`, a passing number beside a failing verdict for the worst regression the gate can see. - The bench target exits 2 for "the harness could not run" and 1 for "a probe left its complexity class", and benchmark.yml branches on the two. Previously any infrastructure error that reproduced on the retry filed an issue claiming a complexity regression. The status capture uses `|| status=$?` because `$?` after an `if` block whose condition failed is the if statement's own 0, not the pipeline's. - `bench_corpus` reports a partial drop, not just an empty one: the composition is printed from the slice while the numbers come from the parsed subset, and those diverge under a reduced feature set. - `CorpusFile::path` is absolute, not repo-relative; a stale doc line left over from a moved const no longer heads the wrong test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1085 +/- ##
=======================================
Coverage 98.06% 98.06%
=======================================
Files 268 268
Lines 66613 66586 -27
Branches 66183 66156 -27
=======================================
- Hits 65321 65299 -22
+ Misses 851 846 -5
Partials 441 441
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dekobon
added a commit
that referenced
this pull request
Jul 26, 2026
PR #1085 merged by rebase, which rewrote every commit hash on the branch. Lesson 80's sub-example was written before that merge and cited bfad0b9 / 2bca5d2, neither of which is reachable from main. The equivalents are 0eeb6fe and 93c23cd — verified by patch-id, not by subject line. This is the second time in one branch: 5f3d790 corrected lesson 81 for citing an orphaned pre-amend copy of the same #1056 commit. A lessons entry written before its own PR lands cites hashes that any rebase or squash merge invalidates, so the citation has to be re-checked after the merge, not at authoring time.
dekobon
added a commit
that referenced
this pull request
Jul 26, 2026
Issue and PR numbers are assigned once and never change. A commit hash is not stable until the change reaches the default branch, and this repository rebase-merges, so every hash on a branch is rewritten when the PR lands. A lesson drafted alongside the change it describes — the normal case, because that is when it is fresh — therefore cites hashes that are orphaned the moment it merges, and nothing detects it: the text still renders and the hash still looks like a hash. That happened twice during PR #1085 and both were caught by chance. Lesson 81 cited an orphaned pre-amend copy of the #1056 commit; lesson 80's sub-example, written in that same PR, cited two branch hashes its own rebase merge rewrote. The lessons-learned skill now records a PR rather than a hash at every citation site (evidence capture, the candidate template, the entry format, and the evidence-trail guardrail), and gives the `gh api .../commits/<sha>/pulls` lookup to resolve one from the other. Existing entries keep their hashes: retrofitting eighty-odd lessons would be churn, and the skill says so explicitly so a future run does not read this as licence to rewrite them. It also carves out the one case where a hash is still right — a commit already on main with no PR behind it — and asks that reachability be verified first. The lessons file header is updated to match; it was the same guidance stated in a second place.
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
1 participant
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.
Adds
big-code-analysis-bench, the benchmark harness #1068 asked for,and moves the wall-clock assertions out of the unit suite into it.
Fixes #1068
Why
Every performance claim this project has made lived only in a commit
message. The absence of a harness caused concrete problems in each of
the four #1052 / #1062 perf commits: a corpus misdescribed, a "~26%"
improvement that re-measured to a 7-41% interval, two measurements
invalidated by generator indentation, and a wall-clock assertion that
produced a false failure in four environments — including under
cargo llvm-cov, whosecoveragejob runs in CI.What landed
A workspace member (member, not default member — the
xtasktreatment, so
cargo buildat the root does not pull criterion) withtwo bench targets. Neither runs in per-PR CI.
--bench scalingis a complexity-class gate. Eight probes pair agenerated shape with the metric selection that reaches one hot path,
measure it at three doubling depths, and fit
time ~ depth^k. A probefails when
kleaves the class it declares. The timed region isAst::metricson a pre-parsed tree, so parse cost is excluded.--bench metric_walkis criterion over a deterministic corpusslice, one benchmark per metric, printing its own composition before
measuring.
make bench-scaling/make bench-walk/make benchare the localentry points;
.github/workflows/benchmark.ymlruns the gate quarterlyand re-measures before filing an issue.
Against the four measurement traps
bytes per nesting level, and
byte_growth_is_affinefails if onestops doing so.
CorpusSlice::summaryprints file count,byte total, per-language breakdown, and an explicit note when a
ceiling truncated a language.
rotated per round, three points are fitted rather than two, and
sub-millisecond cells repeat inside one timed sample.
nom/nested-whileis a metric control (Cognitivedeclares Nom);
cognitive/nested-whileandloc/nested-whileareshape controls for the quadratic probes beside them.
What the first run found
Three walks measure quadratic in nesting depth, all through
Node::parent(tree-sitter stores no parent pointer, so the lookupdescends from the root):
cognitive/nested-ifChecker::is_else_ifloc/nested-declarationNode::count_specific_ancestorsnom/nested-quoteelixir_is_inside_quote_blockis_else_ifwas known. The other two were not — the shape controlsisolate them (
loc/nested-whilefits 0.99 on identical nesting withouta
declaration). Filed as #1084; the probes are pinned under aquadratic bound so further degradation still fails.
On removing the per-PR timing guard
BCA_ASSERT_SCALINGis gone and both tests keep their valueassertions, renamed to
cognitive_nesting_is_inherited_at_depthandtokens_count_holds_at_depth. Your original comments were right that avalue assertion alone lets a reintroduced quadratic walk hang CI rather
than fail it, so the harness carries a per-walk budget: cells build in
ascending depth order and a probe whose walk exceeds
MAX_CELL_WALKisabandoned, contributing no cells at all. A pre-#1052-magnitude
regression is reported in a couple of minutes instead of running out
the job timeout.
Everything mechanical about the guard stays per-PR as ordinary unit
tests — shapes affine in bytes, shapes parse, shapes nest, each probe's
metric reads non-zero, the fit recovers 1.0/2.0/0.0 on synthetic data,
argument parsing. Only the timing verdict, the one thing a shared
runner cannot supply honestly, sits on the rare trigger. That tension
with lesson 80 is recorded as a sub-example on that lesson.
Review notes
Two review passes ran on this branch. The findings were all one shape —
measurement apparatus degrades toward a flattering number — and are
fixed in
2bca5d27andd1d1de93:DeepSpeech/tensorflow/native_client -> ../native_clientmade theJava bucket 8 distinct files counted twice while
summary()reportedsixteen. Directories are now deduplicated by canonical path, which
also bounds the walk against a symlink cycle.
rounds + WARMUP_ROUNDSwalks that changed no verdict.exponent 0.00 (bound 1.50) OVER BOUNDforan abandoned probe.
its class", so an infrastructure error can no longer file an issue
claiming a complexity regression.
Incidental, and worth a look
.checkmake.ini's cap moves 100 → 120. Thehelpbody had alreadyreached exactly 100, so the ≥15-line headroom that file's own comment
promises was spent before this branch touched it.
big-code-analysis-benchis added to.bcaignore, matching./xtask/**. That means the bench crate is not measured by theproject's own gate — deliberate, but say so if you would rather it
were scanned.
7b47aa28is an unrelated one-line fix: lesson 81 cited3fd01c70,an orphaned pre-amend copy of the fix(wire): recursive Serialize/Drop on FuncSpace lack the #700 de-recursion #1056 commit, rather than
93547880which is reachable from main.Validation
make pre-commitgreen. Gate re-run end to end: all 8 probes withinbound. The two symlink regression tests were verified by reverting the
dedup and confirming they fail.