Repository navigation
Conversation
On canary the CST→AST lowering (baml_compiler2_ast::lower_file) is a plain, untracked function that six consumers each re-ran from scratch: HIR's file_semantic_index, PPIR's file_semantic_index, ppir_expansion_items, the two project-wide expansion-map collectors (collect_block_attrs / collect_alias_bodies), and the lsp2 check pass. Repeated CST traversal was ~31% of cold-compile CPU on the test corpus. Changes: - Add a salsa-tracked baml_compiler2_hir::file_ast(db, file) query that performs lowering once per file and shares items + lowering diagnostics + env var refs. Point all six consumers at it. - PPIR file_semantic_index now delegates to HIR's file_semantic_index when a file has no expansion items: the post-expansion index is byte-for-byte the pre-expansion one for such files, so rebuilding it was wasted work (this also dedups scope identity, cutting infer_scope_types executions). - Make PPIR function_body a tracked query returning Arc<FunctionBody> so MIR's per-call-site body fetches don't re-clone the ExprBody arena each time. Re-derived against current canary; reference: PR #4016 (perf/compiler2-cold-compile). Co-authored-by: Cursor <[email protected]>
…s_subtype_of assumption fast path dispatch_target_for_concrete enumerated every impl block in the package closure for every method call and field access, only to conclude "no impl provides this member" for the overwhelmingly common plain member. Collect all interface-declared method names (own package + dependency closure) once per package into PackageLoweringData and answer that case with a single hash lookup. The set is package-wide, so any name declared by any reachable interface still takes the full enumeration — a pure fast path. baml_type::normalize::is_subtype_of ran the co-inductive assumption bookkeeping (deep clone + full-tree hash of the (lhs, rhs) pair) on every recursive step. Only the expanding arms (mu-unfolding, type-variable / associated-projection bound lookup) can revisit a pair; purely structural arms descend into strictly smaller subterms, so the bookkeeping is now restricted to the expanding arms via an is_subtype_of_inner split (termination argument documented at the split). Re-derivation of items 12 and 7 from the cold-compile audit (#4016) against post-#4032 canary, where equivalence goes exclusively through baml_type::normalize. Co-authored-by: Cursor <[email protected]>
… per diagnostic batch Two independent wins for cold / wall time, re-derived from the compiler2 cold-compile audit (#4016): - baml_cli now sets mimalloc as its global allocator. The compiler workload is dominated by small short-lived allocations (Ty trees, Vecs, SmolStrs); system malloc measured ~35% of remaining single-threaded CPU in the original audit. - render_diagnostics builds the ariadne SourceCache (a line index over every project file) once per batch instead of once per diagnostic, threading it as &mut through render_ariadne / render_report_to_string. Rendered bytes are unchanged, so cached diagnostics from the #3924 disk cache replay identically (BAML_CACHE_VERIFY verified). Co-authored-by: Cursor <[email protected]>
…cold-compile-reland
…er' into perf/cold-compile-reland
…r' into perf/cold-compile-reland
Co-authored-by: Cursor <[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.
|
This was referenced Jul 16, 2026
🏎️ Performance benchmarks are running for this PRCodSpeed perf benchmarks were triggered because this PR opted in. Results will appear in the CodSpeed check / dashboard once they finish. |
Contributor
📝 WalkthroughWalkthroughThe compiler now memoizes CST-to-AST lowering, package aliases, implementation locations, and callee generic facts. It also adds interface-dispatch filtering, scoped subtype assumptions, reusable diagnostic caches, mimalloc allocation, and updated compile-profile documentation. ChangesCompiler performance and analysis
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SourceFile
participant HIR
participant PPIR
participant TIR
participant LSP
SourceFile->>HIR: request file_ast
HIR-->>PPIR: cached AST items
PPIR->>PPIR: generate synthetic items
PPIR-->>HIR: expanded semantic index
TIR->>TIR: read cached aliases and generic facts
SourceFile->>LSP: check_file
LSP->>HIR: read cached file_ast items
HIR-->>LSP: AST items for validation
Possibly related PRs
Suggested reviewers: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_compiler_diagnostics/src/render.rs (1)
237-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
matchexpression.Since
ariadne_cacheis guaranteed to beSomeif and only ifconfig.formatisDiagnosticFormat::Ariadne, matching onconfig.formatinside the closure is redundant. You can simplify the code by matching directly on theariadne_cacheOption.♻️ Proposed refactor
- .map(|d| match (&config.format, &mut ariadne_cache) { - (DiagnosticFormat::Ariadne, Some(cache)) => { - render_ariadne(d, sources, cache, config.color) - } - _ => render_concise(d, sources, file_paths), + .map(|d| match &mut ariadne_cache { + Some(cache) => render_ariadne(d, sources, cache, config.color), + None => render_concise(d, sources, file_paths), })🤖 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_compiler_diagnostics/src/render.rs` around lines 237 - 242, Update the closure’s match in the diagnostic rendering flow to match directly on ariadne_cache rather than pairing it with config.format. Render Ariadne output when the cache is Some, passing the cached value to render_ariadne, and use render_concise for None while preserving the existing arguments and behavior.
🤖 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_compiler_diagnostics/src/render.rs`:
- Around line 237-242: Update the closure’s match in the diagnostic rendering
flow to match directly on ariadne_cache rather than pairing it with
config.format. Render Ariadne output when the cache is Some, passing the cached
value to render_ariadne, and use render_concise for None while preserving the
existing arguments and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3d5aad6-94e8-47ee-b7df-474939c1ca62
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
baml_language/crates/baml_cli/Cargo.tomlbaml_language/crates/baml_cli/src/main.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_hir/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler_diagnostics/src/render.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_type/src/normalize.rsbaml_language/crates/tools_compile_profile/README.md
Binary size checks passed✅ 7 passed
Generated by |
Merging this PR will not alter performance
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
… / callee_generics_for_func as tracked queries Three package-invariant computations were being redone inside hot per-scope/per-call query bodies (re-derived from PR #4016 against current canary, post-#4032): - package_resolved_aliases: the package alias map (a full clone of every alias Ty plus a project walk) was rebuilt inside every infer_scope_types execution (~15.6k executions on the baml_tests corpus), plus per callable_throws / validate_impl_signatures / projection-resolution call. Now a salsa-tracked query keyed by PackageId. It sits in a legitimate cycle (an alias whose RHS contains an associated-type projection resolves through inference, which reads the alias map), handled via cycle_initial seeding an EMPTY alias environment, mirroring infer_scope_types' cycle seed. TypeInferenceBuilder now borrows the cached map instead of owning a per-scope clone. - package_impl_locs: existed as a plain function walking every file in the project per call, called from every impl-resolution loop, coherence checking and type-expr lowering. Now #[salsa::tracked(returns(ref))] per PackageId. - callee_generics_for_func: every call site re-derived the callee's declared generic params (item-tree scan for the enclosing class) and re-lowered its bound type exprs. Now a tracked query per FunctionLoc. Cold compile of crates/baml_tests/baml_src (tools_compile_profile from PR #4038, BAML_NO_BYTECODE_CACHE=1, --repeat 5, median): total 2.347s -> 1.608s; check 1.06s -> 0.74s; emit 1.28s -> 0.86s. Cycle iters unchanged at 13; identical diagnostics (0 errors / 100 warnings). Co-authored-by: Cursor <[email protected]>
… into perf/cold-compile-reland Co-authored-by: Cursor <[email protected]> # Conflicts: # baml_language/crates/baml_lsp2_actions/src/check.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
140-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManual
salsa::Updateimpl is correct; consider a small unit test forcallee_generics_for_func.The
unsafe impl salsa::Update for CalleeGenericssignature (maybe_update(old_pointer: *mut Self, new_value: Self) -> bool) matches Salsa's actualUpdatetrait contract, and comparing viaPartialEqbeforedrop_in_place/writeis a standard, safe early-cutoff pattern givenCalleeGenericsholds only plain owned data (no'dbpointers) — the safety comment is accurate.
callee_generics_for_funcitself (class-param scan + bound lowering) has no dedicated unit test. Given it's now a memoized, reusable query consumed at every call site, a focused test (similar to the existingcompile()-based tests inlower_type_expr.rs) assertingclass_params/user_params/user_boundsfor a method inside a generic class would guard against regressions in this extraction.🤖 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/builder.rs` around lines 140 - 224, Add a focused unit test for callee_generics_for_func using the existing compile()-based test pattern from lower_type_expr.rs. Define a method inside a generic class with a user generic parameter and bound referencing the class parameter, then assert the returned CalleeGenerics class_params, user_params, and user_bounds values. Keep the test scoped to verifying the memoized query’s extraction and bound lowering.Source: Path instructions
🤖 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/builder.rs`:
- Around line 140-224: Add a focused unit test for callee_generics_for_func
using the existing compile()-based test pattern from lower_type_expr.rs. Define
a method inside a generic class with a user generic parameter and bound
referencing the class parameter, then assert the returned CalleeGenerics
class_params, user_params, and user_bounds values. Keep the test scoped to
verifying the memoized query’s extraction and bound lowering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb73f1ae-1604-40af-952e-81b6b976aa8a
📒 Files selected for processing (9)
baml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/builder/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/builder/interface_resolution.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/coherence.rsbaml_language/crates/baml_compiler2_tir/src/interfaces/impl_rules.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_lsp2_actions/src/check.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/crates/baml_lsp2_actions/src/check.rs
This was referenced Jul 23, 2026
Merged
This branch was previously deployed
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
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.
Summary
Combined re-landing of the still-orthogonal optimizations from #4016, re-derived against current canary and measured individually with the profiler from #4038. Folds in three reviewed track branches (their PRs are closed as superseded by this one, branches kept):
perf/reland-file-ast): salsa-trackedfile_ast(db, file)so CST→AST lowering runs once per file instead of once per consumer (~6 sites); PPIRfile_semantic_indexdelegates to HIR's for expansion-free files; PPIRfunction_bodytracked returningArc<FunctionBody>. Individually: −22% cold total, instructions retired −18.5%.perf/reland-mir-dispatch-prefilter): interface-method-name pre-filter indispatch_target_for_concrete(FxHashSet inPackageLoweringData; earlyNonefor plain member accesses);is_subtype_ofco-inductive assumption bookkeeping restricted to the expanding arms (Mu/TypeVar/AssociatedTypeProjectionleft,Muright) viais_subtype_of_inner, with the termination argument documented. Individually: −18% cold total, −28% emit.perf/reland-cli-alloc-diag-render): mimalloc asbaml_cli's global allocator;render_diagnosticsbuilds the ariadneSourceCacheonce per batch instead of per diagnostic. Individually: 3.0× CLI wall time.Plus a one-line docs fix labeling the pre-audit warm-run figures in the profiler README as historical (last open CodeRabbit item from #4038).
Two further tracks from the re-landing plan (TIR package-level queries; lambda double-inference fix) are still in progress and will follow separately.
Performance: canary (0e58a20) vs this branch
Pipeline profiler (#4038's
tools_compile_profile,BAML_NO_BYTECODE_CACHE=1, fresh cache dir, corpusbaml_language/crates/baml_tests/baml_src, 77 files / 25,212 lines). Measured as 7 interleaved baseline/combined pairs on the same machine; the machine carried background load, so the paired per-run ratio is the robust statistic:On the quietest pairs (machine idle-ish), combined total was ~2.2 s vs ~3.6 s baseline. Query executions: 25,972 → 26,054 (+82: the new tracked queries themselves; 37 → 39 unique).
End-to-end CLI (
hyperfine --warmup 1 --runs 7,baml checkover the corpus, disk cache disabled) — this additionally captures the mimalloc win the profiler can't see (it uses mimalloc itself):3.36× ± 0.24 faster (user CPU 3.47×, load-independent).
Safety checks
BAML_CACHE_VERIFY=1with the disk cache enabled, two consecutive runs (second replays cached diagnostics): exit 0, no divergence, rendered output byte-identical modulo the timing suffix.Provenance: #4016 (original audit; see
tools_compile_profile/README.md). Superseded findings not re-landed: recursive-alias hoisting (fixed by #4032), builtin-diagnostics skip and counter-based class type tags (superseded by #3924's caching and content-addressed tags).Made with Cursor
Summary by CodeRabbit