Repository navigation
Compiler perf: 2.9x faster cold compile (1.41s → 482ms empty project) - #3911
Conversation
Four independent fixes to the compile pipeline, found by profiling compile_empty_project (which compiles the full 55-file stdlib): 1. normalize: lhs==rhs fast path in is_same_normalized_type / is_subtype_of, and find_recursive_aliases rewritten as a single memoized tri-color DFS (O(nodes+edges) instead of a fresh DFS per alias). 2. emit Pass 1: read intrinsic-kind off the item tree instead of fully MIR-lowering every function just to inspect one field. Previously every function in the project was lowered twice (Pass 1 + Pass 4). 3. mir: package-shared DispatchCandidateCache memoizing interface_method_candidates_for across all functions lowered in a package. Key includes the function's generic_param_bounds - the only per-function input of the resolution family (verified by transitive self-field audit). 4. mir: LoweringContext no longer deep-copies all nine TIR inference tables into per-function merged maps; it now holds &'db ScopeInference views per FileScopeId and reads through tir_* point accessors. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds shared per-package dispatch caching and per-scope TIR point lookups to MIR lowering, threads the new cached lowering entry points through emit, and updates type normalization with identical-type fast paths plus shared memoized cycle detection. ChangesDispatch caching and TIR accessor refactor
Type normalization fast-paths and memoized cycle detection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant generate_project_bytecode_with_opt
participant lower_function_cached
participant LoweringContext
participant DispatchCandidateCache
generate_project_bytecode_with_opt->>lower_function_cached: pass package dispatch_cache
lower_function_cached->>LoweringContext: new_for_function(scope_inference, dispatch_cache)
LoweringContext->>DispatchCandidateCache: lookup DispatchCacheKey
alt cache miss
LoweringContext->>LoweringContext: compute candidates
LoweringContext->>DispatchCandidateCache: insert candidates
end
LoweringContext-->>lower_function_cached: lowered MIR
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_compiler2_tir/src/normalize.rs (1)
55-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the memoized cross-root cycle detection.
The per-alias→shared-
staterewrite is subtle (gray/black memoization reused across roots). A small#[cfg(test)]case exercising a diamond/shared alias graph — e.g.A → B,A → C,B → C → B— would lock in that all reachable aliases are classifiedrecursiveregardless ofaliaseskey iteration order, and guard against future regressions. As per coding guidelines ("Prefer writing Rust unit tests over integration tests where possible").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_tir/src/normalize.rs` around lines 55 - 74, Add a Rust unit test for find_recursive_aliases that exercises shared memoized DFS state across roots using a diamond-shaped alias graph such as A→B, A→C, and B↔C, then assert that all aliases reachable from the cycle are marked recursive regardless of HashMap key iteration order. Place the test under cfg(test) near find_recursive_aliases/has_cycle so it guards the gray/black state reuse behavior and prevents regressions in cross-root cycle detection.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@baml_language/crates/baml_compiler2_tir/src/normalize.rs`:
- Around line 55-74: Add a Rust unit test for find_recursive_aliases that
exercises shared memoized DFS state across roots using a diamond-shaped alias
graph such as A→B, A→C, and B↔C, then assert that all aliases reachable from the
cycle are marked recursive regardless of HashMap key iteration order. Place the
test under cfg(test) near find_recursive_aliases/has_cycle so it guards the
gray/black state reuse behavior and prevents regressions in cross-root cycle
detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b4b5bee9-c165-4f7a-90ab-017407be4d8a
📒 Files selected for processing (5)
baml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rs
The pub struct's doc linked the private LoweringContext::interface_method_candidates_for, which fails `-D rustdoc::private-intra-doc-links` in CI. Plain code formatting instead of an intra-doc link. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Results
cargo bench --bench compiler_benchmark -- compile_empty_project(median, 100 samples). The "empty" project compiles the entire builtin stdlib (55 files, ~9k LOC, 199 classes, 33 interfaces), so this benchmark measures the fixed cost every cold compile pays.Net: −66%, 2.9× faster. The wins compound because they attack the same hot path from four angles: the interface-dispatch machinery inside MIR lowering, which the profile showed dominating compile time (samply @2khz, symbolicated; see per-fix numbers below).
The four changes
1.
tir/normalize.rs: stop redoing alias-graph analysis per type comparisonis_same_normalized_type/is_subtype_ofget alhs == rhsfast path. Sound becausenormalizeis a pure function of(ty, aliases)andStructuralTy::is_subtype_ofis explicitly reflexive — equal inputs always producedtruethrough the slow path.find_recursive_aliaseswas a fresh DFS (with fresh HashSets) per alias, per call: O(A·(A+E)). It is now a single memoized tri-color DFS over the whole graph: O(A+E). Same semantics ("reaches a cycle" is a per-node property, root-independent, so memoizing across roots is sound).Why it wins: these functions sit at the innermost level of
match_ty_pattern, called per recursion level per dispatch candidate. They were 49%/40% inclusive of compile time.2.
emit/lib.rsPass 1: don't lower every function twicePass 1 called
lower_functionon every function in the project solely to check "is this an intrinsic?" for global-slot assignment; Pass 4 then lowered everything again to emit bytecode (lower_functionis not a Salsa query, so nothing deduped the two). Pass 1 now reads the same fact off the item tree (FunctionBodyDef::Builtin(kind)— the exact fieldlower_functioncopies verbatim intoMirFunctionKind) and gets the fq name fromdef_to_item_ref. Full MIR lowering now runs exactly once per function.Why it wins: MIR lowering was 61% inclusive of compile time, and half of it was the duplicate.
3.
mir/lower.rs: memoize interface-dispatch candidate resolution per packageinterface_method_candidates_forrescans every implementor of an interface — running type-pattern matching and normalization per candidate — at every call site. The same questions recur constantly (everyforloop asks the sameIterator.nextquestion). NewDispatchCandidateCache(Rc<RefCell<FxHashMap>>), one per package, shared across all functions the emit driver lowers via newlower_function_cached/lower_let_body_cachedentry points (existinglower_function/lower_let_bodysignatures unchanged — they use a private fresh cache).Correctness: a transitive audit of the 17-method resolution family showed its only inputs are the Salsa db, package-level
PackageLoweringData, the request args, and the function'sgeneric_param_bounds— so the cache key is (interface, type args, assoc bindings, method, receiver ty, sorted bounds), scoped per package.Why it wins: candidate resolution + dispatch-switch emission were 33%/36% inclusive; they drop to ~8%/9% after this change.
4.
mir/lower.rs+tir/inference.rs: read TIR inference through views, not copiesLoweringContext::new(andnew_for_let) eagerly deep-copied all nine TIR inference tables — expr types, pattern types, resolutions, call plans, path types, coercions, exhaustive-match marks — out of the Salsa-cached per-scopeScopeInferencevalues into fresh per-function merged maps, re-keying every entry. Per compile, the whole project's inference output was cloned, re-hashed, and dropped. That was ~17% of compile time in pure memory churn, and it's a materialized-view anti-pattern: the source data is already resident, immutable, and'db-stable behindreturns(ref).The context now holds
scope_inference: FxHashMap<FileScopeId, &'db ScopeInference<'db>>(function scope + descendants — exactly the range the old merge covered, so absence semantics are preserved for the callers that key behavior on a missing entry) and reads throughtir_*point accessors that dispatchMetadataScope::Body→ body tables,::ParameterDefault→ default tables.tir/inference.rsgains the missing point accessors (function_coercion,parameter_defaults(), and point lookups onDefaultParameterInference). All 94 read sites converted; the nine maps were verified strictly read-only after construction before the change.Architecturally this also makes
LoweringContextconstruction cheap and mostly pure — a prerequisite for eventually makinglower_functionitself a tracked query (LSP incrementality) without dragging a copy layer along.What's deliberately NOT in this PR
Follow-ups identified by the post-change profile (remaining 482ms ≈ salsa machinery 32%, normalize cluster ~36%, TIR inference ~23%, SipHash string-hashing leaf 20%):
ResolvedAliases.recursiveset throughis_same_normalized_type(it's still recomputed on the non-equal path),registry_dispatch_target_for_concrete_receiver(uncached sibling of change 3),resolved_aliases_for_package,Validation
baml_testsintegration suite green (2,600+ tests incl. all bytecode/MIR snapshots).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance
Bug Fixes