fix: test-performance batch (#1105, #1120, #1121, #1123) - #1128
Merged
Conversation
Cargo built every dependency at opt-level 0 under `cargo test`, including the tree-sitter runtime and the ~24 grammar C libraries: `cc-rs` forwards Cargo's OPT_LEVEL for the package being built straight through, so every `parser.c` compiled with `-O0`. The fixture-driven suites paid for it. Set `[profile.dev.package."*"] opt-level = 1`. Workspace members are unaffected — the glob never matches one — so our own crates stay at 0 and remain quick to rebuild and to step through. The `insta`/`similar` stanzas survive as named overrides, which take precedence over the glob, and keep opt-level 3. Deliberately 1 rather than 2. At 2 the root-crate unit suite, which is thousands of very short parses, ran ~40% slower (65s -> 91s) because the extra inlining grows the hot code more than the optimization saves; that made the whole suite a net regression (98.2s -> 108.2s mean). At 1 that suite is flat and the parse win is intact: deepspeech_test 13.2s -> 6.9s, vcs_per_function 7.8s -> 2.2s, bench lib 3.6s -> 0.9s. The end-to-end gain is modest — 98.2s -> 95.0s mean on `cargo test --workspace --all-features --lib --bins --tests` — and costs a one-time 66.8s -> 114.8s cold dependency build. The win is concentrated in the suites that grow with the corpora. Fixes #1121
`make test` ran `cargo test`, which finishes each of the workspace's 72 test binaries before starting the next — parallelism existed only within a binary, so the ten root integration binaries holding a single `#[test]` each pinned the machine to one core. CI has used nextest since `.config/nextest.toml` landed; this is the local half. `test` now prefers `cargo nextest run --workspace --all-features` and falls back to the previous invocation when `cargo-nextest` is absent, detected via a `NEXTEST` variable mirroring the existing `FD` / `find-by-ext` optional-tool idiom. `_pc-test` and `_ci-test` delegate to `$(MAKE) test` + `$(MAKE) test-doc` instead of repeating the pair, matching the sibling `_pc-*` / `_ci-*` wrappers, so the runner is defined once. Doctests stay on `cargo test` because nextest cannot run them; `insta-review` / `insta-accept` stay too, since insta's nextest integration cannot force-pass to collect pending snapshots. Coverage is unchanged and verified, not assumed: listing with `--run-ignored all` diffs byte-identical against the pre-change 4,741-test inventory, and nextest's default target selection is exactly `--lib --bins --tests` (same 4,731 runnable tests with and without the flags spelled out), so both paths run the same set. The fallback was exercised via `make test NEXTEST=`: exit 0, 72 binaries. Warm build on a 16-core 7700X, `make test` goes 89.14s at 135% CPU to 79.06s at 237%. The modest wall-clock win is expected: one test now bounds the run. `rust_attr_test_handles_deeply_nested_cfg_without_ overflow` takes ~78s of the 79s, while the other 4,730 finish in 17.3s together, so the suite is already at its floor and fixing the O(len^2) cfg-predicate blowup in #1105 is what turns this into a ~4x win. Fixes #1120
Review of the #1120 switch surfaced a behaviour regression: nextest defaults to fail-fast, so `make test` reported only the first failing test where `cargo test` had reported every failure in the first failing binary. That is strictly less informative than the command it replaced, and it is worst for the bulk-snapshot refreshes AGENTS.md documents, where a grammar bump is expected to move hundreds of tests at once and seeing the whole set is the point. Set fail-fast = false on nextest's default profile, matching the ci profile. Also address the smaller review findings: use $(NEXTEST) as the command so a path override is honoured the way FD's is rather than only being tested for emptiness, say "not found or disabled" in the fallback notice since NEXTEST= reaches it deliberately, document the override in the book, drop the binary count that would silently rot, note that the two runners differ in per-test process isolation, explain what --locked does and does not buy in the _ci-test parity comment, and mirror the gate description into CONTRIBUTING.md, which was the third copy of the list.
`cfg_predicate_marks_test` popped each operand off a work stack and probed it for a top-level comma with `cfg_split_top_level_args(...) .nth(1)`. That probe is a full byte scan of the operand, so a predicate with no top-level commas — `all(all(all(...test...)))` — rescanned its whole remaining tail once per nesting level, giving O(len^2) in the attribute body. `exclude_tests` reaches this from every Rust attribute, and this repo's own `bca.toml` enables it, so `make self-scan` paid the cost. A machine-generated or adversarial Rust file with a deep cfg attribute was a denial-of-service vector. Replace the probe with a `CommaIndex`: one forward scan buckets every comma by the paren depth it sits at. A region's operands are split by the commas at the region's own nesting depth, and a region's depth is always its nesting level, so one scan indexes the split points of every region at once and each region asks for its own by lookup. The walk is now linear in the predicate up to the index sort. Semantics are unchanged, including the corners no test named: the signed depth counter still lets an unbalanced `)` strand a comma at a negative depth, an `all(...)` argument list still runs to the operand's last byte rather than the opening paren's match (`all(a)(b)` yields `a)(b`), and parens inside string literals are still counted as structure. The rewrite was checked against the old walker over 3,402,353 generated predicates — 3M random nestings plus an exhaustive sweep of every string up to length 7 over `a ( ) , <space> t` and every sequence of up to 5 combinator tokens — with zero disagreements, and the integration snapshots show no drift. #709's no-recursion property is preserved: the work stack is still explicit and heap-allocated, and both it and the index stay bounded by the predicate's length. Release `bca metrics --no-config --exclude-tests` on a generated `#[cfg(all(all(...test...)))]` item, same host, best of 3: depth before after 5 000 0.06s 0.01s 10 000 0.20s 0.02s 20 000 0.74s 0.04s 40 000 3.09s 0.09s 80 000 11.91s 0.18s Before scales at ~3.9x per doubling (exponent ~1.95); after at 2.0x (exponent 1.00), still flat out to depth 320 000 at 0.78s. The debug build mattered more day to day: the depth-50 000 regression test took 72.65s under `cargo nextest run --lib`, effectively the entire root lib-suite wall time, and now takes 0.038s. Fixes #1105
A test audit of #1105 found the index lookup's lower bound was entirely unexercised: replacing `region.start` with `0` in the `partition_point` call failed zero of 3,969 tests, yet panics on ordinary input such as `any(all(unix, test), all(windows, foo))` — no test anywhere had two sibling regions at the same depth where the earlier one holds a comma. The line was 100% region-covered and never varied, which is exactly the case coverage percentages cannot see. Add rows for that shape, for an ordered-index dependency, and for a comma at negative paren depth; each one now fails its corresponding perturbation and passed none before. Correct three comments that described the code wrongly. `all(a))(b, test)` is not a negative-depth case — the following `(` restores depth to 0 and the comma does split; the row is false because the operand ends in a trailing paren. The cited exhaustive sweep used an alphabet with no `e` or `s`, so `test` was unspellable and every input classified false in both implementations: the run agreed trivially and evidenced nothing. Seed `comma_index_buckets_by_paren_depth`'s stray-paren fixture with a valid depth-0 comma so the assertion distinguishes "dropped the negative-depth comma" from "recorded no commas at all"; against an empty index those read identically (.claude/rules/testing.md). Production code is unchanged.
`classify_cfg_operand` signalled two outcomes through two channels: a bool for "this operand is `test`", and a side-effecting push onto a `&mut Vec` for "descend into these args". It also took a depth solely to compute `depth + 1` for that push. Return a three-variant `Operand` instead and let the caller own the region bookkeeping, which drops the signature from four parameters to two and gives each outcome a name. Fold the trailing-operand handling into the split loop by chaining the region's end as a final boundary, removing a duplicated four-argument call and its guard. This is sound because `splits` filters on `offset < end`, so no real split can equal `region.end`, and splits are strictly ascending, so the operand range is never inverted. Collapse the args offset arithmetic to a single subtraction: `trim_end` shares the slice's start, so the trimmed operand's absolute end is `operand.start + raw.trim_end().len()` directly. Equivalence was fuzzed against the pre-refactor implementation over 3.3M predicates (3M random plus 299,593 exhaustive token sequences) with zero disagreements, over an alphabet that can actually spell `test` — 33,465 of them classified true, so the match path is exercised rather than trivially agreeing. Cognitive drops 8 to 6 and nexits 2 to 1 on `cfg_predicate_marks_test`; both self-scan tiers pass unchanged.
The corpus snapshot harness hardcoded `num_jobs = 4`, and `ConcurrentRunner` reserves one job for the producer (`max(2, n) - 1`), so DeepSpeech's 1042 files were analyzed by exactly 3 consumer threads while the rest of the box idled. Size the pool from `available_parallelism()` instead. Interleaved A/B over 6 pairs (same binary, alternated to cancel background-load drift): `test_deepspeech` in isolation drops from a 6.08s median to 4.92s, faster in 6/6 pairs. Running all five corpus tests concurrently — the case where several runners nominally oversubscribe the machine under nextest — drops from 6.40s to 5.74s median, faster in 5/6 pairs, so the oversubscription does not cost anything: only DeepSpeech runs longer than ~1.2s, so the overlap window is short. The other three items in the issue were measured and dropped. Of `test_deepspeech`'s CPU, 91.7% is `analyze`, 8.3% is the insta pipeline, and 0.26% is the directory walk, so pruning excluded directories during traversal and reworking the snapshot redaction cannot pay for themselves. A single generated 2.5MB `pywrapfst.cc` takes 4.1s on its own and sets a floor no amount of tuning crosses. Merging the five drivers into one binary was also dropped: nextest already schedules across binaries, so it would rename every corpus test for no test-time win. Also promote the #938 empty-corpus guard to an exact per-corpus file count. A traversal or glob change that silently resolves fewer files skips those files' snapshot assertions while still passing; asserting the count makes that a loud failure rather than a quiet coverage loss. Fixes #1123
Review of #1123 pointed out that the new per-corpus count guards the wrong quantity. It counts resolved paths, but the property that matters is how many snapshot assertions ran, and the two diverge: `act_on_file` returns early both when a file is too short to read and when its language cannot be guessed, so a resolved file can skip its snapshot while the count still matches. Count the files that reach the assertion and check that too. The gap is real, not theoretical — simulating an early return that skips files under 100 bytes leaves serde resolving 172 files while asserting only 163 snapshots, which the resolved-path count alone reports as passing. Also make the parallelism fallback read as a consumer count, so it is consistent with the `consumers + 1` convention the comment above it establishes, and refresh tests/README.md, which still documented the hardcoded 4 jobs and the pre-#1123 three-argument signatures. The README is where a contributor looks after bumping a corpus, so it now carries the "update the count alongside the snapshots" note that previously existed only in an assertion message. Deliberately not capping the consumer count: the review suggested .min(8), but its own sweep measured 16 consumers faster than 8 (5.48s vs 5.77s median) on this box.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1128 +/- ##
========================================
Coverage 98.18% 98.18%
========================================
Files 272 272
Lines 68206 68345 +139
Branches 67776 67915 +139
========================================
+ Hits 66968 67107 +139
Misses 848 848
Partials 390 390
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 31, 2026
Coverage answers "did a test run this line", never "would a test notice if this line were wrong". The two come apart whenever every test reaching a line supplies the same value to the part that matters, and the tooling cannot see the difference because the distinction it would need is not what it measures. The instance is #1105's comma-index lookup. Replacing the `region.start` lower bound with `0` panics on ordinary input, yet failed no test at all before PR #1128 added rows covering that shape, while `CommaIndex::splits` sat at 11 of 11 regions covered and 150,200 entries per run. Two supporting cases from the same batch: the file-level percentage aggregated five crate instantiations of which one ever executes, so `cfg_predicate.rs` read 73.26% while the function under review read 100%, and the same artifact dragged the workspace figure down in a change that covered 97 more regions than it started with; and a test count of 4,741 against 4,731 read as ten lost tests when nothing had changed but whether the listing includes ignored entries. Every figure in the entry was re-measured against the tree rather than carried over from the working notes, after a review found the first draft asserting a claim that its own fix had already made false. Cites issues and the PR, not commit hashes, which this repository's rebase merges orphan.
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.
Fixes four issues from the test-performance cluster on one integration
branch. The headline result is that the workspace suite drops from ~89s
to ~17s, essentially all of it from #1105.
7e1583e02c8df5517f948402b2c5fd69b0a00bc1make testruns throughcargo-nextestae1dd592opt-level = 1in dev/test8a28ead3bef08038#1105 — cfg predicate classification (the substantive one)
Every operand rescanned the whole remaining tail probing for a top-level
comma, so a nested predicate like
all(all(all(…test…)))was quadratic inthe attribute body. This path is reached from every Rust attribute under
--exclude-tests— which this repository's ownbca.tomlenables — so amachine-generated or adversarial source file was a denial-of-service
vector.
Commas are now bucketed by paren depth in one forward pass and each region
queries its own bucket. The depth-50 000 regression test goes from 72.65s
to 0.038s; release-mode scaling moves from exponent ~1.95 to a flat 1.00,
still linear at depth 320k.
Behaviour is unchanged, and that is the load-bearing claim here — this
function decides which code gets measured, so any drift silently changes
metric output for every user with
exclude_tests. It was verifieddifferentially against the previous implementation over 3.4M generated
predicates, then independently re-attacked by a reviewer over 14.7M more
(including exhaustive sweeps and unicode/quote soup) with zero
disagreements in either run. Zero integration-snapshot drift.
Note the fix deliberately does not follow the issue's suggested
forward-scan-with-context-stack design: that needs speculative descent
with rollback, because an operand's argument list runs to the operand's
last byte rather than its opening paren's match (
all(a)(b)yieldsa)(b). Removing the rescan from the existing work-stack walk was bothlinear and far easier to prove equivalent.
#1121 — dependency opt-level
The tree-sitter runtime and every grammar are C/C++ libraries built by
cc-rs, which forwards Cargo'sOPT_LEVEL, so they were parsing atroughly half speed in test builds. Workspace members are unaffected and
stay fully debuggable.
Set to
1, not the2the issue proposed —2measured as a netregression on the unit suite at the time (thousands of tiny parses, where
the extra inlining grows the hot code more than it saves). Post-#1105 that
tuning workload no longer exists, so I re-measured; the two experiments
contradicted each other with ~2x spread on identical configs, meaning
background load dominated. Left at the committed value rather than
flipping a documented decision on inconclusive data. The experiment that
would settle it is written up on #1121.
#1120 — nextest
CI already used nextest; the local loop never got the switch. Falls back
to
cargo testwhencargo-nextestis absent, andNEXTEST=forces thefallback (or point it at a specific binary).
One thing review caught: nextest defaults to
fail-fast, which would havemade local runs report less than the
cargo testthey replaced — worstfor the bulk-snapshot refreshes
AGENTS.mddocuments, where a grammarbump is expected to move hundreds of tests and seeing the whole set is the
point. The default profile now disables it, matching the
ciprofile.#1123 — corpus harness
Only one of the issue's four planned sub-items survived profiling. Of
test_deepspeech's CPU, 91.7% isanalyze, 8.3% is the insta pipeline,and 0.26% is the directory walk the issue targeted; a single generated
2.5MB
pywrapfst.cctakes 4.1s alone and sets a floor no listed itemcrosses. Merging the five drivers was also dropped — nextest already
schedules across binaries, so it would rename every corpus test for no
test-time win.
What did land: the runner is sized from
available_parallelism()insteadof a hardcoded 4 (which left 3 consumers on 1042 files), and the #938
empty-corpus guard became an exact per-corpus file count plus a count
of snapshot assertions actually executed. Those two differ — simulating an
early return leaves serde resolving 172 files while asserting only 163,
which the file-count check alone reports as passing.
Test coverage
No test was removed: 4,741 → 4,743, verified by diffing the executed-test
set (not the count) from
cargo nextest list --run-ignored all.Region coverage was compared against
mainwith one identical command.Covered regions go 69,446 → 69,543 (+97), all of it in
cfg_predicate.rs(281 → 378). The workspace percentage reads86.01% → 85.99%, but that is denominator growth:
spaces.rsandspaces/compute.rsgained uncovered instantiation regions while theircovered counts stayed byte-identical (45 and 525), and neither file is
touched by this branch.
Validation
make pre-commitpasses, including every Python stage. Zero.snap.newanywhere; the
big-code-analysis-outputsubmodule pointer is unmoved.Known gaps, deliberately deferred
test asserts termination only, so a reintroduced quadratic would make
the suite slow rather than red. A probe needs
Probeto carrynon-default
MetricsOptions, which perf(checker): the exclude_tests attribute scan is O(attributes x depth) #1100 explicitly owns; building ithere would duplicate that issue. perf(checker): the exclude_tests attribute scan is O(attributes x depth) #1100 has been commented to record that
perf(checker): cfg predicate classification is O(len^2) on nested predicates #1105 now needs a probe too.
correct, but under nextest the ceiling is roughly cores × cores. It
measured harmless because only one corpus test runs beyond ~1.2s. The
remedy when that changes is a
test-groupsentry in.config/nextest.toml, not a per-runner cap — a hardcoded.min(8)measured worse than uncapped. Recorded on test(perf): corpus snapshot harness caps at 3 threads and walks 25k entries to select 869 #1123.