ci: split benchmark build (Blacksmith ARM64) from CodSpeed walltime run - #3601
Merged
Conversation
The single `benchmarks-instrumented` job built and ran on one small runner, and the `cargo codspeed build` step was timing out. Split it in two: - `benchmarks-build` (blacksmith-8vcpu-ubuntu-2204-arm): `cargo codspeed build -p baml_tests -m walltime`, uploads the prebuilt binaries. - `benchmarks-run` (codspeed-macro): downloads them and `cargo codspeed run`. `cargo codspeed run` only executes the prebuilt binaries (it never invokes the compiler), so the run job can't accidentally rebuild with default flags. Build is on ARM64 to match the codspeed-macro arch, on Ubuntu 22.04 (older glibc) for forward-compatibility, and restores the executable bit that upload-artifact strips. Caching uses a dedicated, bench-only `linux-arm-bench` lane (canary-only saver) — it can't reuse the x86 `linux-cargo` lane (rustc host triple is in the key) and is too thin a seed to share with other ARM jobs. Runs automatically on any change under baml_language/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds a two-phase CodSpeed bench pipeline (ARM64 prebuild + run), documents a ChangesCodSpeed benchmarking with ARM64 build artifacts
Sequence Diagram(s)sequenceDiagram
participant WorkflowTrigger as Workflow Trigger
participant BuildJob as benchmarks-build (ARM64 runner)
participant ArtifactStorage as Artifact Storage
participant RunJob as benchmarks-run
participant CodSpeedAction as CodSpeedHQ/action
WorkflowTrigger->>BuildJob: trigger (baml_language/** changes or RUN_CODSPEED=1)
BuildJob->>BuildJob: install Rust + cargo-codspeed@4.7.0
BuildJob->>BuildJob: cargo codspeed build -p baml_tests --bench runtime_benchmark -m walltime
BuildJob->>ArtifactStorage: upload walltime binaries
ArtifactStorage->>RunJob: download prebuilt walltime binaries
RunJob->>RunJob: restore executable permissions
RunJob->>CodSpeedAction: cargo codspeed run -m walltime 'vm_|engine_init' (continue-on-error)
CodSpeedAction-->>RunJob: benchmark results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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:
- Around line 495-615: The CI failure-alert job is not depending on the
benchmarks-build job, so a failing benchmarks-build is invisible to the `CI-v2
Failure Alert`; update the workflow so the failure-alert job's needs/fan-in
includes `benchmarks-build` (add the `benchmarks-build` job name to the `needs:`
array that currently feeds `ci-failure-alert`), leaving `benchmarks-run` as
advisory/continue-on-error if you want measurement blips non-blocking; this
ensures `ci-failure-alert` sees benchmark build failures.
- Around line 514-557: Pin the floating action refs to immutable commit SHAs by
replacing uses: entries for actions/checkout@v6, Swatinem/rust-cache@v2,
taiki-e/install-action@v2, actions/upload-artifact@v7,
actions/download-artifact@v7 and CodSpeedHQ/action@v4 with their corresponding
full commit SHAs (locate the uses: lines in the new benchmark job steps such as
the "Checkout Branch", rust-cache, "Install cargo-codspeed", "Upload prebuilt
benchmark binaries" and download/upload steps) and update the workflow so the
job dependency graph includes the benchmark jobs (add the benchmark job names,
e.g. benchmarks-build and benchmarks-run, to the ci-failure-alert.needs array)
so ci-failure-alert will run and reflect benchmark failures.
🪄 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: 03af2929-6a76-4230-b374-b818248d5866
📒 Files selected for processing (2)
.github/workflows/cargo-tests.reusable.yaml.github/workflows/ci.yaml
Binary size checks passed✅ 7 passed
Generated by |
The vm_* benches recompiled the BAML project on every divan sample (via with_inputs) and built a fresh tokio runtime inside the measured region on every call_main(). The recompile burned ~208s of wall-clock per pass (untimed but real), and the per-call runtime creation polluted the measurement: ~8ms of each "VM" number was just tokio::Runtime::new(), not VM execution. BexEngine::call_function takes &Arc<Self> and builds a fresh call context per call, so one compiled engine safely serves every sample. Factor the vm_* benches through a bench_vm_main() helper that compiles + builds the runtime once and measures only the call. Effect (100 samples, local): runtime-only suite 255s -> 36s; e.g. vm_fib_20 9.7ms -> 1.8ms, vm_string_concat_5k 9.5ms -> 0.81ms. CI (benchmarks-run): - Filter to `vm_|engine_init` so we measure only runtime, not the compile-bound e2e_/compile_to_engine/startup benches. - Drop the DIVAN_SAMPLE_COUNT band-aid: at ~2 min/pass on the macro runner the full 100-sample suite fits the timeout comfortably. - Build only runtime_benchmark (cache_profile is a macOS/kperf no-op on Linux). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/benches/runtime_benchmark.rs (1)
51-60: 💤 Low valueOptional:
call_mainstill spins up a fresh Tokio runtime on every invocation.For the
bench-based callers (startup_empty_expression,e2e_*) this happens inside the measured closure on every sample, so runtime-construction cost is folded into those measurements. If that overhead is intentional for the e2e/startup numbers, ignore this; otherwise consider the same hoisting pattern asbench_vm_main.🤖 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_tests/benches/runtime_benchmark.rs` around lines 51 - 60, call_main currently builds a new Tokio runtime for every call (fn call_main), which makes per-sample benchmarks include runtime construction cost; instead hoist runtime creation into the benchmark setup and make call_main accept and use the existing runtime/handle (e.g., pass &tokio::runtime::Runtime or &tokio::runtime::Handle or an Arc<Runtime>) or use a once-created static runtime used by bench_vm_main and the e2e/startup benchmarks; update call_main signature and all callers (including bench_vm_main and e2e_* benches) to reuse that runtime so samples only measure engine.call_function execution.
🤖 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_tests/benches/runtime_benchmark.rs`:
- Around line 51-60: call_main currently builds a new Tokio runtime for every
call (fn call_main), which makes per-sample benchmarks include runtime
construction cost; instead hoist runtime creation into the benchmark setup and
make call_main accept and use the existing runtime/handle (e.g., pass
&tokio::runtime::Runtime or &tokio::runtime::Handle or an Arc<Runtime>) or use a
once-created static runtime used by bench_vm_main and the e2e/startup
benchmarks; update call_main signature and all callers (including bench_vm_main
and e2e_* benches) to reuse that runtime so samples only measure
engine.call_function execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fbad646-1b69-41e9-91bb-59e2730b4406
📒 Files selected for processing (2)
.github/workflows/ci.yamlbaml_language/crates/baml_tests/benches/runtime_benchmark.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yaml
Merging this PR will degrade performance by 52.73%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| 👁 | WallTime | vm_array_push_50k |
8.9 ms | 23.5 ms | -61.88% |
| ⚡ | WallTime | vm_string_concat_5k |
27.1 ms | 4.1 ms | ×6.7 |
| 👁 | WallTime | vm_closure_call_50k |
8.4 ms | 22.7 ms | -63.14% |
| 👁 | WallTime | vm_class_create_50k |
12.5 ms | 38.9 ms | -67.92% |
| 👁 | WallTime | vm_fib_20 |
5.7 ms | 8.2 ms | -30.65% |
| 🆕 | WallTime | vm_wide_nested_class_create_50k |
N/A | 300.9 ms | N/A |
| 👁 | WallTime | vm_array_iter_10k |
5.2 ms | 7.8 ms | -33.49% |
| 👁 | WallTime | vm_mixed_ops |
5.9 ms | 10.7 ms | -45.23% |
| 👁 | WallTime | vm_nested_loop |
4.9 ms | 9.9 ms | -50.11% |
| 👁 | WallTime | vm_field_access_50k |
6.7 ms | 17.1 ms | -60.69% |
| 👁 | WallTime | vm_call_chain_100_x_5k |
35.1 ms | 142.9 ms | -75.47% |
| 👁 | WallTime | vm_loop_500k |
21.1 ms | 117.7 ms | -82.08% |
| 👁 | WallTime | engine_init_cost |
3.3 ms | 17.3 ms | -81.03% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing hellovai/fix-benchmarks (16475d4) with canary (d393c6f)2
Footnotes
-
7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
canary(c3b383c) during the generation of this report, so d393c6f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
The run job's 30-min near-timeout was NOT a rebuild (cargo codspeed run only executes prebuilt binaries in 4.7.0). It was `cargo codspeed run`'s implicit `cargo metadata` re-cloning every git dependency (minijinja, the aws-sdk-rust fork, …) on the bare codspeed-macro runner, which has no cargo cache — ~24 min of silence before the first bench. Evidence: the build job spent 13.5 min in `cargo metadata` before its first `Compiling` line, and a cold-CARGO_HOME `cargo metadata` locally sits on "Updating git repository" for many minutes. Add a read-only Swatinem/rust-cache restore (shared-key linux-arm-bench, same aarch64 triple as benchmarks-build) so the run job reuses the cargo home the build job saves on canary. benchmarks-build stays the sole (canary) saver. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Temporary: flip benchmarks-build save-if to true so the cargo-home cache is seeded during this PR run and benchmarks-run can restore it in the same workflow (proving the cargo metadata fix before canary seeds the lane). Revert save-if to canary-only before merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Swatinem-based restore missed: rust-cache folds a per-runner environment
hash into its key, and `rustup toolchain list` errors on codspeed-macro, so the
build (7aa5a20b) and run (63ff4892) keys never matched — the run job restored
nothing and stalled ~24 min in cold `cargo metadata` again.
Switch to a plain actions/cache keyed on hashFiles(Cargo.lock), byte-identical
on both runners: benchmarks-build saves ~/.cargo/{registry,git}, benchmarks-run
restores it, and CARGO_NET_OFFLINE=true makes `cargo metadata` use only the
local cache (no index update, no git fetch). Revert the temporary Swatinem
save-if hack back to canary-only.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ers)
actions/cache addresses entries by (key, version); the version depends on the
runner's compression tooling, so the cargo-home cache saved on the blacksmith
build runner is invisible to the codspeed-macro run job even with a byte-
identical key (verified: same key 4f01266e…, "Cache not found"). The offline
guard then correctly failed in 2s instead of stalling 24 min — proving the
diagnosis but not yet the fix.
Switch to the mechanism we already use for the binary: tar ~/.cargo/{registry,
git} into an artifact in benchmarks-build, download + untar it in benchmarks-run.
Artifacts are content-addressed by name and cross runners cleanly. With
CARGO_NET_OFFLINE=true the run job's cargo metadata then resolves entirely from
the local cargo home — no cold git re-clone.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hellovai
enabled auto-merge
May 30, 2026 06:18
The run job only does offline cargo metadata (needs the registry index + git checkouts), so drop registry/src — the extracted dep sources that dominate the 3.4GB artifact — to cut the run job's download/untar setup time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
Splits the benchmark CI into two jobs so the slow compile and the measurement happen on the right machines:
benchmarks-buildblacksmith-8vcpu-ubuntu-2204-armcargo codspeed build -p baml_tests -m walltime→ uploads prebuilt binariesbenchmarks-runcodspeed-macrocargo codspeed run -m walltimeviaCodSpeedHQ/action@v4Why
The previous single job built and ran on one small runner, and
cargo codspeed buildwas timing out.Key correctness points
cargo codspeed runonly executes the prebuilt binaries (verified against the cargo-codspeed source) — it never invokes the compiler. If the artifact is missing it errors loudly instead of measuring the wrong binary.codspeed-macrois ARM64, so we build on ARM64 too, on Ubuntu 22.04 (older glibc) so binaries stay forward-compatible with whatever glibc the macro runner ships.chmod +xafter download —upload-artifactstrips the executable bit.cargo-codspeed@4.7.0pinned to matchcodspeed-divan-compat.continue-on-erroron the run + kept out of theci-failure-alertgate, so a measurement blip never blocks a merge.Caching
Dedicated bench-only
linux-arm-benchlane (canary-only saver, PRs read-only). It can't reuse the x86linux-cargolane (rustc host triple is baked into the key) and is too thin a seed (onlybaml_testsbenches) to share with other ARM jobs. Documented in the saver/reader map.Triggers
Runs automatically on any change under
baml_language/(determine_changes.code), plus push to main/canary andRUN_CODSPEED=1opt-in for unrelated PRs.codspeed-macrois enabled for the org (separate CodSpeed feature).blacksmith-8vcpu-ubuntu-2204-armis provisioned — only4vcpu-armis attested in the repo today; drop to4vcpuif 8 isn't available.🤖 Generated with Claude Code
Summary by CodeRabbit