perf(build): move baml_std rustgen out of build.rs to kill the double-compile - #3615
Closed
hellovai wants to merge 10 commits into
Closed
perf(build): move baml_std rustgen out of build.rs to kill the double-compile#3615hellovai wants to merge 10 commits into
hellovai wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a deterministic staleness-check crate and a centralized codegen tool, replaces per-crate code generation with verification-only build scripts that assert checked-in generated sources match the baml stdlib, and updates workspace includes, manifests, and dev tooling configuration. ChangesBuild-time Staleness Check and Centralized Codegen Refactor
Sequence DiagramsequenceDiagram
participant Developer
participant ToolsRustgen
participant BamlRustgenCheck
participant Filesystem
participant BuildScript
Developer->>ToolsRustgen: run `cargo run -p tools_rustgen`
ToolsRustgen->>BamlRustgenCheck: header() wrap generated outputs
ToolsRustgen->>Filesystem: write checked-in generated files
BuildScript->>BamlRustgenCheck: assert_generated_matches_baml_std(rel_path)
BamlRustgenCheck->>Filesystem: read generated file header
BamlRustgenCheck->>BamlRustgenCheck: recompute expected hash from baml_std
alt match
BamlRustgenCheck->>BuildScript: no-op (build continues)
else mismatch
BamlRustgenCheck->>BuildScript: panic with mismatch details
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Merging this PR will improve performance by 10.75%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
hellovai
marked this pull request as ready for review
May 30, 2026 22:57
Contributor
Author
|
@codspeedbot explain why this is faster |
⚡ Performance Analysis:
|
| Function | BASE (canary) | HEAD (this PR) | Delta |
|---|---|---|---|
BexVm::exec (self time) |
111.9 ms (94.6%) | 100.6 ms (94.2%) | −11.3 ms (−10.1%) |
BexVm::store_local_value (self time) |
5.8 ms (4.9%) | 5.6 ms (5.2%) | −0.2 ms (−3.4%) |
| Total | 118.2 ms | 106.8 ms | −11.4 ms (−9.6%) |
Why is it faster?
The improvement is almost entirely concentrated in BexVm::exec — the core opcode dispatch loop of the VM. This PR doesn't directly modify any runtime code, so the speedup is a code layout / instruction alignment effect caused by the build-system changes.
Here's what's happening:
-
Changed dependency graph → different link order. By removing
baml_compiler2_astfrom the build-dependency graph of four crates, the overall compilation and linking order changed. This shifts where functions land in the final binary. -
vm_loop_500kis extremely alignment-sensitive. This benchmark is a tight integer loop that spends 94%+ of its time in a single function (BexVm::exec). Opcode dispatch loops like this are dominated by instruction-cache behavior and branch-predictor alignment. Even small shifts in where hot basic blocks land relative to cache-line and fetch-block boundaries can produce 5–15% swings. -
The binary was recompiled differently. The monomorphization hash changed (
h043a1c…→hbe9b24…), confirming LLVM made different code generation decisions for this function in the new build graph — likely better alignment of the hot dispatch paths.
Bottom line
This is a favorable but incidental side effect of the build restructuring — the kind of performance delta that's real and reproducible within the same binary, but tied to code layout rather than algorithmic improvement. The primary win of this PR remains the ~30% reduction in rebuild CPU time after AST edits.
Summary
- Warming up the performance wizard...
- Analyzing performance improvement — Flamegraph analysis complete for both HEAD and BASE runs
- Handling comment — Comment handling finished successfully
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@baml_language/crates/baml_rustgen_check/src/lib.rs`:
- Around line 67-82: The walk() function currently swallows std::fs::read_dir
and std::fs::read errors causing partial input collection; change walk to return
a Result<(), std::io::Error> (or appropriate error type) and propagate errors
with ? for std::fs::read_dir(dir) and std::fs::read(&path) instead of skipping,
and update collect_inputs() to propagate its Result as well so any I/O failure
bubbles up to the caller; reference the walk(root: &Path, dir: &Path, out: &mut
Vec<(String, Vec<u8>)>) and collect_inputs() signatures and ensure callers
handle or return the propagated Result.
In `@baml_language/crates/sys_types/build.rs`:
- Around line 6-9: The build script is using incorrect package-relative paths
(“sys_types/src/...”) in calls to baml_rustgen_check; update the assertions to
use crate-local paths ("src/io_generated.rs" and "src/runtime_io.rs") and any
other occurrences, i.e., change the two assert_generated_matches_baml_std(...)
calls in main() (and keep baml_rustgen_check::rerun_if_baml_std_changed() as-is)
so they reference "src/io_generated.rs" and "src/runtime_io.rs" instead of the
nested "sys_types/..." paths.
In `@baml_language/crates/tools_rustgen/src/main.rs`:
- Around line 7-11: The status message is misleading because
tools_rustgen::write_all() always rewrites files; update the final eprintln so
it reflects that files were written rather than "up to date" — e.g., change the
message that currently uses written.len() to "wrote N file(s)" or "generated N
file(s)"; locate the write_all() call and the written variable in main.rs (the
println! loop and the final eprintln!) and replace the misleading text
accordingly (or alternatively modify write_all() to return created/unchanged
sets and use those to report accurate counts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 071f1f3f-b949-4a5e-9ea4-83ed133eedd0
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.envrcbaml_language/Cargo.tomlbaml_language/crates/baml_rustgen_check/Cargo.tomlbaml_language/crates/baml_rustgen_check/src/lib.rsbaml_language/crates/bex_vm/Cargo.tomlbaml_language/crates/bex_vm/build.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/nativefunctions_generated.rsbaml_language/crates/bex_vm_types/Cargo.tomlbaml_language/crates/bex_vm_types/build.rsbaml_language/crates/bex_vm_types/src/errors_generated.rsbaml_language/crates/bex_vm_types/src/panics_generated.rsbaml_language/crates/bex_vm_types/src/sys_op_generated.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/sys_ops/Cargo.tomlbaml_language/crates/sys_ops/build.rsbaml_language/crates/sys_ops/src/io_adapter.rsbaml_language/crates/sys_ops/src/io_generated.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/crates/sys_types/Cargo.tomlbaml_language/crates/sys_types/build.rsbaml_language/crates/sys_types/src/io_generated.rsbaml_language/crates/sys_types/src/lib.rsbaml_language/crates/sys_types/src/runtime_io.rsbaml_language/crates/tools_rustgen/Cargo.tomlbaml_language/crates/tools_rustgen/src/lib.rsbaml_language/crates/tools_rustgen/src/main.rsbaml_language/crates/tools_rustgen/tests/up_to_date.rsbaml_language/mise.tomlbaml_language/stow.toml
hellovai
force-pushed
the
hellovai/compile-times
branch
from
May 30, 2026 23:16
08429f7 to
0e1fb95
Compare
…-compile
The build.rs of sys_types, sys_ops, bex_vm, and bex_vm_types each ran
baml_builtins2_codegen at build time, which pulled baml_compiler2_ast into
their *build*-dependency graph. Cargo then compiled the ast subtree twice
(once for the host build scripts, once for the target) and re-ran those build
scripts on every ast edit — slowing both clean builds and the edit→rebuild loop.
Instead, generate the builtin/IO source files ahead of time and check them in:
- `crates/tools_rustgen` (binary): runs the same `baml_builtins2_codegen`
calls and writes the 8 generated files into the crates' `src/` dirs.
Invoke with `cargo run -p tools_rustgen` (or `mise run codegen`).
- `include!(concat!(env!("OUT_DIR"), ...))` → `include!("<local>.rs")`.
- The 4 build.rs no longer run codegen.
Freshness is guarded at two layers so nothing silently goes out of sync:
- `crates/baml_rustgen_check` (zero-dependency): hashes the codegen-relevant
surface of baml_std — builtin signatures, `//baml:` directives, and class
fields — and embeds a per-file hash in each generated file's header. Each
crate's build.rs calls `assert_generated_matches_baml_std(...)` and fails the
build if the checked-in file drifts. It depends only on baml_rustgen_check
(no ast), so the double-compile stays gone. 23 unit tests pin the lexical
extraction (directives, class fields, multi-line sigs, pure-BAML functions
dropped, comments/docs ignored).
- `tools_rustgen`'s `up_to_date` test regenerates for real and diffs — the
complete backstop (also covers codegen-logic changes) — and runs in CI's
workspace nextest on all three OSes.
Measured (cargo build --bin baml-cli after an ast edit): ast compiles 2x → 1x;
cascade 29 → 16 crates; ~30% less CPU.
Also includes small, on-theme compile-time tweaks:
- `[profile.dev]` debug = "line-tables-only" + split-debuginfo = "unpacked".
- sccache wired via mise.toml [tools] + .envrc RUSTC_WRAPPER (scaffolding
existed but RUSTC_WRAPPER was never set, so caching was inert).
- stow.toml: add "rustgen" to the baml namespace approved_prefixes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hellovai
force-pushed
the
hellovai/compile-times
branch
from
May 30, 2026 23:21
0e1fb95 to
c618a3e
Compare
The `benchmarks-build` job runs on a Blacksmith self-hosted ARM64 runner. Its
`Swatinem/rust-cache` save uploads the ~4GB cache to 100%, but the GitHub cache
backend's *finalize* step races the runner's immediate post-job shutdown
("Proceeding with shutdown immediately"), so the entry is never committed —
every run is then a cold "No cache found". Confirmed via the cache API: no
`v0-rust-linux-arm-bench` entry exists in the store despite a successful canary
save log (and in fact no `v0-rust-linux-*` Blacksmith cache persists at all,
while the GitHub-hosted `macos-cargo` cache does).
Swap to `useblacksmith/rust-cache@v3` — Blacksmith's drop-in for
Swatinem/rust-cache — which uses Blacksmith's own cache backend and has no
teardown/finalize race. If this confirms the fix on canary, the same one-line
swap should be applied to the other `blacksmith-*` jobs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Temporarily set the benchmarks-build rust-cache `save-if: true` (normally
canary-only) so this PR's own bench-build run saves the cache via
useblacksmith/rust-cache. Lets us confirm the cache now persists (the
finalize/teardown race is gone) without merging to canary first.
REVERT this commit before merge — production behavior must remain
`save-if: ${{ github.ref == 'refs/heads/canary' }}`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-cache@v3 ignores it) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Confirmed on PR run 26698883614: useblacksmith/rust-cache@v3 restored the
bench cache ('Cache restored successfully') that the prior run saved to
Blacksmith's backend — the GitHub-cache finalize/teardown race is gone. Drop
the temporary save-if:true used for the test; production stays canary-only.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validated on the bench job (prior run saved, next run "Cache restored successfully"): the GitHub cache backend's finalize races the Blacksmith runner's immediate post-job teardown, so NONE of the v0-rust-linux-* caches ever persisted — every Blacksmith Rust job ran cold on every PR. Swap Swatinem/rust-cache@v2 -> useblacksmith/rust-cache@v3 (Blacksmith's drop-in, no teardown race) on every Blacksmith-runner job: prek, proto-sync, the cargo-tests linux/windows legs (+ the non-macOS matrix legs), size-gate linux/windows, and wasm-pack. Dropped the unsupported `cache-workspace-crates` input on the swapped steps. The two GitHub-hosted macOS jobs (cargo-test-macos, size-gate-macos) stay on Swatinem/rust-cache — useblacksmith only applies to Blacksmith runners. Since `uses:` can't be an expression, the mixed-runner cargo-tests matrix job uses two `if: matrix.os-label`-gated cache steps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/cargo-tests.reusable.yaml (1)
65-74: ⚡ Quick winUpdate the cache-strategy doc —
cache-workspace-crates: trueis no longer universal.This block states every rust-cache invocation passes both
cache-all-crates: trueandcache-workspace-crates: true, and explains that the latter keeps the compiled workspacetarget/so PRs reading canary's cache "restart with the workspace crates already built." After this PR, the migrateduseblacksmith/rust-cache@v3steps (linux/windows/wasm/msrv/cargo-doc/snapshot) no longer passcache-workspace-crates; only the macOSSwatinemstep still does.Since the Swatinem default for that input is
false, dropping it means workspace-crate artifacts are no longer cached on those legs — a real behavior change, not just a doc nit (PRs lose the warm workspacetarget/on the affected platforms). Please reconcile the comment with the new reality and confirm whether the lost workspace-crate caching is intended (the PR notesv3doesn't support this input).🤖 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 @.github/workflows/cargo-tests.reusable.yaml around lines 65 - 74, The comment claiming "Every rust-cache invocation also passes `cache-all-crates: true` and `cache-workspace-crates: true`" is now inaccurate after migrating to useblacksmith/rust-cache@v3; update the doc block to state that `cache-workspace-crates` is not passed on all legs (only the macOS "Swatinem" step still sets it) and explicitly note the behavioral impact (workspace `target/` artifacts are not cached on other platforms), and either confirm in the comment that this loss of workspace-crate caching is intentional or revert/add the input where needed; reference the strings `cache-workspace-crates`, `cache-all-crates`, and `useblacksmith/rust-cache@v3` when making the change.
🤖 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.
Inline comments:
In @.github/workflows/ci.yaml:
- Line 257: Replace the archived action reference "uses:
useblacksmith/rust-cache@v3" with the upstream maintained Rust cache action
(Swatinem/rust-cache) wherever it appears in workflow YAMLs; update the action
entry to use the Swatinem/rust-cache release (e.g.,
Swatinem/rust-cache@<stable-tag>) and make the same replacement for the
identical occurrences in the other workflow files so CI uses the supported
upstream cache implementation.
---
Nitpick comments:
In @.github/workflows/cargo-tests.reusable.yaml:
- Around line 65-74: The comment claiming "Every rust-cache invocation also
passes `cache-all-crates: true` and `cache-workspace-crates: true`" is now
inaccurate after migrating to useblacksmith/rust-cache@v3; update the doc block
to state that `cache-workspace-crates` is not passed on all legs (only the macOS
"Swatinem" step still sets it) and explicitly note the behavioral impact
(workspace `target/` artifacts are not cached on other platforms), and either
confirm in the comment that this loss of workspace-crate caching is intentional
or revert/add the input where needed; reference the strings
`cache-workspace-crates`, `cache-all-crates`, and `useblacksmith/rust-cache@v3`
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f59a26fd-df9e-4022-b109-18bceb7be722
📒 Files selected for processing (4)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yaml.github/workflows/size-gate.reusable.yaml.github/workflows/wasm-pack-tests.reusable.yaml
baml-cli had grown flush against its macOS (15_072_808) and Windows (16_273_800) max_file_bytes ceilings; build/LTO jitter tipped it +0.1% over, failing size-gate. The PR's codegen refactor is size-neutral (checked-in code == build.rs output; no new deps in baml-cli). Add 100 KiB headroom to each ceiling. The 3% max_delta_pct still guards genuine regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hived) Per Blacksmith's docs, `useblacksmith/rust-cache` (and the other useblacksmith/* cache forks) are archived/unmaintained: Blacksmith now TRANSPARENTLY redirects the upstream GitHub/third-party cache actions (Swatinem/rust-cache, actions/cache) to their colocated cache with zero code changes. The repo's cache design already targets GitHub-cache semantics (canary-only savers + the daily canary-cache-refresh cycle), which Blacksmith accelerates transparently. So revert all the useblacksmith@v3 swaps + the matrix conditional split + the temporary save-if back to canary's upstream Swatinem@v2 setup (the 4 workflow files now match canary exactly). The bench-cache non-persistence we diagnosed is an infra concern — confirm Blacksmith's transparent cache is enabled for the repo/ARM runners — not a code change. Kept: the +100 KiB baml-cli size-gate ceiling bump (validated green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address CodeRabbit review on PR #3615: - baml_rustgen_check::walk now panics on read_dir/read errors instead of silently skipping them — a partial/unreadable source-of-truth must abort the build/regen, never hash a partial tree that could match stale output. (#1) - tools_rustgen status message: "wrote N generated file(s)" instead of the misleading "up to date" (write_all always rewrites). (#4) - Document normalize()'s known whitespace-in-string-literal limitation; the up_to_date regen test is the backstop, so a quote-aware tokenizer isn't worth the complexity. (#2) Not changed: the build.rs "sys_types/src/..." paths flagged critical are correct — assert_generated_matches_baml_std resolves via crates_dir() (CARGO_MANIFEST_DIR/..), not the build script cwd, and CI is green. (#3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
the 3second compile time is not worht the workflow change. |
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.
Moves the BAML stdlib rust codegen out of
build.rsand commits the output.sys_types,sys_ops,bex_vm, andbex_vm_typesrunbaml_builtins2_codegenfrombuild.rs, which putsbaml_compiler2_astin their build-dependency graph. Cargo compiles the ast subtree twice (host + target) and re-runs the build scripts on every ast edit — on top of an already-expensive rebuild, since ast is at the bottom of the IR spine.This generates those files ahead of time, commits them, and replaces the four build scripts with a hash check that doesn't run codegen.
Impact
cargo build --bin baml-cliafter touching ast:astrustc invocationsWall-clock barely moves on a many-core machine (the serial spine still gates it); the win shows up on
--workspace, the test build, and CI.Working with the generated code
Now committed:
sys_types/src/{io_generated,runtime_io}.rssys_ops/src/{io_generated,io_adapter}.rsbex_vm/src/package_baml/nativefunctions_generated.rsbex_vm_types/src/{sys_op,errors,panics}_generated.rsAfter changing
baml_builtins2/baml_std/**or the codegen logic, regenerate and commit:Two guards keep it honest:
build.rshashes the relevantbaml_stdsurface and fails the build (pointing at the command) when its file is stale. Depends only on the zero-depbaml_rustgen_check, so the compiler stays out of the build graph.tools_rustgen'sup_to_datetest regenerates and diffs; the catch-all.The hash only covers what drives codegen (
$rust_function/$rust_io_function/$compiler_intrinsicsignatures, their//baml:directives, class fields), so docs / formatting / pure-BAML edits don't trigger a regen.Tradeoffs
Also in here
tools_rustgen(generator) andbaml_rustgen_check(zero-dep guard, 23 tests)..gitattributespins the generated files to LF so the byte-exact check passes on Windows.baml-climacOS/Windows ceilings — the binary was flush against them and LTO jitter (+0.1%) was tripping the gate; the 3% delta guard is unchanged.Not in this PR: the Blacksmith CI cache (bench cache never persists; the GitHub-cache finalize races the runner teardown) is an infra setting — enable transparent cache — tracked separately.