diff --git a/CHANGELOG.md b/CHANGELOG.md index fe974d69b..cd1bc4405 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,26 @@ 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 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 - 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..6dfa34b79 100644 --- a/big-code-analysis-book/src/recipes/ci.md +++ b/big-code-analysis-book/src/recipes/ci.md @@ -868,7 +868,21 @@ 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 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 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" + ); + } +}