perf(store-dir): share one read-only StoreIndex across cache lookups#261
Conversation
`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.
There was a problem hiding this comment.
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
StoreIndexhandle (Arc<Mutex<...>>) and a helper to open it once per install. - Threads the shared handle through package-manager install paths into
DownloadTarballToStorecache lookups. - Updates benchmarks/tests and crate dependencies to accommodate the new
store_indexplumbing.
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.
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
`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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
#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.
Summary
Fixes part 1 of #260.
DownloadTarballToStore::load_cached_cas_pathsopened a freshConnection::open_with_flags+PRAGMA busy_timeoutper 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