perf: fix usage_graph never completing on large workspaces - #2
Draft
sontek wants to merge 10 commits into
Draft
Conversation
WorkspaceUsageCatalog::build_with_cancellation hydrated every analyzed file's declarations sequentially, one file at a time. Extract the per-file enumeration into declarations_for_file and run it across files with rayon's par_iter, used by both the unrooted build path and the existing rooted build_for_files path. Confirmed with a live sample on a large monorepo: 100% CPU (single core) before, 1000%+ CPU (fully parallel) after, for this phase.
AnalyzerDefinitionLookup resolved one name at a time against the relational store: an exact-name attempt, then (on a miss) an identifier-candidate fallback, each its own round trip. On a large workspace resolving thousands of names this was thousands of sequential round trips. Add prefetch_fqn_in_language, which batches many names' exact-name lookups into one relational query, then batches every miss's identifier-candidate fallback into a second query, writing results into the existing per-name cache that fqn_in_language already reads. Four cross-package names went from 8 round trips (one exact attempt plus one fallback per name) to 2 (one batched round trip per phase).
ImportAnalysisProvider::import_infos_for_files defaults to None, which forces every caller of the batch path (candidate discovery's importer scan, among others) back to one import_info_of call per file. TreeSitterAnalyzer already exposes a real batched implementation (bulk_import_infos, backed by one relational read for every cache-miss file), and Python's provider already forwards to it, but Go, C++, Kotlin, Ruby, and Rust never did. Forward all five to the existing bulk_import_infos, matching the pattern each of them already uses for file_dependency_facts_for_files right above it.
references_to_edges hardcoded its candidate-file provider to None, which routes candidate discovery through find_direct_importers_with_ cancellation: a per-candidate, uncached workspace-wide importer scan that exists specifically so a caller with a real cancellation deadline can bail out mid-scan instead of being forced through one uninterruptible reverse-import-index build. A caller whose cancellation token can never actually fire pays that scan's full, uncached cost on every call for a protection it will never use. Extract the existing body into references_to_edges_with_provider, which forwards an explicit CandidateFileProvider through to query_with_provider_and_source_budget instead of hardcoding None. references_to_edges becomes a thin wrapper that keeps passing None, so its one existing caller (inverse-edge derivation, which does carry a real deadline) is unaffected. Add a test-only call counter to find_direct_importers_with_cancellation so a caller that switches providers can assert it stopped taking the uncached path, and make the candidates module visible within the crate so that counter is reachable from tests elsewhere.
Two batches of methods landed on IAnalyzer / AnalyzerTestHooks (relational_definition_batch_call_count_for_test and its reset, definition_candidates_query_count_for_test / definition_prefetch_batch_count_for_test and their resets, and IAnalyzer::prefetch_definitions) with default no-op implementations, but a hand-written per-language wrapper's own impl block does not auto-inherit a new trait method it does not explicitly list -- it silently falls back to the trait's default rather than forwarding to its inner TreeSitterAnalyzer. Only Rust and C# already forwarded the query-count pair; nothing forwarded prefetch_definitions. Add the missing forwarding to all twelve language wrappers and to MultiAnalyzer's own delegate-summing implementations, so a counter read through &dyn IAnalyzer reflects what actually happened underneath regardless of which language backs it.
ambiguous-edge resolution usage_graph's ambiguous-edge resolution had three separate O(N*M) patterns that only show up at whole-workspace scale: - The exact-layer catalog it builds against declarations was rebuilt from scratch on every iteration of the depth loop, even though declarations only grows within a run and the catalog only needs rebuilding when it actually has. Track the length it was last built from and skip the rebuild when nothing changed. - Resolving each ambiguous endpoint name scanned the full declaration list once to check whether it existed locally, then scanned it again to collect the matching units. Replace both scans with one HashMap<(ecosystem, fq_name), Vec<CodeUnit>> built once. - The get_definition fallback for occurrences without an exact target called resolve_definition_batch_with_source once per ambiguous edge, and each call built a fresh DefinitionBatchContext with empty tree and source caches -- a file referenced by many ambiguous edges got its AST re-parsed once per edge. Restructure the loop into three passes: run today's fast-path and structural/inverse scans unchanged, deferring any site that falls through to a per-file queue; resolve every file's queued sites in one batched call each; then run today's per-site finalization a second time using the now- complete evidence. On a small fixture, four fallback sites sharing one file collapse from four calls to one. Also switch usage_graph's own ReferenceEngine calls to references_to_edges_with_provider(..., Some(&ImportGraphCandidateProvider::new()), ...): this scan never carries a real cancellation deadline, so the interruptible importer scan the default path exists to protect buys nothing here, while the cached reverse-import-index path (already used elsewhere) does real work only once per file instead of once per ambiguous target. Measured on a live Kubernetes-scale reproduction (BIFROST_TIMING): this run had reached ~275s of CPU time in 3 of these functions before these commits, spread across a run that never completed in over an hour; after this branch's fixes the same functions no longer show up in that profile, and the same request completes in ~27 minutes end to end for a request that previously never returned.
usage_graph()'s 935 ambiguous targets (declarations sharing an fq name) were resolved one at a time: each target's fast-path scan plus its exact-reference-engine scan ran sequentially, even though no target's work can collide with another's (every site key embeds the target's own name). On the k8s reproduction this loop was ~24.5% of total runtime with a heavily skewed cost distribution. Select the ambiguous targets first (a cheap sequential pass that keeps today's exact "first edge in BTreeMap order wins" semantics for a name shared by several edges), hoist the one-time structural scan out of the loop entirely, then run each target's resolution in parallel on the existing HEAVY_SCAN_POOL rayon pool and merge results back sequentially. Also drops the now-redundant endpoints_by_edge map (its value never depended on from_name) in favor of endpoints_by_target, keyed only by (ecosystem, to_name). Full test suite: 2338 passed, same 4 pre-existing environment-only failures (missing Go 1.24 toolchain, missing javac/jar) as before. Extended the existing ambiguous-edges test from two to three parallel targets to catch a merge bug that would only surface with more than two workers.
…ared default code path The ambiguous-edges regression test asserted a global AtomicUsize count of find_direct_importers_with_cancellation calls to prove usage_graph's candidate discovery avoids the slow, uncached importer scan. That function is also the workspace's shared default candidate-discovery path (dead_code_smells and five of candidates.rs's own tests call it directly), so the counter raced with unrelated tests running concurrently in the same test binary and flaked under cargo test's default parallelism -- confirmed by 15/15 clean passes in isolation (--test-threads=1, this test only) against 2 failures in 3 full-suite runs. The property itself needs no runtime count: ImportGraphCandidateProvider ::find_candidates always calls find_import_graph_candidates with scope: None, which forces cancellation: None and therefore the fast referencing_files_of path -- there is no code path from there to the slow scan. Documented that guarantee in the test instead of asserting it empirically, and removed the now-unused counter plumbing. Full suite: 2412 passed, same pre-existing environment-only Go-toolchain failure, across 5 consecutive clean runs.
sontek
force-pushed
the
fix/batch-relational-definition-lookups
branch
from
September 3, 2026 17:00
174666b to
08d106a
Compare
…s targets build_go_graph_with_edge_index re-read and, on a text match, re-parsed every candidate file from scratch on every call. usage_graph's ambiguous-edge resolution calls it once per distinct ambiguous target name, and a workspace with heavy per-platform build-tag duplication (golang.org/x/sys/unix's many _linux.go/_darwin.go/... variants of the same function name is the extreme case) makes hundreds of distinct targets share a near-identical candidate file set -- on the k8s reproduction, 460 of 911 ambiguous targets share an identical 335-file set, and many of those files reference more than one of the 460 functions, so the same file's tree was rebuilt from scratch by every target that touched it. Adds two KeyedPoolSafeMemo caches to GoEdgeIndex, which already lives for the analyzer's lifetime: one for raw source text (backing the existing identifier/owner text prefilter, so a non-matching file still costs one cached read instead of one read per target) and one for the resulting parsed tree (populated only for files that pass that prefilter, preserving the original lazy-parse behavior for the common, low-sharing case). Both are shared by every caller of build_go_graph_with_edge_index, not only the ambiguous-edge path. Measured against the same Kubernetes-scale reproduction, holding the corpus fixed to isolate this change: go_query_graph::build and parse_candidates collectively dropped from ~2.47M ms to ~25K ms summed duration (98%), and total usage_graph wall time dropped from 12m49s to 11m44s (8.5%). The gap between those two numbers is real: most of graph_find_usages's remaining cost is the per-(file, target) semantic scan (binding resolution, owner matching, local inference), which is not redundant the way re-parsing was -- each (file, target) pair is scanned exactly once regardless, so it is not fixed by this change. Full suite: 2412 passed, same pre-existing environment-only Go-toolchain failure, both before and after.
… candidate declaration find_import_graph_candidates calls analyzed_files_for_language once per source file of every declaration it examines -- once per ambiguous target's every overload, on a workspace with heavy same-name duplication (golang.org/x/sys/unix again: 460 of 911 ambiguous targets on the k8s reproduction). That free function's only implementation fanned out across every language delegate, sorted and deduped the combined result, then filtered it back down to one language on every single call (issue #1738's shape, one call site further out): live stack sampling during a benchmark run showed this sorting code actively executing across many concurrent worker threads at once. Promotes analyzed_files_for_language into a CodeUnitIndex trait method, defaulting to the previous behavior for any analyzer that doesn't override it, and gives MultiAnalyzer a cached override that goes straight to the delegate owning the requested language -- skipping every other language's files rather than fanning out and filtering them away -- and memoizes the (already sorted, per-language) result so the same unchanging list isn't rebuilt and re-sorted on every one of thousands of calls. The memo shares snapshot_caches' lifetime: valid as long as the snapshot is, rebuilt fresh whenever a new one is. Also fixes usage_ecosystem_files's one caller, which called analyzer.analyzed_files() directly (the same whole-workspace fan-out, bypassing analyzed_files_for_language entirely) to then filter by ecosystem instead of language, and the JS/TS candidate-discovery special case, which had the identical pattern. Full test suite: 2412 passed, same pre-existing environment-only Go-toolchain failure, both before and after. Measured against the same Kubernetes-scale reproduction, holding the corpus fixed: analyzer::analyzed_files.fan_out calls dropped from 11,758 to 2, and the parallel ambiguous-edge phase's own summed cost and worst single straggler both improved to their best measurements across every run this session. Total wall-clock time across repeated runs stayed within a ~45s band with no clean monotonic trend, which looks like run-to-run variance on a shared, non-isolated machine layered on top of a real but partial fix -- recorded here rather than claimed as a specific percentage.
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.
Fixes usage_graph's whole-workspace path, which never completed on a Kubernetes-scale monorepo (killed at 53+ minutes with no result, then again past 4 hours in a later diagnostic run). Nine commits, in order:
Measured against the original Kubernetes-scale reproduction: never completed before any of this, completed in about 27 minutes after commits 1 through 6, then about 23 minutes with commit 7 added.
That reproduction's checkout was lost partway through this work, so commits 8 and 9 are measured against a freshly rebuilt equivalent-scale Go corpus instead (also kubernetes/kubernetes, a different commit), holding that corpus fixed. Commit 8 took it from 12m49s to 11m44s. Commit 9's phase-level numbers are the best of any run this session (the parallel ambiguous-edge phase's own cost and its worst single straggler both dropped further, and a live-sampled hot path collapsed from 11,758 calls to 2), but total wall-clock time across repeated runs stayed within a roughly 45-second band with no clean trend, which looks like run-to-run variance on a shared, non-isolated benchmark machine layered on top of a real but partial fix. Recording that honestly rather than claiming a specific percentage for commit 9.
Full test suite passes throughout (cargo test -p brokk-bifrost-analysis --lib): 2412 passed, the same pre-existing environment-only failure (missing local Go 1.24 toolchain) both before and after.
Refs BrokkAi/bifrost#15