Add memory-aware concurrent indexing for large repositories - #1925
Add memory-aware concurrent indexing for large repositories#1925zhiyuzhang001-a11y wants to merge 7 commits into
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
f7032c5 to
1e15c63
Compare
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
Signed-off-by: Zhiyu <zhiyuzhang001@gmail.com>
|
Read in full and routed for a maintainer fit decision. Since the only thing on this thread so far is the automated acknowledgement, here is honestly where it stands rather than silence. CI is green and the validation you supplied is unusually thorough for a first contribution — ASan/UBSan focused suites, the full local suite, and named downstream results. That is not what is holding it. What needs a maintainer ruling is that this is three separable changes in one PR, and one of them is a one-way door:
Each of those is reviewable on its own evidence; together, a reservation about any one blocks all three. That is why the split matters and it is not a formality — but I am not going to ask you to do the splitting work before the direction call on (3) is made, because the answer changes what the split should look like. One thing that would genuinely help the decision, if you want to write it while this is queued: what does the scheduler do when the budget is wrong — too low, or too high on a machine it mis-measures? Failing open, failing closed, and degrading to sequential are three different products, and which one it is matters more than the admission arithmetic. I will come back with the ruling rather than leaving this to age. |
|
The maintainer ruling is in. Short version: please split this into three PRs, and the third one needs to work on all three platforms before it can be reviewed. Split into three
What (3) needs first: cross-platform measurementThe admission logic rests on DIR *directory = opendir("/proc");
if (!directory) {
return false;
}…then a walk of
Failing open is the right choice over failing closed — nobody wants indexing blocked because a probe is unavailable. The problem is that the result is a scheduler which ships its code, its five environment knobs and its maintenance cost to all three platforms while protecting only one, and does so invisibly: there is no signal at runtime that the memory half is inert. So (3) is reviewable once the RSS probe either works natively on macOS ( To be honest with you about scope: that is a substantial piece of platform work, and it is entirely reasonable to decide it is more than you want to take on right now. Splitting (1) and (2) out means neither is held hostage to that decision. One design question for whenever (3) comes back
Credit where it is dueThe validation you supplied is well beyond what a first contribution usually carries: ASan/UBSan focused suites, the full local suite, and named downstream results rather than "tests pass". The None of the above is a rejection of the idea. It is a request to let the two uncontroversial pieces land on their own evidence while the third gets the platform work it needs. |
DeusData
added
enhancement
Fixes #2010. Refs #1997. Indexing a 14k-file TypeScript repository holds 13.3 GB resident at post_extract on Linux aarch64, and about 3.1 GB of it is AST traversal scratch that no CBMFileResult points at. ts_nstack_init cut its stacks from ctx->arena, which is result->arena. In the parallel path that arena is stored into ec->result_cache[file_idx] (pass_parallel.c:963) and freed only after parallel_resolve and the infra passes (pipeline.c:1324), so a structure written for a one-file lifetime is in fact held for the whole index, once per file, concurrently. The header claimed otherwise and was wrong: "freed when the arena is destroyed at end of file extraction". The largest instance is channels. CHAN_STACK_CAP is 4096 and sizeof(TSNode) is 32, so scan_string_consts_js (extract_channels.c:104) and extract_channels_js (:376) take 128 KB each, per JavaScript or TypeScript file. Across 12,673 such files that is 3,168 MB, and it produced 23 Channel nodes out of 164,684. CBMExtractCtx gains a scratch arena, created and destroyed by the cbm_extract_file_ex call that builds the context. Nothing else changes about where anything lives: ctx->arena still owns every string a CBMFileResult points at, and ctx->scratch owns only traversal stacks, which nothing points at. Nothing a CBMFileResult points at moved. A context built without a scratch, and a file whose scratch arena fails to allocate, both fall back to ctx->arena, which is exactly the behaviour that shipped before this change, so no path can be handed a NULL arena and silently lose nodes. The signature change is what makes the mistake unrepresentable. ts_nstack_init takes the context rather than an arena, so handing it ctx->arena is an incompatible pointer type; push and push_children read the arena the stack recorded, so a stale argument is an arity error. Both fail under -Werror rather than silently retaining memory. The 55 init sites are NOT one uniform sed. 29 pass ctx->arena and 26 pass a local alias, and that alias is shared with result allocations: in parse_zig_imports (extract_imports.c:1093) the same `a` feeds cbm_node_text, strip_quotes, path_last and cbm_imports_push (extract_imports.c:1101, 1106, 1108, 1109). Rebinding it would have moved import names and module paths into memory that dies with the call while the result kept pointing at them. Each site was edited on its own and no alias was rebound. Re-derive the set with: grep -rn 'ts_nstack_init' internal/cbm/extract_*.c grep -rn 'ts_nstack_push' internal/cbm/extract_*.c parse_lisp_imports used its alias only for the stack, so the alias goes too. push_nested_class_nodes and push_class_body_children took an arena purely to reach the stack, so they take the context now; the one call site passes ctx. Two measurements, from two different builds, and they are not the same number. A macOS census build, which instruments every cbm_arena_alloc and sums the blocks behind every live CBMFileResult arena, reports the retained per-file arena set dropping from 11,226 MB mapped to 5,680 MB, and blocks from 48,045 to 34,186. Those are mapped bytes, not resident: that build logs mem.allocator.bound_populations_only, so untouched block tails never commit. The resident figures come from Linux aarch64, gcc 13.3, static build, where the build logs mem.allocator.owned classes=all and mapped equals resident. Five runs per side on the full 13,999-file tree: mem.collect post_extract rss_mb 13293 -> 6752 (-49.2%), mem.phase peak_mb 14840 -> 8565 (-42.3%). nodes 164684, parse_partial 280 and registry entries/defines/imports 787954/804999/100027 identical on all ten runs; edges 835869-835874 against 835867-835881, overlapping ranges, so run to run noise. Wall clock means 35651 ms against 30502 ms, but the ranges overlap and the host had other load, so the only supportable claim is no regression. ObjectScript Studio Export on a small fixture: nodes 18, edges 25, both sides. Scratch block size is 512 KB, chosen from measurement. arena_grow fires on one file in 12,000 at both 512 KB and 1 MB, and constantly at 256 KB, where the two channel stacks alone are exactly 262144 bytes. 512 KB therefore costs half the resident block per worker for the same growth behaviour, and stays under MI_LARGE_MAX_OBJ_SIZE in the vendored mimalloc, which is MI_LARGE_PAGE_SIZE/8, exactly 524288 bytes (types.h:426, with MI_ENABLE_LARGE_PAGES defaulting to 1 at :115 and not overridden by this build), so a 512 KB block is still bin-allocated from a 4 MiB large page. Growth is not free at that size for the same reason: arena_grow doubles to 1 MiB, above that bound and therefore a singleton OS allocation. One file in twelve thousand pays it, which is why it is accepted. The largest single file's scratch high water mark on this corpus is 33.5 MB, now transient rather than retained. Tests. Nothing in the suite asserted on result->arena size, block count or total_alloc after cbm_extract_file, which is why this went unnoticed for the life of extract_node_stack.h. traversal_stack_not_in_result_arena_issue2010 pins the byte budget: extracting "export const x = 1;" as TypeScript charged 365984 bytes to the result arena before and charges 87456 after, a difference of 278528, exactly the two 4096-entry channel walks plus the 512-entry ES import walk at 32 bytes per TSNode. Of the 87456 that remain, 7680 is the defs item array at GROW_ARRAY's starting capacity of 32 times sizeof(CBMDefinition) 240 and the other 79776 is everything else the extraction interns, none of it traversal scratch; the 128 KB bound is therefore above the new figure with room and a factor of four below the old one. extract_traversal_stacks_come_from_ctx_scratch_issue2010 in test_mem.c pins the lifetime, since a byte budget alone would also be satisfied by shrinking CHAN_STACK_CAP: it builds the extraction context directly over two arenas it owns, runs cbm_extract_channels, and asserts the scratch took at least the 262144 bytes of the two walks while the result arena did not. extract_c_macro_hidden_call_survives_preprocessed_pass_issue2010 is not a scratch test. pp_ctx carries ctx->scratch so every context in the file is uniform, but nothing reads it there: pp_ctx reaches only cbm_extract_unified and cbm_run_c_lsp, and neither extract_unified.c nor anything under internal/cbm/lsp/ includes extract_node_stack.h, so no traversal stack is built on that path today. The test guards the macro-expansion path itself, which had no assertion on a call that exists only after expansion. ObjectScript composite extraction is already covered by pipeline_objectscript_export_preserves_calls_sequential_parallel and iris_export_xml_multi_class, so no new test is added there; a per-call arena is correct on that path by construction, since each call owns and destroys its own. Local: scripts/test.sh, 7558 passed, 0 failed, 8 skipped, 139 suites. Alternatives considered Thread-local scratch arena, reset per file, with a depth guard for nesting. Rejected on three counts. This codebase never uses thread-local storage for owned bulk memory; every existing _Thread_local here is a small cache, a guard or a log buffer, and every per-worker resource is a field in a context struct. It would have needed cbm_arena_reset, which has no production caller and a documented subtlety with grown block sizes (arena.c:213-217). And extraction threads are created per cbm_parallel_for call and joined (worker_pool.c:71,88) with no thread-exit hook in this project's own code, so the arena would have leaked once per thread per dispatch and turned the leak lane red. Save and restore mark and release on the arena. Rejected because a per-call arena already makes nesting correct by construction, and a mark type plus two functions would have to be added to both arena headers. Per-walk arenas, one per traversal rather than one per file. Rejected on cost: 55 sites would each need an explicit span, for the same bound. Right-sizing the initial capacities alone. CHAN_STACK_CAP at 256 rather than 4096 would cut those two sites from 3,168 MB to about 198 MB, which is real, but it tunes the constant while the lifetime defect remains at all 55 sites, and growth is not free: 256 doubling to 4096 abandons 122,880 bytes, and one file in this corpus took 33.5 MB of stacks. The cap and the scratch block size are coupled, which is a further reason to change one at a time. Worth doing separately. Routing to a scratch inside extract_node_stack.h while keeping the arena parameters. Rejected because a parameter that is ignored lies about the API, and the alias hazard above would stay a review obligation instead of a compile error. Freeing the per-file result arena earlier. Rejected: the results are read by parallel_resolve and the infra passes. That is the retention half #1925 addresses, and this change is independent of it. Signed-off-by: Daniel Ochoa <daniel.ochoa@closedloop.ai>
2 participants
Summary
Motivation
This enables multiple project-scoped MCP clients to share the Provider safely. Small daily repositories can run concurrently; large jobs are admitted according to measured source bytes and memory budget instead of starting without a global bound.
Validation
maingit diff --checkpassesNo release or binary distribution is included in this PR.