perf(metrics): bound the cognitive nesting map - #1377
Conversation
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1377 +/- ##
==========================================
- Coverage 98.47% 98.46% -0.01%
==========================================
Files 278 278
Lines 75812 75772 -40
Branches 75382 75342 -40
==========================================
- Hits 74653 74610 -43
- Misses 755 756 +1
- Partials 404 406 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| 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 |
There was a problem hiding this comment.
Location: big-code-analysis-book/src/recipes/ci.md:874
MEDIUM -- Scope the RSS sizing rule to output modes that do not retain completed results
The new formula is presented as generic CI guidance, but it is not a process-wide bound for every documented metrics destination. metrics --output <FILE> deliberately sends every FuncSpace over an unbounded channel and only collects/writes them after the walk (big-code-analysis-cli/src/commands/analyze.rs:159-184), so peak memory in that mode also grows with the full result set. Structured stdout has a similar extra term: OrderedStdout documents that its reorder buffer can degrade toward the whole tree when an early file dominates (big-code-analysis-cli/src/ordered_stdout.rs:28-38). Please qualify this as the parse/in-flight term (or scope it to --output-dir/check) rather than telling memory-limited CI users that lowering --jobs alone bounds RSS.
There was a problem hiding this comment.
Fixed in 6424a983. Verified both terms before editing, and both hold:
metrics --output <FILE>builds acrossbeam::channel::unbounded()and only drains it withrx.into_iter().collect::<Vec<_>>()afterrun_walkreturns (commands/analyze.rs:161-183), so the whole result set is live at once — the same shape as the perf(cli): diff --since round-trips both sides through temp JSON files #1116 regression, deliberate here.OrderedStdoutdocuments the degradation itself (ordered_stdout.rs:32-34): the buffer approaches the whole tree "when one early file dominates the run". Worth noting that is precisely the tree this PR measures — a 28 MBparser.camong 100k small files — so it is the live case, not a hypothetical one.
The bullet now states the formula as the in-flight term and says --jobs bounds that term only, then splits the destinations: --output-dir writes per document and bca check reduces each file to its violations, so both stay at the in-flight bound; --output <FILE> and structured stdout add a result-set term. It closes by pointing a memory-limited runner at --output-dir.
The CHANGELOG sentence that referred to this rule was making the same overreach by reference, so it now says "the in-flight term in its --jobs sizing guidance" and names the two destinations that add to it.
make pre-commit passes.
dekobon
left a comment
dekobon
left a comment
There was a problem hiding this comment.
Code Review: perf(metrics): bound the cognitive nesting map
Verdict: APPROVE WITH COMMENTS
| Severity | Count |
|---|---|
| BLOCKER | 0 |
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 1 |
Findings
| # | Severity | File:Line | Title | Category |
|---|---|---|---|---|
| 1 | MEDIUM | big-code-analysis-book/src/recipes/ci.md:874 |
Scope the RSS sizing rule to output modes that do not retain completed results | correctness |
Review passes executed
- Correctness and logic bugs
- Security review of the changed traversal state lifecycle
- Performance and scalability
- Test coverage
Files reviewed: 8
I did not find a metric-value or traversal-correctness regression in the slot reclamation itself. The only comment is on the new generic memory-sizing guidance, which omits existing output-buffer terms.
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 <FILE>` 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`.
1 participant
Fixes #1375.
What the peak actually was
#1375 was filed from a measurement with the attribution explicitly not
done, and two of its three hypotheses turned out to be wrong. The 5.1 GB
is not worker fan-out over ordinary files: the registry carries a dozen
tree-sitter-*crates whose generatedsrc/parser.cruns 8–28 MB andslips past the generated-file heuristic (the header opens with
#include "tree_sitter/parser.h", no marker). At-j 1the whole100k-file tree peaks at 1.31 GB — exactly the largest single file, so
nothing accumulates across files.
Splitting one 28 MB file's 1,265 MB by selection:
-j 1)dump— parse only--metrics cyclomatic/halstead/abc/nargs/nexits--metrics cognitive--output-dirSo ~720 MB is the parse and cognitive alone added ~540 MB. The
reorder buffer and the wire clone are ruled out by the last two rows
(
--output-dirhas neither and matches stdout);bca check, which hasno reorder buffer at all, peaked at 4.5 GB on the same tree.
MALLOC_ARENA_MAX=1gives 3.4 GB at 5× the wall time — real, butsecondary and not ours.
The change
NestingMapwas reserved atdescendant_countand never freed a slot,so it converged on one entry per node. Each slot is now freed by its
last reader, and the map grows from empty, so the live set is the
seeded-but-unvisited nodes — bounded by the traversal stack.
propagate_nesting_to_childrenreserves per sibling set so a widegenerated table still costs one allocation.
parser.c(28 MB),-j 1-j 16-j 1/-j 16make bench-scalingunchanged, all 27 probes linear.Why this needs its own guard
No metric value moves — the retained entries were already dead — so
nothing in the output can distinguish the fix from its absence. Two
guards, deliberately overlapping:
walk_frees_every_cognitive_nesting_slotreads thenesting_slots_retainedcounter over 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
tryhandler bodies),and an
exclude_testsprune. Both freeing sites were verified byreverting them individually and watching only this test fail.
debug_assert!generalises that to every walk the suite runs —twenty languages of corpus fixtures rather than the four the test
names. This matters because the freeing rule now has two owners, and a
third
continueadded later would otherwise leak in silence.Incidental
Cognitive::SEEDS_NESTINGandNode::descendant_countexisted onlyto size the reserve; removed with their tests.
parser_cache's module doc asserted a per-thread retention bound of"tens of KiB". The pinned tree-sitter caps those pools at fixed counts
(
TS_MAX_TREE_POOL_SIZE,MAX_NODE_POOL_SIZE) and the surroundingscratch arrays track parse depth rather than input size, so the figure
was unsupported in both directions. The doc now states what the source
actually shows.
--jobsmemory sizing rule:bcaiscgroup-CPU-aware but not memory-aware, and a parse-only run measures
25–70× the source size.
Deliberately not done
A
--max-file-sizegate, a memory-aware--jobs auto, a boundedOrderedStdout, and an allocator swap all address terms the measurementshows are small. The remaining ~720 MB on a 28 MB input is the parse
tree itself.