Perf: batch usage-graph declaration and reference resolution - #16
Closed
DavidBakerEffendi wants to merge 9 commits into
Closed
Perf: batch usage-graph declaration and reference resolution#16DavidBakerEffendi wants to merge 9 commits into
DavidBakerEffendi wants to merge 9 commits into
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. (cherry picked from commit 3cc293d2202bbc37ccced570a97b3e2c0201511d)
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). (cherry picked from commit d6ea3be15bbb89041e6641eae1f63b2a0d83414a)
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. (cherry picked from commit 5aa191d1b750a5f59bbf4fb6236b79e5833a7916)
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. (cherry picked from commit 6ac0c54323e160e2731dd961a8c9bd894f365292)
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. (cherry picked from commit c9bf7a645711dcade83e74685eaebed0638abb04)
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. (cherry picked from commit 5a0491b6022fd5528f3e340dacb0fa67f8c305fe)
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. (cherry picked from commit 174666ba13341b508a3d5e1cde3d5d9cba5b6df5)
|
Two more commits landed on the fork branch after this port: sontek/bifrost#2 now has 9 commits instead of 7.
Both target the same shape of problem as the rest of this port: work that gets redone once per ambiguous target instead of once per underlying file or language, which a heavily build-tag-duplicated package like golang.org/x/sys/unix hits hard (460 of 911 ambiguous targets on the k8s reproduction share a near-identical candidate file set). Commit 8 measured a clean before/after on a held-fixed corpus (12m49s to 11m44s); commit 9's phase-level numbers were the best of any run but total wall-clock time didn't show a clean improvement across repeated runs, which looks like machine variance layered on a real but partial fix, so that one is not attached to a specific percentage. Full details in the PR body and the issue thread: #15 |
Contributor
Author
|
Retargeted to the private repo: BrokkAi/bifrost-dev#2974 (built from sontek's current branch, including the two follow-up commits noted above). This repo's master is projection-only. |
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
2 participants
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.
Description
Ports the seven-reference fix from @sontek (sontek/bifrost#2) onto the current master projection, preserving the authorship of each upstream commit. The usage-graph path now parallelizes whole-workspace declaration enumeration, warms definition identities in batched relational reads, reuses batched import reads, selects the cached candidate-file provider when no real cancellation deadline exists, and resolves ambiguous-edge targets concurrently without changing typed incompleteness semantics.
Fixes #15
Thanks to @sontek for diagnosing the regression, implementing the reference solution, and reporting the Kubernetes-scale performance evidence.
Key Changes
Touch Points
.bifrost/suppressions.jsoncrates/bifrost-analysis/src/analyzer/analyzer_definition_lookup.rscrates/bifrost-analysis/src/analyzer/usages/crates/bifrost-analysis/src/searchtools/scan_usages.rscrates/bifrost-analysis/src/analyzer/Validation
Verified locally on Rust 1.97.1 at
a2c797404c52a4b9f75510bac621ccdc129be0f1:cargo fmt --checkcargo nextest run --max-fail 100 -p brokk-bifrost-analysis --lib: 2413 passed, 118 skipped, 0 failed. No Go or JVM environment-only failures appeared locally.Commit
e44dbd363c3eba1ece0aa3d6b6013b5621ac17f6removes only one now-orphaned suppression identified by the policy CI; it does not change Rust behavior.Upstream performance results are Sontek's reported evidence, not independently reproduced here: before the fixes, unrooted Kubernetes-scale
usage_graphdid not complete (killed after 53+ minutes, and again after 4+ hours); after the fixes, it completed in approximately 27 minutes. His analysis-crate lib run reported 2338 passed and 4 environment-only failures caused by missing Go 1.24 and javac/jar tooling.