perf(install): overlap cold-install lockfile write with the link tail - #961
Conversation
On a fresh-resolve cold install the lockfile serialize + pnpm-parity reformat + atomic write ran as a serial blocking span before filter_graph and the link phase. On a large tree that costs 11-55 ms (measured: 11.0 ms +/- 0.56 at 1.5k packages, 55.8 ms +/- 1.73 at 4.2k) with the linker idle. Move that work onto a spawn_blocking task that runs concurrently with filter_graph, the progress reconcile, and run_link_phase, joining before run_finalize_phase re-reads the graph. The task operates on a clone of the prepared graph taken after the write-prep mutations and before filter_graph mutates the original, so it serializes the exact same state the inline write did. A full graph clone measures 0.6-3 ms (12-18x less than the write it hides), so the net win is the recovered write time minus one clone: ~10 ms at 1.5k packages, ~51 ms at 4.2k. On by default; AUBE_DISABLE_LOCKFILE_WRITE_OVERLAP reverts to the inline serial write (byte-identical output, same error point, no graph clone), matching aube's existing AUBE_DISABLE_* overlap-opt convention. The rare catch-up integrity-rewrite joins the in-flight write first so the two never race the same atomic-write rename and the on-disk result is the rewrite, preserving the old serial ordering. The join surfaces a write error (including a task panic) rather than dropping it. Adds a byte-identical cold-install test (install.bats) asserting the overlapped and serial paths produce identical lockfile bytes, and a pnpm_lock_write criterion bench measuring write_full vs graph_clone.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds an optional overlapped lockfile write path during install, with an environment-variable killswitch and joins before integrity rewrite and finalize. It also adds a pnpm lockfile write benchmark and a regression test comparing overlap enabled and disabled outputs. ChangesInstall lockfile write overlap
Pnpm lockfile write benchmark
Sequence Diagram(s)sequenceDiagram
participant InstallMod
participant lockfile_write_overlap
participant spawn_blocking
participant write_one
participant run_finalize_phase
InstallMod->>lockfile_write_overlap: overlap_enabled()
alt overlap enabled
InstallMod->>lockfile_write_overlap: spawn(LockfileWriteInputs)
lockfile_write_overlap->>spawn_blocking: run captured write_one
spawn_blocking->>write_one: write lockfile
else overlap disabled
InstallMod->>lockfile_write_overlap: write_one(...)
end
InstallMod->>lockfile_write_overlap: join(handle) before integrity rewrite
InstallMod->>lockfile_write_overlap: join(handle) before run_finalize_phase
InstallMod->>run_finalize_phase: finalize install
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryMoves the cold-install lockfile serialize+write off the critical path onto a
Confidence Score: 5/5Safe to merge; the race between the overlapped write and the catch-up integrity rewrite is correctly serialized, and write errors are surfaced before finalize on all non-error install paths. The overlap logic is contained in a dedicated module, the two join points cover the critical ordering constraints, and the killswitch + byte-identity bats test together give high confidence the optimization changes only timing, not output. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "refactor(install): inline single-use wri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aube/src/commands/install/mod.rs (1)
2049-2058: 🩺 Stability & Availability | 🔵 TrivialLog ordering is non-deterministic under concurrency; silent write errors remain a risk
The search confirmed no strict ordering assertions for "Wrote" logs in
test/resolve.batsor other.batsfiles, implying the concurrent emission of debug logs during the link phase does not break existing tests.However, the behavioral difference regarding error handling persists. If an error occurs between the
spawn_blockingcall and the subsequentjoin(e.g., duringfetch_packages_with_rootorrun_link_phase), thelockfile_write_handleis dropped without joining. This results in the blocking task's error being silently swallowed, whereas the inline path would surface it. This breaks strict error-ordering parity.To ensure deterministic error reporting:
- Explicitly join the
handleand propagate its error on all early-return paths before the join point.- Alternatively, wrap the blocking task to propagate failures via a shared channel or
Resultfield if the task must continue independently.Without this fix, the specific error logged during a failure may be indeterminate.
🤖 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 `@crates/aube/src/commands/install/mod.rs` around lines 2049 - 2058, The lockfile write path in install/mod.rs drops the in-flight `lockfile_write_handle` on early returns, so errors from the `spawn_blocking` write can be lost. Update the `lockfile_write_handle`/`lockfile_write_overlap::join` flow so every exit path after spawning the write explicitly joins the handle and propagates any failure before returning, not only the happy path. Use the existing `lockfile_write_handle` variable and the join helper in the same install/link phase logic to keep error reporting consistent with the inline write 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 `@crates/aube/src/commands/install/mod.rs`:
- Around line 2049-2058: The lockfile write path in install/mod.rs drops the
in-flight `lockfile_write_handle` on early returns, so errors from the
`spawn_blocking` write can be lost. Update the
`lockfile_write_handle`/`lockfile_write_overlap::join` flow so every exit path
after spawning the write explicitly joins the handle and propagates any failure
before returning, not only the happy path. Use the existing
`lockfile_write_handle` variable and the join helper in the same install/link
phase logic to keep error reporting consistent with the inline write behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bc088ed-1415-4cf4-82e8-9afab365bd65
📒 Files selected for processing (6)
crates/aube-lockfile/Cargo.tomlcrates/aube-lockfile/benches/pnpm_lock_write.rscrates/aube-lockfile/src/pnpm/mod.rscrates/aube/src/commands/install/lockfile_write_overlap.rscrates/aube/src/commands/install/mod.rstest/install.bats
…or conversion Inline the single-use `write_inline` pass-through into its sole caller by making `write_one` `pub(super)` and calling it directly from the killswitch-disabled inline path in mod.rs. Drop the `Err::<(), _>(join_err).into_diagnostic().unwrap_err()` dance in `join()` for `Result::<(), _>::Err(join_err).into_diagnostic().wrap_err(..)`, which removes the `unwrap_err()` while keeping the required `into_diagnostic` bridge (`JoinError` is a plain `std::error::Error`, not a miette `Diagnostic`). Behavior is unchanged: the killswitch path stays byte-identical to the overlap path (install.bats byte-identity test green), and the join-error `Report` chain and panic-context message are preserved.
2 participants
Measured
Overlapping the cold-install lockfile write with the link tail recovers the full serialize+write span minus a graph clone. Measured with a criterion bench (
pnpm_lock_write) on realpnpm-lock.yamlfiles, release profile, on macOS/arm64:write_fullis the serialize + pnpm-parity reformat + atomic fs write — exactly the work that moves off the critical path.graph_cloneis the offsetting cost the spawned task pays. The write is ~12-18x the clone, so the net is the recovered write time minus one clone, well outside noise.Mechanism
On the fresh-resolve cold install the lockfile write ran as a serial blocking span before
filter_graph+run_link_phase, with the linker idle. This moves it onto atokio::task::spawn_blockingtask that runs concurrently withfilter_graph, the progress reconcile, and the link phase, joining beforerun_finalize_phasere-reads the graph.The task operates on a clone of the prepared graph, taken after the write-prep mutations (
refresh_lockfile_pin,stamp_pnpm_config_checksums,prepare_resolved_graph_for_lockfile_write) and beforefilter_graphmutates the original in place — so it serializes the exact same state the inline write did.Killswitch
On by default.
AUBE_DISABLE_LOCKFILE_WRITE_OVERLAPreverts to the inline serial write — byte-identical output, same error point, and no graph clone (exactly the pre-overlap cost). Matches the existingAUBE_DISABLE_*overlap-opt convention (DISABLE_CRITICAL_PATH,DISABLE_TARBALL_STREAM, …) and reads throughembedder_env, so a host with noenv_prefixexposes no branded toggle.Error ordering
The rare catch-up integrity-rewrite (when a platform-mismatched survivor needs a computed-integrity refresh) overwrites the same lockfile. It joins the in-flight write first, so the two never race the same atomic-write rename and the on-disk result is the rewrite — preserving the old serial "write, then catch-up rewrite" order. The post-link join surfaces a write error (including a task panic, wrapped distinctly) rather than dropping it.
Tests
install.bats: a byte-identical cold-install test that drives a fresh resolve with the overlap on (default) and off (AUBE_DISABLE_LOCKFILE_WRITE_OVERLAP=1) and diffs the resultingaube-lock.yaml— they must be byte-for-byte identical (the overlap changes when the write runs, never what).pnpm_lock_writecriterion bench measuringwrite_fullvsgraph_clone(feature-gated behindbench).cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings, theaube-lockfileunit tests, the hermetice2e.rs, and the install / lockfile / workspace / resolve bats files all pass.Summary by CodeRabbit
DISABLE_LOCKFILE_WRITE_OVERLAP.