From 03cb037fe19536b3d235b65d8a29769ffc320c0a Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 30 Aug 2026 12:23:06 -0700 Subject: [PATCH 1/2] perf(metrics): bound the cognitive nesting map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The walk's `NestingMap` grew to one entry per AST node and was reserved at `descendant_count` up front. Each slot is seeded by the parent, read once by the node's own `compute`, read once more when its children are seeded, and was then kept until the walk ended, so the map converged on the tree's size whether or not anything still read it. Free the slot in its last reader — `propagate_nesting_to_children`, plus the `exclude_tests` prune arm, which pops a seeded node without computing or propagating it — and drop the up-front reserve. The live set is then the seeded-but-unvisited nodes, bounded by the traversal stack. `propagate_nesting_to_children` reserves per sibling set, so a generated parser table hanging half a million children off one `initializer_list` still costs one allocation rather than a doubling chain. Measured on a 28 MB generated `tree-sitter` `parser.c`, release, `-j 1`: 1,265 MB -> 735 MB peak RSS, 5.1 s -> 3.3 s. `metrics -O json` over a 100k-file cargo registry at `-j 16`: 5.1 GB -> 3.6 GB. `tests/repositories/DeepSpeech` at `-j 16`: 488 MB -> 439 MB. Attributed by selection: `--metrics cognitive` alone accounted for 540 MB of the old peak, while every other metric and a parse-only `dump` sat at ~722 MB. `make bench-scaling` is unchanged, all 27 probes linear. No metric value moves, so nothing in the output can show the map was retaining dead entries. A `debug_assert!` covers every walk the suite runs, and `nesting_slots_retained` plus `walk_frees_every_cognitive_nesting_slot` pin the four paths that seed a slot: a plain nested walk, the two overrides that write a child's slot ahead of the walk (Python comprehension clauses, Tcl `try` handler bodies), and an `exclude_tests` prune. Each freeing site was verified by reverting it and watching that test fail. `Cognitive::SEEDS_NESTING` and `Node::descendant_count` existed only to size the reserve and are removed with their tests. `parser_cache`'s module doc claimed a retention bound of "tens of KiB" per thread; the pinned tree-sitter caps those pools at fixed counts and the surrounding scratch arrays track parse depth rather than input size, so the doc now states what the source supports instead. Fixes #1375 --- CHANGELOG.md | 18 +++++ big-code-analysis-book/src/recipes/ci.md | 7 +- src/macros/mod.rs | 4 -- src/metrics/cognitive.rs | 67 ------------------ src/node.rs | 47 ------------- src/node/parser_cache.rs | 14 ++-- src/spaces/compute.rs | 88 ++++++++++++++++-------- src/spaces_tests.rs | 65 +++++++++++++++++ 8 files changed, 159 insertions(+), 151 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe974d69b..aa469a4f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,24 @@ for historical reference. ## [Unreleased] +### Performance + +- The metric walk's cognitive nesting map no longer grows to one entry + per AST node (#1375). Each node's slot was seeded by its parent, read + once by its own `compute`, read once more when its children were + seeded, and then kept until the walk ended; the map was also reserved + at the tree's node count up front. On a 28 MB generated + `tree-sitter` `parser.c` that reserve was 540 MB of a 1,265 MB peak. + The slot is now freed by its last reader, so the map holds only the + seeded-but-unvisited nodes — bounded by the traversal stack, not the + tree. Same file, release build, `-j 1`: 1,265 MB → 737 MB peak RSS + and 5.1 s → 3.9 s; `metrics -O json` over a 100k-file cargo registry + at `-j 16`: 5.1 GB → 3.6 GB; `tests/repositories/DeepSpeech` at + `-j 16`: 488 MB → 439 MB. No metric value moves. What remains is + dominated by the parse rather than by the walk: a parse-only + `bca dump` over the same files peaks at 25–70× the source size, which + the CI recipe now states as the `--jobs` memory sizing rule. + ### Changed - The `tree-sitter` runtime is `=0.26.13`, up one upstream patch diff --git a/big-code-analysis-book/src/recipes/ci.md b/big-code-analysis-book/src/recipes/ci.md index 7348561ad..6c0974f53 100644 --- a/big-code-analysis-book/src/recipes/ci.md +++ b/big-code-analysis-book/src/recipes/ci.md @@ -868,7 +868,12 @@ Applies regardless of provider: cgroup-/cpuset-/quota-aware on Linux, OS CPU count on macOS/Windows — so CI runners no longer need to thread `--jobs "$(nproc)"` through every recipe. `--jobs 1` - remains a debugging knob, not a default. + remains a debugging knob, not a default. It is *not* + memory-aware: each worker holds one whole file in memory at a + time, and a parse-only run measures 25–70× the source size, so + peak RSS is roughly `jobs × 70 × the largest source file` in + the tree. A memory-limited container with many vCPUs should + pass an explicit `--jobs`. - **Always pass `--strip-prefix "$PWD/"` to `bca report markdown`** so the path column is identical across runners with different workspace paths. Without it the diff between two reports is diff --git a/src/macros/mod.rs b/src/macros/mod.rs index 0311a9981..856cd6f46 100644 --- a/src/macros/mod.rs +++ b/src/macros/mod.rs @@ -42,10 +42,6 @@ macro_rules! implement_metric_trait { (Cognitive, $($code:ident),+) => ( $( impl Cognitive for $code { - // No slot is ever written, so the walker must not - // pre-size a nesting map this grammar leaves empty. - const SEEDS_NESTING: bool = false; - fn compute<'a>( _node: &Node<'a>, _code: &'a [u8], diff --git a/src/metrics/cognitive.rs b/src/metrics/cognitive.rs index f1f7cd160..20db1aff6 100644 --- a/src/metrics/cognitive.rs +++ b/src/metrics/cognitive.rs @@ -153,16 +153,6 @@ pub(crate) trait Cognitive where Self: Checker, { - /// Whether [`compute`](Cognitive::compute) writes a `nesting_map` - /// slot for the node it was handed. - /// - /// Every real implementation does, on every path, which is what - /// lets the walker size the map to the node count up front. The - /// macro-generated no-op impls (`Preproc`, `Ccomment`) never write - /// one, so their map stays empty and must stay unallocated too — - /// they override this to `false`. - const SEEDS_NESTING: bool = true; - /// Walk `node` and update `stats` with this metric for the language /// implementing the trait. /// @@ -872,63 +862,6 @@ mod tests { } } - /// `SEEDS_NESTING` must say what `compute` actually does. - /// - /// The walker trusts the const twice over: `true` means the map is - /// worth sizing to the node count up front, `false` means it must be - /// left unallocated. Both are silent when wrong — a `false` on a - /// language that does seed only costs the rehashing back, and a - /// `true` on one that does not only wastes an allocation, so no - /// metric value moves either way. This pins each impl against what - /// it observably writes. - #[test] - fn seeds_nesting_matches_what_compute_writes() { - use std::path::Path; - - fn writes_a_slot(source: &str, filename: &str) -> bool { - let parser = T::new(source.as_bytes().to_vec(), Path::new(filename), None); - let mut nesting_map = NestingMap::default(); - T::Cognitive::compute( - &parser.root(), - parser.code(), - Ancestors::known(&[]), - &mut Stats::default(), - &mut nesting_map, - ); - assert_eq!( - ::SEEDS_NESTING, - !nesting_map.is_empty(), - "{filename}: SEEDS_NESTING disagrees with what compute wrote" - ); - !nesting_map.is_empty() - } - - // One representative of each family that carries a real impl: - // the C-like macro, the JS-family macro, and the two languages - // with hand-written `compute` bodies. - assert!(writes_a_slot::( - "int main() { return 0; }", - "a.cpp" - )); - assert!(writes_a_slot::( - "function f() { return 0; }", - "a.js" - )); - assert!(writes_a_slot::( - "def f():\n pass\n", - "a.py" - )); - assert!(writes_a_slot::( - "def f do\n :ok\nend\n", - "a.ex" - )); - - // The macro-generated no-ops: `SEEDS_NESTING` is what keeps the - // walker from reserving a map they never fill. - assert!(!writes_a_slot::("#define A 1\n", "a.h")); - assert!(!writes_a_slot::("/* c */ int x;", "a.c")); - } - /// A `Stats::default()` that never sees an /// observation must not leak the `usize::MAX` sentinel for /// `structural_min`. The getter collapses the sentinel to `0.0` diff --git a/src/node.rs b/src/node.rs index 7e034d4d6..0233c7c37 100644 --- a/src/node.rs +++ b/src/node.rs @@ -284,17 +284,6 @@ impl<'a> Node<'a> { self.0.child_count() } - /// Number of nodes in this node's subtree, counting the node itself. - /// - /// `O(1)`: `tree_sitter` stores the visible-descendant count on each - /// subtree, so this is a field read rather than a walk. It counts the - /// same nodes the metric walk visits — visible children, named and - /// anonymous alike — which is what makes it usable as an exact - /// capacity for a per-node map (see `spaces::compute::metrics_inner`). - pub(crate) fn descendant_count(&self) -> usize { - self.0.descendant_count() - } - // Returns `true` if this node is a named grammar production // (as opposed to an anonymous token such as a punctuation or // keyword literal). Used to skip anonymous tokens like the @@ -1275,42 +1264,6 @@ mod tests { ); } - /// `descendant_count` must count the same nodes the metric walk - /// visits, because `spaces::compute::metrics_inner` uses it as the - /// exact capacity for a map that ends up holding one entry per - /// visited node. - /// - /// The risk it guards is silent: `ts_node_descendant_count` counts - /// *visible* descendants, so were it ever to narrow to named nodes - /// only, the reserve would under-size by the anonymous-token share - /// of the tree — roughly half — and the map would quietly go back - /// to rehashing, with no test failing. The source below is chosen to - /// carry plenty of anonymous tokens (`int`, `(`, `{`, `=`, `;`) so - /// the named-only reading is not accidentally equal. - #[test] - fn descendant_count_matches_the_walked_node_population() { - let code = b"int main() { int x = 1; foo(x); return 0; }"; - let tree = Tree::new::(code); - let root = tree.get_root(); - - // `preorder` yields the node itself and then every descendant, - // enumerating children exactly as the metric walk's - // `push_children` does. - let walked = root.preorder().count(); - assert_eq!( - root.descendant_count(), - walked, - "descendant_count must equal the pre-order node count" - ); - - let named = root.preorder().filter(Node::is_named).count(); - assert!( - named < walked, - "fixture must contain anonymous tokens, else the assertion \ - above cannot distinguish a named-only count" - ); - } - /// Drains `iter`, holding it to the `ExactSizeIterator` contract at /// every step, and returns the `(id, kind_id)` of each child yielded. /// diff --git a/src/node/parser_cache.rs b/src/node/parser_cache.rs index a47a7009b..99cc7fef8 100644 --- a/src/node/parser_cache.rs +++ b/src/node/parser_cache.rs @@ -7,11 +7,15 @@ //! already grown. Construction is *not* what costs; the re-grown buffers //! are, and only modestly — see issue #1118 for the measurements. //! -//! What a thread keeps between files is bounded by the largest file it -//! has parsed (tens of KiB): the subtree and stack-node pools are -//! capped, but the scratch arrays around them are cleared without -//! releasing capacity, so a long-lived server holds that per worker for -//! the process lifetime. +//! What a thread keeps between files is the subtree and stack-node +//! pools, which are capped at small fixed counts, plus the scratch +//! arrays around them, which are cleared without releasing capacity — +//! so a long-lived server holds the latter per worker for the process +//! lifetime. Those arrays track parse-stack depth and reduce-action +//! counts rather than input size, so this is deliberately not stated as +//! a bound in bytes: the "tens of KiB" figure that stood here was never +//! measured, and #1375 established only that the *input* can run to +//! tens of MB, not that the retention follows it. //! //! Only the parser is cached, not the language bound to it. Rebinding //! per file costs nothing measurable — the gain survives consecutive diff --git a/src/spaces/compute.rs b/src/spaces/compute.rs index f6539b376..2449679f0 100644 --- a/src/spaces/compute.rs +++ b/src/spaces/compute.rs @@ -6,11 +6,15 @@ //! verbatim and re-exported from the parent so the public path //! `crate::spaces::analyze` (and `pub(crate) metrics_inner`) is preserved. -use std::hash::BuildHasherDefault; - use super::*; use crate::diag::warn; +// Walks that ended with a cognitive nesting slot still live. Freeing +// each slot when its node is popped keeps the map bounded by the +// traversal stack instead of the tree (#1375); reverting that changes +// no output, only the peak, so this is what a test can observe. +crate::observation::counter!(nesting_slots_retained); + /// Derives the two metrics that read from a space's *complete* state: /// Halstead's `Stats` from the accumulated occurrence maps, and MI from /// the resulting volume plus the space's final LOC and cyclomatic. @@ -562,11 +566,14 @@ fn propagate_nesting_to_children( children: &[(Node<'_>, Walk)], nesting_map: &mut NestingMap, ) { - // Leaves are roughly half of a real AST, so bail before hashing a key - // we would only read to iterate zero children. - if children.is_empty() { - return; - } + // `remove`, not `get`: this is the slot's last reader — its own + // `compute` has already consumed it and the children seeded below + // each carry a copy — so freeing it here is what keeps the map + // bounded by the traversal stack rather than the tree (#1375; see + // the declaration in `metrics_inner`). Leaves free theirs too, which + // is why there is no longer an empty-children bail-out ahead of the + // hash: it only ever skipped a read, and the slot stayed behind. + // // A miss here is a *root-only* path. Every non-root slot is created // by this function's `or_insert` below, before that node is ever // popped, so reaching a node with no slot means it had no parent to @@ -578,9 +585,17 @@ fn propagate_nesting_to_children( // path: a real impl that skipped its write would still leave its // children seeded, and would show up as wrong nesting values, not as // a missing slot. - let Some(&inherited) = nesting_map.get(&node.id()) else { + let Some(inherited) = nesting_map.remove(&node.id()) else { return; }; + // One allocation for the whole sibling set rather than a doubling + // chain through it. Dropping the walk's up-front reserve left this + // as the only place the map grows, and a generated parser table can + // hang half a million children off one `initializer_list` — which + // from an empty map is ~18 doublings and twice that many re-inserts. + // A `reserve` that the current capacity already covers is a compare, + // so the ordinary two- or three-child node pays nothing. + nesting_map.reserve(children.len()); for (child, _) in children { nesting_map.entry(child.id()).or_insert(inherited); } @@ -659,26 +674,20 @@ pub(crate) fn metrics_inner( // back to `Nesting::default()`, so a seed would change nothing for // grammars that compute cognitive — while for the two whose impl is // the macro's no-op it is the one write that would make the walk - // build an entry per node that nothing ever reads. - // - // Sized up front rather than grown: every real `Cognitive::compute` - // ends by writing its own node's slot, so the map converges on one - // entry per visited node and a default-capacity map rehashes its way - // there a doubling at a time. `descendant_count` is that final size, - // known in O(1) — an upper bound rather than an exact one only when - // `exclude_tests` prunes a subtree the walk never descends into. + // seed and free a slot per node that nothing ever reads. // - // Both guards exist to keep an empty map unallocated: an unselected - // `Cognitive` never calls `compute` at all, and the two grammars - // whose impl is the macro's no-op (`Preproc`, `Ccomment`) report - // `SEEDS_NESTING = false` because they write no slot. - let mut nesting_map = if selected.contains(Metric::Cognitive) - && ::SEEDS_NESTING - { - NestingMap::with_capacity_and_hasher(node.descendant_count(), BuildHasherDefault::default()) - } else { - NestingMap::default() - }; + // The map holds only nodes that have been seeded and not yet + // popped: `propagate_nesting_to_children` frees a slot as its last + // reader, so the live set is bounded by the traversal `stack`, never + // by the tree. It used to be reserved at `descendant_count` on the + // reasoning that it would converge on one entry per node — which it + // did, and on a 28 MB generated `parser.c` that reserve alone was + // 540 MB of a 1.27 GB peak (#1375). Grown from empty, it now peaks + // at the widest pending sibling set instead, and stays unallocated + // for good when nothing seeds the root: an unselected `Cognitive` + // never calls `compute`, and the two grammars whose impl is the + // macro's no-op (`Preproc`, `Ccomment`) write no slot. + let mut nesting_map = NestingMap::default(); // Suppression markers are resolved inline during the walk rather // than queued for a post-finalize pass. When we visit a comment @@ -757,6 +766,11 @@ pub(crate) fn metrics_inner( .loc .exclude_test_span(node.start_row(), node.end_line()); } + // The pruned node was seeded by its parent and is neither + // computed nor propagated, so nothing else frees its slot. + if selected.contains(Metric::Cognitive) { + nesting_map.remove(&node.id()); + } continue; } @@ -816,6 +830,26 @@ pub(crate) fn metrics_inner( } } + // Every slot is freed by the node that last reads it, so a non-empty + // map here means a seeded node was popped without freeing — the + // O(nodes) growth #1375 removed, which no metric value can reveal. + // + // Two guards, covering different ground. The counter is what the + // named regression test reads, and it survives into release builds. + // The assertion generalises that test to every other walk the suite + // runs — twenty languages of corpus fixtures rather than the four + // the test names — which is the coverage that matters here, because + // the freeing rule now has two owners (this walk's prune arm and + // `propagate_nesting_to_children`) and a third `continue` added + // later would leak in silence. + debug_assert!( + nesting_map.is_empty(), + "every seeded nesting slot must be freed by the node that last reads it (#1375)" + ); + if !nesting_map.is_empty() { + nesting_slots_retained::record(); + } + finalize::(&mut state_stack, usize::MAX, selected); // Reserved error path: `MetricsError::EmptyRoot` is unreachable diff --git a/src/spaces_tests.rs b/src/spaces_tests.rs index 57fb32ee5..74a507263 100644 --- a/src/spaces_tests.rs +++ b/src/spaces_tests.rs @@ -2466,3 +2466,68 @@ mod nameless_construct_spaces { assert_eq!(names, vec![Some(""), Some("")]); } } + +/// Every cognitive nesting slot is freed by the node that last reads +/// it, so the map is bounded by the traversal stack rather than the +/// tree (#1375). No metric value can tell the two apart — the retained +/// entries were dead — so the walk records whether any slot survived +/// it, and this asserts none did on each path that seeds one: a plain +/// nested-control-flow walk, the two overrides that write a child's +/// slot ahead of the walk (Python comprehension clauses, Tcl `try` +/// handler bodies), and an `exclude_tests` prune, which pops a seeded +/// node without computing or propagating it. +#[test] +#[cfg(all(feature = "cpp", feature = "python", feature = "tcl", feature = "rust"))] +fn walk_frees_every_cognitive_nesting_slot() { + use crate::spaces::compute::nesting_slots_retained; + use crate::test_support::metrics_verbatim; + use crate::{LANG, Metric}; + + const CASES: &[(LANG, &str, bool)] = &[ + ( + LANG::Cpp, + "int f(int a) { if (a) { while (a) { a--; } } else { return 1; } return 0; }\n", + false, + ), + ( + LANG::Python, + "def f(xs):\n return [x for x in xs if x for y in xs]\n", + false, + ), + ( + LANG::Tcl, + "try { risky } on error {msg} { puts $msg } finally { cleanup }\n", + false, + ), + ( + LANG::Rust, + "fn f(a: u8) -> u8 { if a > 1 { a } else { 0 } }\n\ + #[test]\nfn t() { if true { assert!(f(2) == 2); } }\n", + true, + ), + ]; + + for &(lang, source, exclude_tests) in CASES { + let before = nesting_slots_retained::observed(); + // Restricted to the one metric that seeds a slot, per + // `metrics_verbatim`'s own guidance (#1127): it is also the + // minimal selection that still enables both freeing sites, so + // nothing else can be what keeps the map empty. + let metrics = metrics_verbatim( + lang, + source.as_bytes(), + MetricsOptions::default() + .with_only(&[Metric::Cognitive]) + .with_exclude_tests(exclude_tests), + ); + assert!( + metrics.cognitive.cognitive_sum() > 0, + "{lang:?}: fixture must exercise cognitive, else no slot was ever seeded" + ); + assert_eq!( + nesting_slots_retained::observed(), + before, + "{lang:?}: a nesting slot outlived the walk" + ); + } +} From 6424a9838f45f3efd158bf10d6fdb410fea8542d Mon Sep 17 00:00:00 2001 From: Elijah Zupancic Date: Sun, 30 Aug 2026 14:47:42 -0700 Subject: [PATCH 2/2] docs(ci): scope the RSS rule to the in-flight term MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `--jobs` sizing guidance read as a process-wide bound, and it is one only where the destination does not retain finished results. `metrics --output ` sends every space over an unbounded channel and collects it after the walk, and structured stdout holds completed documents while an earlier file is still being analyzed — worst exactly when one large file sorts early, which is the shape this change was measured on. `--output-dir` writes per document and `bca check` reduces each file to its violations, so those two stay at the in-flight bound. State the formula as the in-flight term, name the destinations that add a result-set term, and point a memory-limited runner at `--output-dir`. --- CHANGELOG.md | 4 +++- big-code-analysis-book/src/recipes/ci.md | 19 ++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa469a4f4..cd1bc4405 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,9 @@ for historical reference. `-j 16`: 488 MB → 439 MB. No metric value moves. What remains is dominated by the parse rather than by the walk: a parse-only `bca dump` over the same files peaks at 25–70× the source size, which - the CI recipe now states as the `--jobs` memory sizing rule. + the CI recipe now records as the in-flight term in its `--jobs` sizing + guidance — alongside the destinations that add a result-set term on + top of it (`metrics --output `, structured stdout). ### Changed diff --git a/big-code-analysis-book/src/recipes/ci.md b/big-code-analysis-book/src/recipes/ci.md index 6c0974f53..6dfa34b79 100644 --- a/big-code-analysis-book/src/recipes/ci.md +++ b/big-code-analysis-book/src/recipes/ci.md @@ -869,11 +869,20 @@ Applies regardless of provider: macOS/Windows — so CI runners no longer need to thread `--jobs "$(nproc)"` through every recipe. `--jobs 1` remains a debugging knob, not a default. It is *not* - memory-aware: each worker holds one whole file in memory at a - time, and a parse-only run measures 25–70× the source size, so - peak RSS is roughly `jobs × 70 × the largest source file` in - the tree. A memory-limited container with many vCPUs should - pass an explicit `--jobs`. + memory-aware. Each worker holds one whole file at a time and a + parse-only run measures 25–70× the source size, so the + *in-flight* term is roughly `jobs × 70 × the largest source + file` in the tree — and `--jobs` bounds that term only. + Whether it is the whole story depends on the destination: + `--output-dir` writes each document as it is produced and + `bca check` reduces each file to its violations, so both stay + at the in-flight bound, while `metrics --output ` + collects every file's result and serializes it after the walk, + and structured stdout holds finished documents while an + earlier file is still being analyzed — which is worst when one + large file sorts early. A memory-limited container with many + vCPUs should pass an explicit `--jobs`, and prefer + `--output-dir` to `--output ` on a large tree. - **Always pass `--strip-prefix "$PWD/"` to `bca report markdown`** so the path column is identical across runners with different workspace paths. Without it the diff between two reports is