perf(mcp): build the cold-scan trigram index in parallel (~4.5x on the build phase) - #672
Merged
Merged
Conversation
…r a file edit Red on release/0.2.5828: the second findCallPath cannot resolve the newly added alpha -> beta edge because call_graph is never invalidated on the file-commit path, and its node_name/node_path slices borrow outline memory the re-index just freed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Explorer.call_graph was built once and never invalidated: the file-commit path refreshed deps/symbol-index/line-offsets but left call_graph and call_centrality pointing at the process-start snapshot, so codedb_callpath and the graph-distance rerank could not see edges added by an edit. Worse, CallGraph.node_path/node_name borrowed slices into outline symbol memory; re-indexing freed those outlines, dangling the name/path tables (use-after-free, silently-wrong under the testing allocator). Fix: (1) deinit+null call_graph and call_centrality in commitParsedFileOwnedOutline so the next graph query lazily rebuilds; (2) dupe node_path/node_name in ensureCallGraph and free them in CallGraph.deinit, unwinding partial arrays on every fallible early-return. call_centrality keys keep borrowing the stable self.outlines keys (node_path.items), preserving the snapshot-restore ownership invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#3 #6) Three behavior-preserving optimizations on the per-query search/rank path, identified by the perf audit and adversarially verified: #1 (mcp.zig, appendSearchSymbolNudge): the bare-identifier nudge did a full O(U+S) symbol scan (searchSymbols scans the whole symbol_index AND outlines every call) just to test existence. Switched to findAllSymbols — the same helper codedb_symbol uses: ensureSymbolIndex + O(1) symbol_index.get, with the outline scan only when !symbol_index_complete. Output byte-identical (the nudge only checks results.len and prints the query string). #3 (explore.zig, BM25 posting loop): dropped the doc_tf hashmap. doc_best_line already accumulates .count identically per posting, so df = doc_best_line.count() and tf = entry.value_ptr.count — one map instead of two, and the redundant per-doc .get() re-lookup is gone. Values bit-identical. #6 (explore.zig, tier-1 sort): the comparator hashed 4 strings + touched 2 atomics per comparison. Precompute (count, len) per candidate once in O(C) (mirroring tier0_order), then sort a struct-key array — identical predicate (count desc, len asc) and input order, so ranking is unchanged. Also stops the comparator corrupting ContentCache hit/miss stats. zig build test: full suite green (0 failures). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y blocks Every MCP tool result carries the assistant data block plus two audience:["user"] blocks (a colored summary + a guidance hint). These exist for interactive preview panes, but agent harnesses forward them straight into model context where they cost output tokens the model can't use — ~34% on a small result like codedb_symbol (115→75 tok), the very tool codedb tells agents to reach for first. A lean mode already existed (CODEDB_MCP_LEAN) but was off by default and undocumented, so agents paid the tax unless they knew the knob. Make the decision client-aware, keyed on clientInfo.name from initialize: - Agent harnesses (claude-code, codex, unknown clients) default LEAN. - Human-facing GUI clients (claude-ai / Claude Desktop) get the rich blocks. - Overrides: CODEDB_MCP_LEAN / CODEDB_MCP_RICH force globally (LEAN wins); CODEDB_MCP_RICH_CLIENTS=a,b extends the rich allowlist. Also fixes a latent use-after-free this surfaced: session.client_name borrowed a slice from the initialize request's parsed JSON, which is freed at the end of that dispatch iteration. It was only ever read during initialize before; reading it on a later tools/call (the new policy) dangled. Now duped into session-owned memory and freed in Session.deinit. Verified end-to-end (built binary, clean env): claude-code -> lean, claude-ai -> rich, CODEDB_MCP_LEAN=1 -> lean even for claude-ai, CODEDB_MCP_RICH_CLIENTS -> opt-in works. Unit test in test_mcp.zig; full `zig build test` green (testing allocator clean on the UAF fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#1) When codedb_search's query is a bare identifier that resolves to an indexed symbol, the nudge used to just say "go call codedb_symbol" — forcing a second round-trip (~85 tok response + the agent's own call emission) to learn where the symbol is defined. Now it surfaces the definition site(s) inline (path:line (kind), up to 3, "+N more"), reusing the O(1) findAllSymbols the nudge already ran. One call now answers "where is X defined?". Fewer round-trips = fewer output tokens, which dominate cost. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…irect (#658) The PreToolUse hook hard-blocked grep/rg/cat/head/tail/sed/awk/find the moment the codedb binary existed — unconditionally, even outside any repo, on unindexed dirs, and in ~/.claude. It fired before permission evaluation (overriding permissions.allow) and re-registered on every install. Blocking a command an agent then just routes around (python/sed) is friction, not adoption. Layered redesign (coercion last), fail-open by default: - Scope 1: only intercept when cwd is at/under a codedb-indexed root recorded in ~/.codedb/projects/*/project.txt. Degenerate "/" and bare-$HOME roots are skipped so an indexed home doesn't block all of ~. Outside an indexed repo, codedb has nothing to offer — the native tool is allowed. - Scope 2: if the command targets an ABSOLUTE path outside the repo (cat /etc/hosts, grep x /tmp/f), allow — codedb can't read it. Relative paths are assumed in-repo. - Redirect: when it does block, name the exact codedb substitute for THAT command (grep->codedb_search, cat->codedb_read, find->codedb_find/glob, ...) and state that native tools work outside the repo or with CODEDB_NO_HOOKS=1. - Opt-out: CODEDB_NO_HOOKS=1 disables the hook entirely. Verified across the behavior matrix (in-repo relative -> block+redirect; not-indexed -> allow; in-repo absolute-outside -> allow; bare-home -> allow; opt-out -> allow; non-listed command -> allow). Installer Python heredoc compiles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Consolidate the ~16 duplicated "path:line[: text]" render sites in mcp.zig behind one appendMatchLine helper (prefixes " "/"- "/"" and the scope suffix preserved; paths_only output byte-identical), and trim the match text to save agent output tokens: - strip leading indentation (the line number already locates the hit) - cap at 200 bytes with a UTF-8-safe cut + "…" Also apply the same trim to the default search path — renderPlainSearch in explore.zig (mcp's helper can't reach it), which the mcp-only consolidation missed. This is the highest-traffic path (a bare codedb_search query), where deeply-indented source lines were the bulk of the waste. Render-only: result set and order unchanged. Full `zig build test` green. The mcp.zig consolidation was produced by a 3-attempt swarm (winner's diff applied + independently re-verified); the explore.zig completion + measurement verification done here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # src/test_mcp.zig
…first) codedb_search's fast path (renderPlainSearchUncached) ordered tier0 files by hit count + basename prior, with no definition-awareness — so a file that merely *mentions* a symbol many times (tests, callers) outranked the file that actually DEFINES it when the def file's basename didn't match the query. The rich def-first ranking existed only in searchContentRanked, which codedb_search never calls. Fix: give tier0 a `defines` flag (via the existing fileDefinesSymbol) and add +20 to the rank prior for files that define the queried symbol — larger than the basename-match prior (+15), so a non-canonically-named definition still wins. Same result SET, better ORDER. Measured on a pinned, deterministic eval (frozen corpus + isolated $HOME, so the on-disk index can't drift between runs — the confound that sank the first attempt): 10 symbol queries, "def file ranks #1" improved 8/10 -> 9/10 with ZERO regressions (searchContentRanked: pos 3 -> #1). Full `zig build test` green; new regression test fails on baseline, passes with the fix. Follow-up: the one remaining holdout (renderPlainSearch) bails to searchContentAuto, a separate path where docs aren't demoted and this boost doesn't apply — tracked for a later pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Checks the ranking eval used for PR #665 into the repo so it's reproducible, not just in a scratchpad: - scripts/rank-eval.py — self-contained pinned A/B harness for codedb_search def-first ranking. Frozen corpus (copied out of the repo) + isolated $HOME so the on-disk index can't drift between builds; only the binary differs. Handles the temp-root guard (CODEDB_ALLOW_TEMP), async snapshot load (spaced pipe + pre-warm), and default/custom gold cases. - experiments/ranking/def-first-eval.md — methodology: what it measures, the snapshot-drift confound it kills, the gotchas it encodes, how to run it, the PR #665 result (def-file-#1 8/10 -> 9/10, zero regressions), and the known searchContentAuto holdout for the next pass. - experiments/ranking/todo.md — logged def-first as done (matches the rVSM entry). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… pass 2) Follow-up to the def-first ranking (#665). That floated the defining FILE to the top, but the render still showed the file's first hit LINE — often a comment or a call above the definition. Now, within a file that defines the queried symbol, the definition line(s) render before mere mentions. Implementation: after lineSpans computes the per-file spans, stable-move the spans whose line is a definition line (outline symbol named == query) to the front. Reorders the OUTPUT spans only — lineSpans' sorted input is untouched, so offset resolution is unaffected. Same result set, better in-file order. Verified on the pinned eval (scripts/rank-eval.py): def-file rank unchanged (9/10, no regression); qualitatively, `codedb_search bumpSearchGen` now leads with `explore.zig:2021: fn bumpSearchGen(...)` (the def) instead of a comment. Regression test in test_search.zig fails on baseline, passes with the fix; full `zig build test` green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scanBg (the MCP cold-scan path) built trigrams single-threaded: either inline per-file during the initial walk (when heads_match was false -- the common no-snapshot/no-disk-index case), or via the serial Explorer.rebuildTrigrams() (when a stale/partial disk index was found). A parallel builder, watcher.buildTrigramsFromCache, already exists and is used on the CLI cold-index path (bootstrap.zig) but was never wired into the MCP path, which blocks .ready. Route scanBg through the same builder: skip trigram indexing during the walk unconditionally, then build the whole index in one parallel pass over the already-populated ContentCache, write it to disk, and reload it as an mmap index via the existing adoptTrigramIndex path -- an exact mirror of the CLI cold path, including the disk round-trip (needed because buildTrigramsFromCache's lean insertBulkNew doesn't populate file_trigrams, so containsFile/pruneSkipTrigramLocked only work correctly once the index round-trips through the mmap format). The parallel build runs under explorer.mu's shared (read) lock for its entire duration, since worker threads read ContentCache content slices without copying -- this excludes any concurrent writer (codedb_edit, incremental file-watcher commits) for the whole section while still allowing concurrent search readers, unlike the CLI path which has no concurrent writers to worry about. Benchmark (ReleaseFast, 2000-file / ~90MB synthetic corpus, cold ContentCache already populated): serial rebuildTrigrams ~660-680ms vs parallel buildTrigramsFromCache (8 workers) ~147ms -- consistently ~4.5x across 3 runs. Added a result-identity test (test_search.zig) that builds the same multi-file corpus through both the old serial path and the new parallel+disk-round-trip path and asserts identical search results across several queries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Benchmark Regression ReportThresholds: 10.00% and 50,000 ns absolute delta
|
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e9b73389
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cpu_count = std.Thread.getCpuCount() catch 1; | ||
| const tri_workers: usize = @min(@as(usize, @intCast(cpu_count)), 8); | ||
| explorer.mu.lockShared(); | ||
| const tmp_tri = watcher.buildTrigramsFromCache(&explorer.contents, allocator, std.heap.c_allocator, tri_workers) catch null; |
There was a problem hiding this comment.
When a project has more files than max_cached can retain or exceeds the ContentCache byte budget, initialScan(..., true) has added every file to skip_trigram_files but explorer.contents contains only the retained subset. Building the replacement trigram index from that cache writes/adopts an index for only those retained files; the rest stay in skip_trigram_files, so tier 3 re-reads them from disk on every fall-through search, and the persisted trigram header's file_count will never match outlines.count() for warm reuse. The old cold MCP path indexed each file during the walk and covered all non-capped files regardless of ContentCache eviction.
Useful? React with 👍 / 👎.
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.
The second real 2×-class win from the perf audit — measured ~4.5× in ReleaseFast.
Problem
On a genuinely cold MCP start (no snapshot, no disk index),
scanBgbuilt the trigram index single-threaded:watcher.initialScan(..., skip_trigram=false)built trigrams file-by-file inline during the walk (under the exclusive lock), while a battle-tested parallel builder (buildTrigramsFromCache, used by the CLI cold path) already existed. (The audit pointed atrebuildTrigrams; investigation found that's only the narrower stale-disk-index case — the common cold path was the inline build.)Change (
src/background.zigscanBg)initialScannow always passesskip_trigram=true(matches the CLI cold path — no inline trigram build during the walk).buildTrigramsFromCache(workers =min(cpus, 8)), write to disk, then reload asMmapTrigramIndexandadoptTrigramIndex— reusing the disk-round-tripscanBgalready did.Correctness / safety
perf/a2: parallel buildTrigramsFromCache is result-identical to serial rebuildTrigramsbuilds a corpus both ways and asserts identicalsearchContentresults across 5 queries.explorer.mu.lockShared()for its whole duration (workers readContentCacheslices without copying), excluding concurrent writers (codedb_edit, watcher commits — both take the exclusive lock) while letting search readers through.insertBulkNewdoesn't populatefile_trigrams, so adopting as.heapwould blindpruneSkipTrigramLocked;MmapTrigramIndex.containsFiledoesn't have this issue, andwriteToDiskalready has thefile_trigrams.count()==0fallback the CLI path uses.Benchmark (ReleaseFast, 2000 files / ~90MB, 8 workers)
Consistently ~4.5× on the trigram-build phase (walk phase excluded, unaffected).
zig build testgreen;test-mcpstable across repeats.Stacks with #671 (A1, the writeToDisk remap). Produced by a Sonnet agent; independently re-verified (full suite + concurrency stability).
🤖 Generated with Claude Code