perf(metrics/cognitive): inherit nesting via the walker#1064
Merged
Conversation
dekobon
added
bug
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1064 +/- ##
========================================
Coverage 98.04% 98.04%
========================================
Files 267 267
Lines 66073 66363 +290
Branches 65643 65933 +290
========================================
+ Hits 64778 65063 +285
- Misses 854 859 +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
force-pushed
the
fix/1062-parent-lookups
branch
from
July 25, 2026 18:05
8ee6e1d to
0f3ce88
Compare
`get_nesting_from_map` recovered a node's inherited `(nesting, depth, lambda)` triple with `node.parent()`. `Node::parent` is O(depth) -- tree-sitter stores no parent pointer, so `ts_node_parent` restarts at the root and descends -- which made the metric O(nodes x depth). With the `tokens` walk fixed in #1052 this was the dominant remaining cost on nesting-heavy input. The map is now keyed so a node's own slot holds what that node inherits: after a node's `compute` has run, the walker seeds each child's slot from it. The lookup becomes a single hash probe and no `parent()` call remains on the per-node path. Values are unchanged, including the case that made this non-obvious. Python's comprehension handling pre-writes its clause children's slots during the *comprehension's* compute (the #421 fix, so clause nesting does not depend on sibling traversal order), and those deliberately differ from the comprehension's own triple. The walker therefore seeds with `or_insert`, not `insert`; a blanket overwrite would have silently shifted cognitive for every Python comprehension. All 22 read sites and both non-self write sites were checked before settling on this shape. On shapes that exercise only this path, whole-file analysis is now linear: nested parentheses at depths 8000 / 16000 / 32000 / 64000 take 13 / 23 / 45 / 88 ms, so a 128 KB file completes in under a tenth of a second, where 8 KB previously took over a second. This does NOT make deeply nested code linear in general. `Checker::is_else_if` calls `Node::parent` per `if` across 13 languages, so nested and `else if`-chained conditionals stay quadratic: 60 / 224 / 824 / 3325 ms at depths 1000 / 2000 / 4000 / 8000, against 6 / 10 / 16 / 44 ms for the identical shape built from `while`. The regression test asserts the value *and* the scaling ratio. A value assertion alone would let a regression hang CI rather than fail it, since the pre-fix code produced identical numbers, only quadratically. An absolute wall-clock budget was rejected: it is calibrated to one machine, and this suite also runs under `cargo llvm-cov` and on shared Windows / macOS runners. Doubling the depth costs ~2x when the lookup is O(1) and ~4x when it is O(depth), independent of host speed -- revert-tested at 4.2x failing against ~2x passing. The test uses nested `while`, not `if`, precisely because `is_else_if` leaves the `if` shape quadratic; anchoring to it would have measured the unfixed path. `metrics_inner` grew 16% in halstead effort past its recorded value; the baseline is refreshed here per the same-PR discipline. Still outstanding, all tracked in #1062: `Checker::is_else_if` (above), `Node::count_specific_ancestors` (4000 nested declarations, ~557 ms; also called twice per node by Python's boolean-operator handling), `increment_function_depth` (4000 nested Rust fns, 55 KB, ~1.9 s), and Elixir's `is_inside_quote_block`, which runs for every node behind no metric selection and so cannot be deselected (4000 nested quote blocks, 52 KB, ~2.7 s). A shared parent lookup owned by the walker would retire them together -- an earlier claim that each needs a different mechanism was wrong. Refs #1062
`loc_verbatim`, `tokens_of` and `cognitive_of` were three near-identical bodies added in three consecutive PRs, each doing `analyze(Source::new(lang, bytes), opts).expect(..).metrics`. They exist because `check_metrics` cannot serve these tests: `check_func_space` normalises CRLF and trims then re-appends a trailing newline, so a construct ending at EOF is unreachable through it and such a test passes vacuously -- that is how #1051 escaped a ~3000-test suite -- and its `check` parameter is a bare `fn` pointer, so a test that needs to compare two analyses cannot get a value out. Replaced by one `metrics_verbatim(lang, source, options)` in `src/tools.rs`, beside the other cross-module test helpers, with that rationale stated once instead of three times. `#[track_caller]` so a failing `expect` reports the test's line rather than the helper's. `loc_verbatim` collapsed into `rust_loc`: it had become a one-line wrapper with a single caller. Net -5 lines. `src/tools.rs` grew 767 -> 779 gated SLOC and its baseline entry is refreshed accordingly. Note the file is now 21 lines under its 800 hard cap, and the budget is not fungible -- with `exclude_tests`, a `#[cfg(test)]` fn body costs nothing but its doc comment costs full price. A better end state is to move all five `#[cfg(test)]` helpers out of `tools.rs` into the ignore-listed `tools_tests.rs`, which would drop the file to ~746 and retire the baseline entry entirely; filed separately rather than half-migrating one helper away from its four siblings. The `.bca-baseline.toml` hunk touching `metrics_inner`'s `start_line` is drift from the preceding commit, not from this change: `start_line` is only a +/-50-line disambiguator and the recorded value is unchanged. Three similar call sites in `src/metrics/cyclomatic.rs` were left alone deliberately -- they pass `.with_name(Some("try.rs"))`, which this signature does not express.
The cognitive nesting map is keyed by tree-sitter node id and probed
several times per AST node. `HashMap`'s default SipHash-1-3 exists to
resist hash-flooding from untrusted *keys*; these keys are
pointer-derived `usize` values we produced ourselves and are never
attacker-chosen, so the keyed round buys a property this map cannot
use.
Replaced with FxHash behind a `NestingMap` alias. The swap is
behaviour-neutral by construction: the map is only ever touched by
`get` / `insert` / `entry` and is never iterated, so no metric value or
ordering can depend on the hasher.
Two details that are easy to get wrong, and that a first cut here did
get wrong:
* `finish` rotates left by 26. A multiplicative hash concentrates
entropy in the top bits while hashbrown takes the bucket index from
the bottom ones, and node ids are pointer-aligned -- without the
rotate every hash ends in at least three zero bits and only one home
bucket in eight is reachable. That is cheap on x86-64 (SSE2 probe
group of 16) but materially worse on aarch64 (NEON group of 8),
which is a shipped release target. `rustc-hash` added this in 2.0;
copying the 1.x algorithm reproduces exactly the defect upstream
removed.
* `write_u64` / `write_u32` / `write_u8` are overridden alongside
`write_usize`. Without them a key-type change falls back to
`write`'s byte-at-a-time loop -- eight dependent rounds per word,
slower than the SipHash this replaces, silently and with no test to
catch it.
Effect size, stated honestly: an independent replication against a
freshly built parent commit measured ~3% off total runtime with a
one-sided permutation p = 0.028 and a clean control, so the win is real
but modest. The per-metric figure is not quotable to two significant
figures -- five interleaved blocks on this workload put the
cognitive-minus-nom delta anywhere between 637 ms and 1948 ms, drifting
downward run over run, and a bootstrap CI on the derived percentage
spans roughly 7-41%.
Corpus, corrected: `tests/repositories/DeepSpeech` is 25022 files --
6104 .cc, 3517 .h, 2862 .py -- so roughly 78% C/C++, not the Python
corpus an earlier draft of this message claimed.
On owning the algorithm rather than depending on one: `foldhash` 0.1.5
and 0.2.0 are already in `Cargo.lock` via `hashbrown` and `actix`, so
promoting one to a direct dependency would add no new crate, license or
MSRV surface. That is a fair argument for doing so, and the defect
above is a fair argument that owning a copy means owning its bug list.
Kept in-tree for now at twenty lines; revisit if it needs a third fix.
The nesting map's value was `(usize, usize, usize)` destructured
positionally in 20 language modules and rebuilt positionally in 22
places, against AGENTS.md's "do not pass two same-typed primitives where
they could be confused". It is now a `Nesting` struct with
`conditional` / `function_depth` / `lambda` fields.
Only the boundary changed. The locals keep their names and the arm
bodies are untouched, because those names appear 8-36 times per file and
renaming them would be churn with no benefit; what matters is that the
read and the write now say which field is which.
`function_depth` and `lambda` are separable, and the new
`javascript_function_depth_and_lambda_are_distinguishable` guards it.
Getting there took a wrong turn worth recording: transposing the two at
the write site left the whole suite green, and five hand-built shapes
(`fn>arrow>fn>if`, `arrow>fn>if`, `fn>arrow>if`, `arrow>arrow>if`,
`fn>fn>if`) all measured identically, so an earlier draft of this commit
concluded they were interchangeable and said no test could ever catch a
mix-up. That was wrong.
The two are summed symmetrically almost everywhere, so a swap preserves
the total. The single asymmetric operation in the whole cognitive family
is the JS `FunctionDeclaration` arm's `lambda = 0`. Whether it separates
them depends on the *parity* of the ancestor chain between the innermost
arrow and the `function_declaration`, because a write-site swap
transposes the pair at every node on the way down. Plain
`arrow -> statement_block -> function_declaration` is odd parity and
cancels; a second chained arrow flips it. Every one of those five shapes
happened to sit on the cancelling side.
const f = () => () => { function inner() { if (a) { } } };
measures 1 normally and 2 with the fields transposed, so it is a real
guard, verified by revert. `javascript_arrow_contributes_lambda_nesting`
pins the `ArrowFunction` arm separately, since the shape above resets
lambda before the measurement and so cannot see that arm at all.
Also drops the stale positional wording from `get_nesting_from_map`'s
doc and the "triple" references in `spaces::compute`, which otherwise
re-taught the naming this commit removes.
`metrics_inner` recorded `stack.len()` before calling `push_children` and sliced `&stack[first_child..]` fourteen lines later, to hand this node's children to `propagate_nesting_to_children`. Correct today, but the invariant that nothing may push between those two points was stated at neither site. `push_children` now returns the children it pushed as a slice borrowed from `stack` -- empty for a leaf, reverse source order like the stack itself. Borrowing rather than returning indices is what makes this a compiler-checked claim: inserting a `stack.push(..)` between the call and the use is now `error[E0499]`, verified, where before it silently widened the slice. `ops.rs` discards the return and needed no change beyond a note saying why. A first cut returned `Range<usize>`. That is strictly better than the hand-rolled arithmetic -- the range is frozen at return time, so it cannot over-count -- but two bare integers carry no relationship to the vector, so the validity window stayed a convention. The borrow closes it for the same line count. Correcting a claim in that first cut: it said the existing cognitive suite covers the range end to end, "since any wrong range breaks nesting inheritance". That is true only for under-count. Perturbing the range to sweep in one extra node leaves all 332 cognitive tests passing, because `propagate_nesting_to_children` uses `entry().or_insert()` and every node is already seeded when it is pushed, so the extra write no-ops. Under-count is caught loudly (102 and 30 failures for the two off-by-one directions); over-count is invisible. It is unreachable after this change, which is the point, but the evidence offered for it was wrong. Also adds a `debug_assert!` that the scratch buffer arrives drained -- the new doc asserts the slice is exactly this node's children, which leftovers would falsify. The tractability test now takes the best of three timings per depth. It flaked once inside `make pre-commit`, which runs clippy, rustdoc and the Python stages alongside the suite: 70 ms vs 394 ms, a 5.6x reading on code that is linear. The two timings are sequential, so a load spike between them skews the ratio by itself. Contention is bursty and sheds under a minimum; a real O(depth) regression is present in every run and still fails at 4.0x, verified by revert.
`metrics_inner` pre-seeded the cognitive nesting map with the root, and
`propagate_nesting_to_children` handed that default down the tree. The
two grammars whose `Cognitive::compute` is the macro's no-op (`Preproc`,
`Ccomment`) therefore built an entry per AST node that nothing ever
read. Dropping the seed removes it, with no trait change and no
per-grammar branch.
The transformation is value-preserving by construction, which is a
shorter argument than auditing impls: the seed's only effect was the
value observed at the root, and `get_nesting_from_map` already returns
the identical `Nesting::default()` there. An unseeded child reads the
same default too. So every node observes exactly what it observed
before, whatever any individual impl does.
Two effects, measured with temporary instrumentation on a four-line
preprocessor input (map entries at end of walk):
Preproc 7 -> 0
Ccomment 7 -> 0
C 14 -> 14
C, --metrics loc 1 -> 0
The last row is the broader one and an earlier draft of this message
missed it. Whenever `cognitive` is deselected nothing else ever writes
the map, so the seed was its *only* entry -- meaning every metric-subset
run, on all 25 languages, allocated a hash table per file for a single
entry no one read. `NestingMap::default()` allocates nothing.
The map size tracks the AST node count exactly, so a real header sheds
one entry per node: `/usr/include/stdio.h` is ~885 Preproc nodes.
`propagate_nesting_to_children`'s missing-slot branch becomes the
documented normal path for those two grammars and loses its
`debug_assert!(false, ..)`. Nothing was lost: the assert was already
unreachable, and the obvious replacement is vacuous, because a miss is
*root-only* -- every non-root slot is created by that function's own
`or_insert` before the node is popped, so at a miss the map is always
empty. That is also the invariant the code comment now states; a first
draft justified it by claiming every real impl writes on every path,
which is true (23 of them do) but is not what the code relies on.
Cognitive values are unchanged; 3070 lib tests pass.
No test pins the saving. Map size is not observable outside the walk, a
unit test on the helper would build its own map and so would not catch a
re-added seed, and an end-of-walk assertion cannot tell a no-op grammar
from a real one without a trait const this deliberately avoids.
Codecov flagged 14 uncovered lines on this PR, all in `src/spaces.rs`:
`write`, `write_u64`, `write_u32` and `write_u8` on `NodeIdHasher`.
`HashMap<usize, _>` only ever calls `write_usize`, so those four are
unreachable today. They exist so that changing the key type cannot
silently fall back to `Hasher::write`'s byte-at-a-time default -- eight
dependent rounds per word, slower than the SipHash this replaced -- and
that regression is invisible, because results stay correct and only
speed changes.
Two tests, both verified by revert:
* the integer writes must agree for the same value, and the byte-slice
path must NOT agree with them -- if it ever did, the overrides would
be doing nothing and their removal would go unnoticed. Deleting
`write_u64` fails this.
* `finish` must rotate. A multiplicative hash concentrates entropy in
the top bits while hashbrown takes the bucket index from the bottom
ones, and node ids are pointer-aligned, so without the rotate every
hash ends in three zero bits and one home bucket in eight is
reachable. A first cut of this hasher had exactly that defect.
Removing the rotate fails this.
`src/spaces.rs` goes 43 -> 29 uncovered lines; the remaining 29 predate
this branch (`a17e82b1`).
Also makes the cognitive scaling assertion opt-in behind
`BCA_ASSERT_SCALING`. Its value assertions still run by default; only
the wall-clock ratio is gated. It has now produced a false failure in
three environments -- `windows-latest` at 10.9 s against an absolute
budget, a local `make pre-commit` at 5.6x with clippy and rustdoc
alongside, and `cargo llvm-cov` at 3.5x even with best-of-three, whose
instrumentation is the reason this surfaced during coverage work. The
`coverage` job runs in CI, so leaving it armed reds builds on
measurement artefacts. It still fails at 4.1x with the flag set against
the pre-fix lookup, so the guard is intact where it can be trusted.
Complexity guards belong in a benchmark harness (#1068).
dekobon
force-pushed
the
fix/1062-parent-lookups
branch
from
July 26, 2026 04:40
44073e5 to
c74f5e6
Compare
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 the first of the four
Node::parent()misuses tracked in #1062 — theone in this issue's title. The other three are deliberately not in this
PR; see Scope below.
Problem
Cognitiverecovered each node's inherited(nesting, depth, lambda)tripleby looking up its parent:
Node::parentisO(depth)— tree-sitter stores no parent pointer, sots_node_parentrestarts at the root and descends — making the metricO(nodes × depth). With #1052'stokenswalk fixed, this was the dominantremaining cost on nesting-heavy input: 1080 ms of the 1122 ms total at nesting
depth 4000, against ~5 ms for every other metric.
Fix
The map is re-keyed so a node's own slot holds what that node inherits.
After a node's
computehas run, the walker seeds each child's slot from it,so the lookup is a single hash probe and no
parent()call remains on theper-node path.
Whole-file analysis of nested parentheses is now linear — exactly 2× per
doubling:
A 128 KB file now completes in under a tenth of a second. Before #1052, 4 KB
took over two minutes.
The subtlety worth reviewing carefully
Re-keying looks like it needs no per-language changes, and almost does — but
Python's comprehension handling pre-writes its clause children's slots
during the comprehension's own
compute(the #421 fix, so clause nestingdoesn't depend on sibling traversal order), and those values deliberately
differ from the comprehension's triple.
The walker therefore seeds with
or_insert, notinsert. A blanketoverwrite compiles, reads fine, and would silently shift cognitive for every
Python comprehension. All 22 read sites and both non-self write sites were
enumerated before settling on this shape.
Cognitive values are unchanged — 329 cognitive tests and 3,068 lib tests pass.
Testing
cognitive_deep_nesting_is_tractableasserts the value (n(n+1)/2fornnested
ifs, so nesting is still inherited correctly at depth) and awall-clock budget — the pre-fix code produced identical numbers, only
quadratically, so a value assertion alone would let a regression hang CI rather
than fail it.
Its first form did not discriminate: at depth 300 the unfixed code passed in
0.18 s. It now runs at depth 3000, where unfixed takes 16.8 s and fails
with a clear message against 1.7 s fixed. The 8 s budget sits between them
with ~4.7× headroom above the passing time and ~2.1× below the failing one;
the doc comment records those margins and says to raise the depth rather than
the budget if it ever gets flaky.
Scope — three sites remain, deliberately
Loc::count_specific_ancestorsparent()perDeclaration(the walk stops at the enclosingCompoundStatement)Loc)cognitive::increment_function_depthO(depth²)per callelixir_is_inside_quote_blockO(depth²)per callThere is no cheap universal fix. Rooting a
TreeCursorand ascending with theO(1)goto_parent()doesn't work —tree_sitter::Nodeexposes no way backto its
Tree, andCursor::reset(node)makes that node the cursor root, sogoto_parent()returns false immediately. A walker-ownedparent_ofmap failson access, because these helpers are called from
Loc::compute,Checker::promotes_to_func_space_with_codeandGetter::get_space_kind_with_code, none of which receive walker state.Each therefore needs its own signature change — ~60 semantic edits across three
unrelated mechanisms, for input shapes needing thousands of nesting levels.
Bundling them here would put three mechanisms behind one review. Full design
notes and churn estimates are on #1062, which stays open.
Baseline
metrics_innergrew 16% in halstead effort past its recorded value; thebaseline is refreshed in the same commit per the same-PR discipline. Note this
is the second consecutive refresh of that function (+12% for #1052's comment
flag, +16% here) — both are genuine walker-level state threading and each
extraction is already a named helper, but a third would be worth pushing back
on rather than baselining again.
Validation
make pre-commitgreen — 3,068 lib tests, clippy clean on default and--all-features, markdown lint, man-page drift, both self-scan tiers, nosnapshot drift.
Refs #1062