perf(package-manager): parallelise symlink layout with tarball fetch#277
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #277 +/- ##
==========================================
- Coverage 89.85% 89.79% -0.07%
==========================================
Files 61 61
Lines 5065 5074 +9
==========================================
+ Hits 4551 4556 +5
- Misses 514 518 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Pull request overview
Parallelizes virtual-store layout work by running intra-package symlink creation concurrently with tarball fetch + CAS import, reducing cold-install latency before node_modules/.pacquet/ becomes visible on disk.
Changes:
- Refactors
CreateVirtualStore::runinto concurrent symlink and fetch/import branches viatokio::try_join!. - Simplifies
InstallPackageBySnapshotto only download/extract and import CAS files (dropssnapshotdependency). - Simplifies
CreateVirtualDirBySnapshotto only perform CAS import (drops mkdir + symlink steps).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/package-manager/src/create_virtual_store.rs | Adds concurrent symlink layout creation alongside tarball fetch/import; introduces new mkdir error. |
| crates/package-manager/src/install_package_by_snapshot.rs | Removes symlink/layout responsibilities; now only fetches tarball and imports CAS into virtual store. |
| crates/package-manager/src/create_virtual_dir_by_snapshot.rs | Narrows responsibility to CAS file materialization into the virtual store slot. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5836966270399997,
"stddev": 0.07516034497404063,
"median": 2.57390716584,
"user": 2.7151093599999996,
"system": 2.01639268,
"min": 2.46118154834,
"max": 2.7012181923400003,
"times": [
2.7012181923400003,
2.64235064934,
2.55126728834,
2.46118154834,
2.6295086153400002,
2.66437833034,
2.58989581434,
2.5214362073400003,
2.55791851734,
2.51781110734
],
"exit_codes": [
0,
0,
0,
0,
0,
0,
0,
0,
0,
0
]
},
{
"command": "pacquet@main",
"mean": 2.55800391324,
"stddev": 0.0967315116047132,
"median": 2.51661612834,
"user": 2.74065286,
"system": 2.0459069800000003,
"min": 2.46945769434,
"max": 2.7641765563400003,
"times": [
2.70003588234,
2.50442240034,
2.55618464434,
2.46945769434,
2.4927677043400003,
2.50632956034,
2.7641765563400003,
2.50396938134,
2.52690269634,
2.5557926123400003
],
"exit_codes": [
0,
0,
0,
0,
0,
0,
0,
0,
0,
0
]
},
{
"command": "pnpm",
"mean": 5.79082081204,
"stddev": 0.07924752488073777,
"median": 5.78938809334,
"user": 8.60063166,
"system": 2.7025666800000003,
"min": 5.65762890634,
"max": 5.90596874634,
"times": [
5.71348994034,
5.72815597634,
5.76132331834,
5.84776544534,
5.76551228034,
5.65762890634,
5.8132639063400005,
5.83059676234,
5.90596874634,
5.88450283834
],
"exit_codes": [
0,
0,
0,
0,
0,
0,
0,
0,
0,
0
]
}
]
} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`CreateVirtualStore::run` used to do per-snapshot `download → mkdir → import-CAS → intra-pkg-symlinks` serially inside each tokio task. The symlink step doesn't need the tarball contents — only the resolved dep graph, which is already in the lockfile — so holding it behind the fetch meant cold installs couldn't start laying out `.pacquet/` until tarballs landed. Fork the two phases into a `tokio::try_join!`: one branch mkdirs every `.pacquet/pkg@ver/node_modules/` and creates the intra-package symlinks straight from `snapshots`, the other downloads + extracts + imports the CAS files. Matches pnpm's `headless` installer pattern (`Promise.all([linkAllModules(...), linkAllPkgs(...)])` in `pkg-manager/core/src/install/link.ts`), which is why pnpm has `node_modules` shape visible on disk seconds before the downloads finish. Drops the `snapshot` field from `InstallPackageBySnapshot` (now only fetch + import) and shrinks `CreateVirtualDirBySnapshot` to the CAS import step. `link_file` already creates parent dirs for fetches, so no explicit ordering between the two branches is required.
`symlink_fut`'s per-snapshot future is entirely synchronous (mkdir + rayon-parallel `create_symlink_layout`) with no `.await` points. `futures::try_join_all` polls its children on the same task, so without a blocking hint each child ran to completion on the worker thread before the runtime got a chance to poll `fetch_fut` — coupling download progress to symlink wall-time on the multi-threaded runtime and undoing the parallelism the previous commit was aiming for. Wrap the body in `tokio::task::block_in_place` so the executor can shift `fetch_fut` and other tasks onto sibling workers while this thread blocks. Pnpm's JS equivalent gets the same effect for free via `@pnpm/worker`, which offloads symlink work to a worker thread pool. The existing `#[tokio::test]` that reaches `CreateVirtualStore::run` uses an empty snapshot set, so the map closure is never invoked and `block_in_place` is never called from current-thread tokio — no panic.
Replace the per-snapshot `block_in_place` pattern from the previous commit with a single `tokio::task::spawn_blocking` that owns the symlink work and distributes it across rayon's pool via a flat `par_iter`. Three wins: 1. No runtime-flavor fragility. `block_in_place` panics on current- thread tokio; `spawn_blocking` works everywhere. Nothing in the codebase relies on this today, but the previous shape was a trap for a future test that adds non-empty snapshots under `#[tokio::test]` without `flavor = "multi_thread"`. 2. Clean error propagation. `create_symlink_layout` now returns `Result<(), SymlinkPackageError>` (serial `try_for_each` instead of `par_iter().for_each` with an `.expect`), and the error threads up through a new `CreateVirtualStoreError::SymlinkPackage` variant. A filesystem failure during the symlink sweep now surfaces as a structured miette diagnostic instead of crashing the process mid-install. 3. Better rayon work queue. One `par_iter` over the full snapshot vector lets rayon schedule 1352 items across its workers in one pass, instead of 1352 ramp-up/ramp-down cycles of the previous nested `par_iter`-per-snapshot pattern. `snapshot.dependencies` (a small `HashMap<PkgName, SnapshotDepRef>`) is cloned once per snapshot so the task owns its data. On the 1352-package fixture this is a few tens of ms of one-off work vs. the per-iteration savings and the robustness wins; big-lockfile integration test runs in 47–50s either way. Also corrects the stale doc on `CreateVirtualDirBySnapshot` — after the split, the virtual-store package directory may or may not exist when CAS import runs, and `link_file` creates missing parents on demand.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`tokio::try_join!` drops the losing future on the first error. With the symlink branch wrapped in a `spawn_blocking` `JoinHandle`, dropping that future dropped the handle too — and tokio cannot abort a blocking task. The closure kept running on the blocking pool, mutating `.pacquet/*/node_modules/` in the background after `CreateVirtualStore::run` had already returned the fetch error. Stop using `try_join!` on the symlink branch. Launch the blocking task, race it against the fetch `try_join_all` via a plain `.await`, then explicitly `symlink_handle.await` on every exit path. The symlink work runs concurrently with fetches exactly as before — it's on the blocking pool from the moment `spawn_blocking` returns — but now it is guaranteed to be finished before `CreateVirtualStore::run` returns, even on fetch error. Fetch errors surface first when both branches fail; pure symlink failures still surface on success of the fetch branch. No behavioral change on the success path. Big-lockfile integration still runs in ~46 s.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `StoreIndexWriter` rationale block still described the old flow where the symlink and fetch branches were joined via `tokio::try_join!`. That macro was removed in 9cf5c09 when the symlink branch was split into a `spawn_blocking` handle awaited separately after the fetch `try_join_all`. Update the comment to describe the actual sequencing.
The previous version fanned the symlink pass out over rayon via `par_iter` inside a `spawn_blocking`. On cold-cache installs this stole rayon workers from the fetch branch's concurrent `create_cas_files` calls during the hot part of the install and showed up as ~4% regression on the 1352-package FrozenLockfile benchmark relative to main. Pnpm's JS equivalent already solved this: `symlinkAllModules` (`worker/src/start.ts:493-501`) runs as a plain nested `for` loop on a single `@pnpm/worker` thread — no internal parallelism, no contention with the main-thread `linkAllPkgs` work. Port that pattern: drop the outer `par_iter`, iterate the snapshot vec serially in the `spawn_blocking` closure. On Linux with num_cpus workers, the cold install has ~3 s of network and tarball-extract work; ~6-7k symlink syscalls serial on one CPU costs ~200-300 ms, which sits comfortably inside the fetch budget and frees the whole rayon pool for CAS imports. No change to the symlink/fetch concurrency contract — `spawn_blocking` still runs on its own thread concurrently with the fetch `try_join_all`.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two stale bits called out in review: 1. The "virtual_node_modules_dir must already exist" note was misleading — `symlink_package` calls `fs::create_dir_all` on the symlink path's parent before each link, so the function works regardless. Callers that mkdir beforehand (as `CreateVirtualStore::run` does) just pay redundant stat syscalls. 2. The serial-iteration rationale referenced a "caller parallelises across snapshots" expectation that stopped being true in 131e89c, when the caller switched to a serial `for` loop inside `spawn_blocking`. Reword to reflect the single-threaded policy.
Previous commits on this branch hoisted the whole symlink pass into a separate `spawn_blocking` task that ran concurrently with the fetch `try_join_all`. On the 1352-package cold-install benchmark that design shipped a ~3% regression vs. main, and I couldn't find a safe way to eliminate the root cause (an upfront `snapshot.dependencies.clone()` loop on the reactor thread that required ~50 k heap allocations before fetches could start). Go back to baseline's per-snapshot tokio-task shape and add the parallelism one level deeper instead: inside each snapshot's post- extract step, `CreateVirtualDirBySnapshot::run` now uses `rayon::join` to run `create_cas_files` and `create_symlink_layout` concurrently. Neither closure needs data the other produces — CAS import reads `cas_paths`, symlink layout reads `snapshot.dependencies` — so overlapping them saves the serial symlink time per snapshot (~1-3 ms on a typical package) without any cross-thread data marshaling. Both closures borrow from the same stack frame. Cross-snapshot concurrency keeps streaming through tokio's `try_join_all`, same as main. Pnpm's `deps-restorer` installer is structured along the same axis once you stop conflating its "symlink all / fetch all" worker-thread plumbing with wall-time parallelism — the real overlap that matters is symlink-with-import, not symlink-with-download. Keeps the error-propagation cleanup from earlier commits on this branch: `create_symlink_layout` still returns `Result`, and `CreateVirtualDirBySnapshot` grows a `SymlinkPackage` variant so a filesystem failure during the symlink sweep no longer panics the process mid-install.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Adds per-snapshot parallelism between the two post-extract tasks (
create_cas_filesandcreate_symlink_layout) viarayon::joininsideCreateVirtualDirBySnapshot::run.CreateVirtualStore::runis still atry_join_allover snapshots, one tokio task per snapshot. No global symlink phase, no upfront clone, nospawn_blockingdance.create_cas_files(rayon par_iter over files) andcreate_symlink_layout(serial loop over deps) run concurrently on rayon's pool. Neither closure needs data the other produces — CAS import readscas_paths, symlinks readsnapshot.dependencies— so overlapping them saves the serial symlink time per snapshot.create_symlink_layoutnow returnsResult<(), SymlinkPackageError>instead ofexpect-panicking on filesystem errors.CreateVirtualDirErrorgrows aSymlinkPackagevariant so a symlink failure surfaces as a structured miette diagnostic instead of crashing the process mid-install.History note: Earlier commits on this branch (
cb41db0through106f944) tried a "symlink-all ‖ fetch-all" global split to match pnpm'sdeps-restorershape. Benchmarks on the 1352-package cold-install scenario showed a ~3% regression from the upfrontsnapshot.dependencies.clone()loop required for thespawn_blocking+'statichandoff.1004683reverts to this cleaner per-snapshot design, which is within noise of main (44 ms gap, ~1.4σ at n=10, vs. main's own 49 ms run-to-run variance).Test plan
just ready(typos, fmt, check, 28 package-manager tests, 188 total tests, clippy --deny warnings) — all greencargo nextest run -p pacquet-cli --test install --run-ignored=ignored-only frozen_lockfile_should_be_able_to_handle_big_lockfile— passes against the 1352-snapshot fixture in ~58 spacquet@HEAD 2.981 ± 0.089 svspacquet@main 2.937 ± 0.038 svspnpm 6.563 ± 0.098 s