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

perf(store-dir): share one read-only StoreIndex across cache lookups#261

Merged
zkochan merged 8 commits into
mainfrom
perf/reuse-store-index
Apr 24, 2026
Merged

perf(store-dir): share one read-only StoreIndex across cache lookups#261
zkochan merged 8 commits into
mainfrom
perf/reuse-store-index

Conversation

@zkochan

@zkochan zkochan commented Apr 24, 2026

Copy link
Copy Markdown
Member

Summary

Fixes part 1 of #260. DownloadTarballToStore::load_cached_cas_paths opened a fresh Connection::open_with_flags + PRAGMA busy_timeout per snapshot — a 1352-package frozen-lockfile install paid 1352 connection-opens just to read cache hints, all serialized on the tokio blocking pool.

Open a `StoreIndex` once per install, wrap in `Arc<Mutex<…>>` (rusqlite `Connection` is `Send` but not `Sync`), and thread the handle through `CreateVirtualStore`, `InstallWithoutLockfile`, and `DownloadTarballToStore`. The mutex is taken only for the sub-millisecond `SELECT` and released before the per-file validation pass.

Follow-up #260 item — replace the per-file `symlink_metadata` storm with pnpm's `checkedAt`-based fast path — is left for a separate PR.

Test plan

  • `cargo test -p pacquet-tarball` — all 7 cache-hit / fall-through cases pass (`reuses_cached_cas_paths_when_index_entry_is_live`, `falls_through_when_cafs_file_missing`, `…_digest_is_malformed`, `…_cafs_path_is_a_directory`, `…_cafs_path_is_a_symlink`, plus the two download-path tests updated to `store_index: None`).
  • Bench on Cache lookup opens a fresh SQLite connection per snapshot; per-file stat storm on every install #260 repro (1352 packages, release, macOS APFS, warm CAS store): `rm -rf node_modules && pacquet install --frozen-lockfile` 17.4 s → 13.9 s. CPU utilization 135 % → 160 % as the serialized-open bottleneck clears.
  • `cargo fmt --all -- --check` clean.

`DownloadTarballToStore::load_cached_cas_paths` previously opened a
fresh SQLite `Connection::open_with_flags` + `PRAGMA busy_timeout` on
every snapshot, so a 1352-package frozen-lockfile install paid 1352
connection-opens just to read cache hints — all serialized on tokio's
blocking pool because `spawn_blocking` can't parallelize opens against
one file.

Open a `StoreIndex` once per install, wrap it in `Arc<Mutex<…>>`
(rusqlite `Connection` is `Send` but not `Sync`), and thread the handle
through `CreateVirtualStore`, `InstallWithoutLockfile`, and
`DownloadTarballToStore`. Each lookup now takes the mutex just for the
sub-millisecond `SELECT`, releases it before the per-file validation
pass, and frees the Arc when the install finishes.

Measured on the #260 repro (1352 packages, release build, macOS APFS,
warm CAS store): `rm -rf node_modules && pacquet install --frozen-
lockfile` drops from ~17.4 s to ~13.9 s, with CPU utilization rising
from ~135 % to ~160 % as the serialized-open bottleneck clears.

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

This PR improves install-time performance by avoiding reopening the SQLite index.db store index for each package cache lookup, instead sharing a single read-only StoreIndex handle across the install flow.

Changes:

  • Introduces a shared read-only StoreIndex handle (Arc<Mutex<...>>) and a helper to open it once per install.
  • Threads the shared handle through package-manager install paths into DownloadTarballToStore cache lookups.
  • Updates benchmarks/tests and crate dependencies to accommodate the new store_index plumbing.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tasks/micro-benchmark/src/main.rs Passes store_index: None into the benchmark tarball download path.
crates/tarball/src/lib.rs Adds optional shared store index to DownloadTarballToStore and uses it in cache lookups.
crates/store-dir/src/store_index.rs Defines SharedReadonlyStoreIndex and adds shared_readonly_in() opener.
crates/package-manager/src/install_without_lockfile.rs Opens shared read-only store index once per run and passes through recursive installs.
crates/package-manager/src/install_package_from_registry.rs Plumbs shared store index into tarball download.
crates/package-manager/src/install_package_by_snapshot.rs Plumbs shared store index into tarball download for lockfile snapshot installs.
crates/package-manager/src/create_virtual_store.rs Opens shared read-only store index once for snapshot install loop.
crates/package-manager/Cargo.toml Promotes pacquet-store-dir from dev-dependency to dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/package-manager/src/create_virtual_store.rs
Comment thread crates/tarball/src/lib.rs Outdated
@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.56522% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.75%. Comparing base (badb479) to head (4eb0431).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...package-manager/src/install_package_by_snapshot.rs 0.00% 8 Missing ⚠️
crates/tarball/src/lib.rs 76.00% 6 Missing ⚠️
crates/package-manager/src/create_virtual_store.rs 60.00% 4 Missing ⚠️
...es/package-manager/src/install_without_lockfile.rs 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
- Coverage   90.04%   89.75%   -0.30%     
==========================================
  Files          58       58              
  Lines        4471     4527      +56     
==========================================
+ Hits         4026     4063      +37     
- Misses        445      464      +19     

☔ 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     19.3±1.03ms   224.3 KB/sec    1.00     19.0±0.30ms   228.0 KB/sec

@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 56.0 ± 1.3 53.9 59.9 1.00
pacquet@main 60.0 ± 1.3 57.7 64.3 1.07 ± 0.03
pnpm 753.8 ± 4.1 749.6 762.8 13.45 ± 0.33
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.056029856662051285,
      "stddev": 0.001349833727650206,
      "median": 0.05614406238,
      "user": 0.03249852205128204,
      "system": 0.07325191743589743,
      "min": 0.05390867838,
      "max": 0.05989877338,
      "times": [
        0.05644475738,
        0.05594983638,
        0.05614406238,
        0.05630586638,
        0.05989877338,
        0.05686943638,
        0.058046870380000004,
        0.05709869738,
        0.05628683038,
        0.056627401380000005,
        0.05734985938,
        0.05758868838,
        0.05390867838,
        0.055334199380000004,
        0.05469035838,
        0.05559400738,
        0.05433092138,
        0.05560520838,
        0.05435175138,
        0.05629318238,
        0.05709111338,
        0.054055643380000004,
        0.05613458438,
        0.05730891238,
        0.05820379538,
        0.05480421238,
        0.05474295738,
        0.05632995138,
        0.05733554238,
        0.05531683638,
        0.05470396338,
        0.05394575838,
        0.05393379538,
        0.05630617638,
        0.05584778438,
        0.05581861038,
        0.05708443038,
        0.05479604638,
        0.05668490938
      ],
      "exit_codes": [
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.06004182956421054,
      "stddev": 0.0012912453255827245,
      "median": 0.06013402138,
      "user": 0.038435292631578945,
      "system": 0.0868776252631579,
      "min": 0.057659029380000004,
      "max": 0.06425335238,
      "times": [
        0.06041611538,
        0.06183002638000001,
        0.06069148338,
        0.06425335238,
        0.06077284738,
        0.05896264338,
        0.06010770438,
        0.05783651538,
        0.05907836038,
        0.05876470138,
        0.05915193838,
        0.061209327380000005,
        0.05996625038,
        0.06015634938,
        0.06037724138,
        0.05910597238,
        0.06048185038,
        0.05995500838,
        0.06106663038,
        0.06070454838,
        0.06079999638,
        0.06081443638,
        0.059846534380000004,
        0.06204777438,
        0.05803881438,
        0.058088739380000004,
        0.057659029380000004,
        0.060180613380000005,
        0.05973489438,
        0.05991998338,
        0.060830175380000004,
        0.06051191638,
        0.059107315380000004,
        0.05797087738,
        0.05950599438,
        0.06072429638,
        0.06080757238,
        0.060111693380000004
      ],
      "exit_codes": [
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0
      ]
    },
    {
      "command": "pnpm",
      "mean": 0.7538233855800001,
      "stddev": 0.0041386231934413065,
      "median": 0.75234988838,
      "user": 0.9877728399999999,
      "system": 0.16742372,
      "min": 0.7496345063800001,
      "max": 0.76277463838,
      "times": [
        0.76277463838,
        0.7564650223800001,
        0.75509248238,
        0.7514691873800001,
        0.7524396923800001,
        0.75747270738,
        0.7496345063800001,
        0.7501984573800001,
        0.75226008438,
        0.75042707738
      ],
      "exit_codes": [
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0
      ]
    }
  ]
}

* `create_virtual_store.rs`: bind `store_index.as_ref()` to a local and
  use `async move` so the closure's copy of the `Option<&…>` is moved
  into each future, rather than borrowing from the closure's
  per-iteration scope.
* `load_cached_cas_paths`: treat a poisoned index mutex as a cache miss
  rather than propagating the panic. The `SELECT` is stateless, so the
  prior panic can't have left the index in an inconsistent shape, and
  cache lookups are a best-effort hint — falling through to a fresh
  download is strictly safer than crashing every subsequent snapshot.

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 8 out of 8 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_store.rs Outdated
Comment thread crates/package-manager/src/install_without_lockfile.rs Outdated
Copilot flagged that `StoreIndex::shared_readonly_in` does synchronous
SQLite work (`Connection::open_with_flags` + `PRAGMA busy_timeout`) and
calling it directly in an `async fn` stalls the tokio reactor thread
for the duration of the open. It's once per install, not a hot spot,
but matches the pattern the store-index writer already uses and costs
us nothing to keep consistent.

Wrap both call sites (frozen-lockfile and no-lockfile install paths)
in `tokio::task::spawn_blocking` and await the result before kicking
off concurrent package work. Also moves `tokio` from `dev-dependencies`
into `dependencies` in package-manager's `Cargo.toml` since it's now a
runtime dep for the library code, not just tests.

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 8 out of 8 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_store.rs
Comment thread crates/package-manager/src/install_without_lockfile.rs Outdated
`spawn_blocking(...).await` returns `Result<_, JoinError>` which fires
on blocking-task panic or on cancellation during runtime shutdown.
Panicking on that turns a transient runtime issue into a hard crash
for an install that could otherwise succeed — cache lookups just miss.

Drop the `.expect(...)` and degrade to `None` via `.ok().flatten()`.
`shared_readonly_in` already yields `None` for a first-time install
against an empty store, and every downstream caller already handles
that shape, so this is the same failure mode the code already copes
with — just reached from a different source.

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 8 out of 8 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/install_without_lockfile.rs Outdated
Makes the recursive no-lockfile closure's captures explicit: bind
`&node_modules_path` to a local `node_modules_path_ref` (Copy) and
switch to `async move` so every captured variable is moved (copied,
for all the `&T` and `Option<&T>` captures) into the future instead
of borrowing the closure's per-iteration stack frame.

Matches the pattern already used in `create_virtual_store.rs` and in
the outer no-lockfile closure — keeps all three parallel-map sites
consistent.

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 8 out of 8 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_store.rs Outdated
Comment thread crates/package-manager/src/install_without_lockfile.rs Outdated
zkochan added 2 commits April 24, 2026 07:21
The silent `.ok().flatten()` hid two failure modes behind a cache miss:
a panic inside the blocking task, and task cancellation during runtime
shutdown. Neither is a crash-worthy error, but a blocking-task panic
during install really should be visible somewhere.

Promote the two call sites to a `match` and emit a `tracing::warn!` on
the `Err(JoinError)` branch before falling back to `None`. Uses the
`pacquet::install` target in common with the rest of this crate's
tracing. Degradation path is unchanged.

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 8 out of 8 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/tarball/src/lib.rs Outdated
Comment thread crates/tarball/src/lib.rs
Mirror the tracing change from the index-open call sites: when the
blocking cache-lookup task returns `Err(JoinError)`, emit a
`tracing::warn!` with the error and the `cache_key` before degrading
to `None`. Panic / cancellation stays diagnosable; the caller still
falls through to a fresh download.

Also drop the stale "msgpackr-records decoding is a follow-up"
wording from the cache-lookup rationale — `StoreIndex::get` has
decoded pnpm-written msgpackr-records since 0ea1252 (#251), and
`transcode_to_plain_msgpack` has handled it on the read side since
90be4f6 (#252). Simplify to "an undecodable entry … falls through
to the download path" so the comment reflects today's decoder.

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 8 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zkochan zkochan merged commit ee2fcba into main Apr 24, 2026
16 of 18 checks passed
@zkochan zkochan deleted the perf/reuse-store-index branch April 24, 2026 06:00
zkochan added a commit that referenced this pull request Apr 24, 2026
#265)

* refactor(store-dir): batch store-index writes via a single writer task

Port pnpm v11's `queueWrites` / `flush` / `setRawMany` pattern from
`store/index/src/index.ts` so the per-tarball store-index INSERT
stops opening a fresh SQLite connection and pulling its own
blocking-pool thread.

* Add `StoreIndex::set_many`, which wraps a batch in
  `BEGIN IMMEDIATE; INSERT OR REPLACE x N; COMMIT;`. One WAL fsync
  amortizes across the batch, with a rollback on per-batch error so
  a partial apply never leaves the index in a half-written state.
  Encoding happens on the caller's thread before the transaction
  begins, so the writer only pays SQLite work.
* Introduce `StoreIndexWriter`, an `Arc`-cloneable handle around a
  tokio unbounded channel. Producers call `queue(key, value)` — no
  SQLite, no `spawn_blocking`, no per-row lock. A single blocking
  task drains the channel, collects each non-blocking burst into a
  batch (cap 256), and flushes via `set_many`.
* `CreateVirtualStore` and `InstallWithoutLockfile` spawn the writer
  at the top of the run, thread the `Arc<StoreIndexWriter>` through
  `InstallPackageBySnapshot` / `InstallPackageFromRegistry` /
  `DownloadTarballToStore`, drop the orchestration's clone after
  `try_join_all`, then `await` the writer task so the final batch
  flushes before the install returns.
* `DownloadTarballToStore::run_without_mem_cache` drops its
  per-tarball `spawn_blocking` for the index write entirely.

Batch errors, `JoinError`s, and writer-open failures are all
downgraded to `warn!` and "skip the row", matching the read side's
graceful-degradation stance from #261. Tests pass against the
existing `pacquet-registry-mock` harness.

Context: opened while investigating #263. On macOS I haven't been
able to demonstrate a wall-time win from this change yet — the
measurable bottleneck in pacquet's cold-install sys time sits
elsewhere (most likely tarball extraction / `write_cas_file` and
sha512 per file), see the comment thread on #263 for the
investigation notes and sample profile. The change still stands
as structural cleanup and the pnpm-model alignment, and it will
compound with whatever fix the real bottleneck gets.

* refactor(store-dir): skip per-row encode failures in `set_many`

`collect::<Result<_, _>>()?` turned a single `encode_package_files_index`
failure into "drop the whole batch" — up to 256 otherwise-valid rows
lost because of one malformed `PackageFilesIndex`. That's stricter
than the "best-effort index" contract the writer task and the
read-side already follow.

Encode in a loop, log the failing key at `warn!`, and keep the rest
of the batch alive for commit. SQLite errors inside the transaction
still roll back the whole batch (that's how atomicity is supposed
to work); only the per-row encoding step is skipped.

Addresses Copilot review on #265.

* refactor(store-dir,tarball): reword #263 ref; quiet `queue` warn on first failure; fix private-doc-link

Three follow-up nits from the Copilot review on #265:

* The comment in `tarball/src/lib.rs` described #263 as "fixed" — it
  isn't, this PR is one step toward fixing it. Reworded to "the
  per-write blocking thread churn described in #263" so the
  citation survives the landing of both.

* `StoreIndexWriter::queue` logged a `warn!` on every `send`
  failure. When the writer task exits early (open failure, panic)
  every subsequent `queue` call fails — on a 1352-snapshot install
  that's a thousand identical warnings drowning everything else
  out. Add a `warn_on_send_failure: AtomicBool` one-shot guard:
  first failure logs (with "further failures silenced" hint),
  subsequent failures go silent. The observable signal the user
  actually cares about — "this install's rows aren't in the index,
  the next install will re-download" — surfaces on the next run
  regardless.

* `StoreIndexWriter`'s doc block linked to the private constant
  `MAX_BATCH_SIZE`. `RUSTDOCFLAGS=-D warnings` in CI rejects the
  private-intra-doc-link lint, so replace the link with a literal
  "256 entries" plus a `see MAX_BATCH_SIZE` pointer in prose.

Addresses Copilot review on #265.
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