Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

perf(package-manager): parallelise symlink layout with tarball fetch#277

Merged
zkochan merged 8 commits into
mainfrom
improve-perf3
Apr 24, 2026
Merged

perf(package-manager): parallelise symlink layout with tarball fetch#277
zkochan merged 8 commits into
mainfrom
improve-perf3

Conversation

@zkochan

@zkochan zkochan commented Apr 24, 2026

Copy link
Copy Markdown
Member

Summary

Adds per-snapshot parallelism between the two post-extract tasks (create_cas_files and create_symlink_layout) via rayon::join inside CreateVirtualDirBySnapshot::run.

  • Baseline shape preserved: CreateVirtualStore::run is still a try_join_all over snapshots, one tokio task per snapshot. No global symlink phase, no upfront clone, no spawn_blocking dance.
  • New parallelism: inside each snapshot's post-extract step, create_cas_files (rayon par_iter over files) and create_symlink_layout (serial loop over deps) run concurrently on rayon's pool. Neither closure needs data the other produces — CAS import reads cas_paths, symlinks read snapshot.dependencies — so overlapping them saves the serial symlink time per snapshot.
  • Error propagation cleanup: create_symlink_layout now returns Result<(), SymlinkPackageError> instead of expect-panicking on filesystem errors. CreateVirtualDirError grows a SymlinkPackage variant so a symlink failure surfaces as a structured miette diagnostic instead of crashing the process mid-install.

History note: Earlier commits on this branch (cb41db0 through 106f944) tried a "symlink-all ‖ fetch-all" global split to match pnpm's deps-restorer shape. Benchmarks on the 1352-package cold-install scenario showed a ~3% regression from the upfront snapshot.dependencies.clone() loop required for the spawn_blocking + 'static handoff. 1004683 reverts 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 green
  • cargo 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 s
  • Linux integrated-benchmark, FrozenLockfile scenario: pacquet@HEAD 2.981 ± 0.089 s vs pacquet@main 2.937 ± 0.038 s vs pnpm 6.563 ± 0.098 s

@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.79%. Comparing base (e974548) to head (1004683).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...kage-manager/src/create_virtual_dir_by_snapshot.rs 0.00% 16 Missing ⚠️
...rates/package-manager/src/create_symlink_layout.rs 0.00% 3 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.02     16.6±0.70ms   261.5 KB/sec    1.00     16.2±0.24ms   267.9 KB/sec

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::run into concurrent symlink and fetch/import branches via tokio::try_join!.
  • Simplifies InstallPackageBySnapshot to only download/extract and import CAS files (drops snapshot dependency).
  • Simplifies CreateVirtualDirBySnapshot to 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.

Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.584 ± 0.075 2.461 2.701 1.01 ± 0.05
pacquet@main 2.558 ± 0.097 2.469 2.764 1.00
pnpm 5.791 ± 0.079 5.658 5.906 2.26 ± 0.09
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
      ]
    }
  ]
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/package-manager/src/create_virtual_dir_by_snapshot.rs Outdated
Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
zkochan added 3 commits April 24, 2026 18:41
`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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/package-manager/src/create_virtual_dir_by_snapshot.rs Outdated
Comment thread crates/package-manager/src/create_symlink_layout.rs
Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
Comment thread crates/package-manager/src/install_package_by_snapshot.rs Outdated
`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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/package-manager/src/create_virtual_store.rs
zkochan added 2 commits April 24, 2026 20:44
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`.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/package-manager/src/create_symlink_layout.rs
Comment thread crates/package-manager/src/create_symlink_layout.rs Outdated
zkochan added 2 commits April 24, 2026 21:49
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/package-manager/src/create_virtual_dir_by_snapshot.rs
Comment thread crates/package-manager/src/create_virtual_store.rs
@zkochan zkochan merged commit af4b10d into main Apr 24, 2026
17 of 19 checks passed
@zkochan zkochan deleted the improve-perf3 branch April 24, 2026 20:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants