perf(store-dir)!: split bundled manifests into a dedicated SQLite table#417
perf(store-dir)!: split bundled manifests into a dedicated SQLite table#417zkochan wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
==========================================
- Coverage 84.55% 83.60% -0.95%
==========================================
Files 75 75
Lines 5159 5263 +104
==========================================
+ Hits 4362 4400 +38
- Misses 797 863 +66 ☔ 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": 2.4445699669,
"stddev": 0.06964548305444165,
"median": 2.4230739107000003,
"user": 2.5960527,
"system": 3.3336576399999998,
"min": 2.3716907837,
"max": 2.5630174677,
"times": [
2.4875805277,
2.5630174677,
2.4462767487,
2.3840606357,
2.3945861387,
2.4453182817,
2.3974621177,
2.5548774276999997,
2.3716907837,
2.4008295397
]
},
{
"command": "pacquet@main",
"mean": 2.4375492007000004,
"stddev": 0.06795551574211843,
"median": 2.4206841812,
"user": 2.5653703,
"system": 3.31648754,
"min": 2.3599268087,
"max": 2.5845777127000003,
"times": [
2.3883875667,
2.5845777127000003,
2.4927562667,
2.4031867387,
2.4501997427,
2.3784036937,
2.3974891087,
2.4823827447,
2.3599268087,
2.4381816237
]
},
{
"command": "pnpm",
"mean": 5.968964787599999,
"stddev": 0.09311319765448639,
"median": 5.945613613200001,
"user": 8.5880052,
"system": 4.29210434,
"min": 5.828678029700001,
"max": 6.120037825700001,
"times": [
6.0564777527,
5.9079579327000005,
5.9338038717,
6.120037825700001,
6.0900764037,
5.9115812297,
5.9009327847,
5.9826786907,
5.9574233547,
5.828678029700001
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6661397433000001,
"stddev": 0.030435398282017086,
"median": 0.6587197978,
"user": 0.33658592,
"system": 1.4959351599999997,
"min": 0.6369630058,
"max": 0.7472160688,
"times": [
0.7472160688,
0.6468830978,
0.6772250968,
0.6533831188,
0.6630544918,
0.6559250628,
0.6638222318,
0.6615145328,
0.6554107258,
0.6369630058
]
},
{
"command": "pacquet@main",
"mean": 0.6879085508,
"stddev": 0.025246882153742715,
"median": 0.6806128853,
"user": 0.37074122,
"system": 1.49983546,
"min": 0.6649780848,
"max": 0.7510136208,
"times": [
0.7510136208,
0.6858837498,
0.6810941398,
0.6705053178,
0.6716296828,
0.7092398108,
0.6862966448,
0.6649780848,
0.6783128258,
0.6801316308
]
},
{
"command": "pnpm",
"mean": 2.466723285,
"stddev": 0.08076456105735597,
"median": 2.4400869798,
"user": 2.88541892,
"system": 2.18387596,
"min": 2.3723147378,
"max": 2.5937095798,
"times": [
2.4124668768,
2.3723147378,
2.5937095798,
2.5544121048000004,
2.4455730548,
2.4000097618000003,
2.3821131488000002,
2.5229234948,
2.4346009048,
2.5491091858000003
]
}
]
} |
Resolves #330. Mirrors pnpm v11's `@pnpm/bins.resolver`, `@pnpm/bins.linker`, and `@zkochan/cmd-shim` (resolved at pnpm `4750fd370c` and cmd-shim `0d79ca9534`): - New `pacquet-cmd-shim` crate parses the `bin` field of a package manifest into commands. Both `bin: string | object` and the `directories.bin` glob fallback are supported, with the same path-traversal and URL-safe-name guards as upstream. Conflict resolution follows `pkgOwnsBin` + lexical compare. - Shim generator writes all three flavors per bin: Unix `/bin/sh` (byte-compatible with pnpm's `generateShShim`), Windows `.cmd` (CRLF, `%~dp0` paths), and PowerShell `.ps1` (`$basedir`/`$exe` header with Windows/POSIX-pwsh detection). Mirrors `generateCmdShim` and `generatePwshShim` minus the unused `nodePath`/`prependToPath`/`progArgs` branches. - All filesystem IO behind per-capability DI traits — `FsReadHead`, `FsReadFile`, `FsReadString`, `FsReadDir`, `FsWalkFiles`, `FsCreateDirAll`, `FsWrite`, `FsSetExecutable`, `FsEnsureExecutableBits` — following the pattern documented in [#332](#332 (comment)). `FsWrite` is a thin shim over `std::fs::write` and is **not atomic**; a future atomic-write capability can build on it without renaming. Production callers turbofish the unit-struct provider `::<RealApi>`; tests inject unit-struct fakes to cover IO error paths real fs can't trigger portably. Generic param is named `Api` (not `Fs`) so the same provider can grow non-fs capabilities without a rename. - New `LinkVirtualStoreBins` step walks each virtual-store slot and links its child packages' bins into the slot's own `node_modules/.bin`, matching `linkBinsOfDependencies` in pnpm's `building/during-install`. Slot iteration, per-slot child reads, and shim writes parallelised on rayon. - `SymlinkDirectDependencies` and `InstallWithoutLockfile` call the bin linker after the symlink layout is in place. Direct-dep linking and the symlink loop are exposed as free `link_direct_dep_bins` / `symlink_direct_deps_into_node_modules` functions so they're unit-testable without the mock-registry scaffold. Tests port the relevant `bins/resolver` and `bins/linker` cases from pnpm (`directories.bin` discovery, scoped bin names, dangerous locations, idempotent shim skip, conflict resolution) and add fakes-injected coverage for the IO error variants. **Not in this PR**: hoisted-bin precedence and lifecycle-script-created bins. Both depend on subsystems pacquet hasn't built yet — hoisting (`hoistPattern` / `publicHoistPattern` resolution + the lift step) and lifecycle scripts (`exec/lifecycle/` runner + `allowBuilds` plumbing). The bin-linking side already does the right thing for both (shims are paths, so they pick up files generated post-extract; the precedence rule is a small addition once hoisting tags candidates as hoisted). Tracked separately in with full porting coordinates. ## Performance When the bin-linking step landed it added ~7% to the warm-cache Frozen Lockfile install ([benchmark](#333 (comment))). Closing that gap took a sequence of changes that mirror how pnpm v11 avoids the same costs in its own `linkBinsOfDependencies` ([4750fd370c](https://github.com/pnpm/pnpm/blob/4750fd370c/building/during-install/src/index.ts#L258-L309)): - **Bundled manifests in `index.db`.** `extract_tarball_entries` captures the parsed `package.json`, picks the subset pnpm caches via a port of `normalizeBundledManifest` (`name`, `version`, `bin`, `directories`, `dependencies`, lifecycle `scripts`, …), and stuffs it into `PackageFilesIndex.manifest`. `prefetch_cas_paths` surfaces it back as a `HashMap<cache_key, Arc<Value>>` so the bin linker doesn't have to re-read `package.json` per child off disk. - **msgpackr-records encoder learns `serde_json::Value`.** The encoder previously errored with `ManifestNotSupported` if anything ever set `manifest`; now it records-encodes nested JSON objects (slot-allocated per distinct key set) so `useRecords: true` readers see them as JS `Object`s rather than `Map`s — necessary for pnpm's `manifest.bin` / `manifest.directories?.bin` property accesses to resolve. - **`Arc<Value>` for `PackageBinSource.manifest`.** The lockfile-driven bin-link path produces `slots × children` clones of the parsed manifest (~6k on the integrated-benchmark fixture). Sharing via `Arc` turns each push into a refcount bump. - **`hasBin` filter via lockfile.** `LinkVirtualStoreBins` now takes the lockfile `packages:` section, pre-computes an `Option<HashSet<PackageKey>>` of `hasBin: true` keys at install start, and skips snapshot children that aren't in the set *before* any path-building or manifest lookup. ~95% of a real lockfile's packages don't declare a bin, so this short-circuits the bulk of the per-slot work. `None` is reserved for the pathological case where the lockfile lacks a `packages:` section (fall back to processing every child); `Some(empty)` is authoritative — lockfile says no package has a bin, every slot short-circuits. Mirrors pnpm's filter at `building/during-install/src/index.ts:283`. - **Lockfile-driven slot iteration.** `run_lockfile_driven` walks `snapshots` directly and builds slot paths lexically. The previous `run_with_readdir` path enumerated every slot via `Api::read_dir(virtual_store_dir)` and probed each slot's own package directory with a wasted `read_dir` (~1267 wasted `open(O_DIRECTORY) + close` per warm install). The readdir-driven path survives as a fallback for `install_without_lockfile` and retains the existence probe there (small N, no virtual-store invariant from `create_virtual_dir_by_snapshot` to lean on). - **Parallel prefetch decode.** `StoreIndex::get_many_raw` returns undecoded row bytes; the SQLite mutex releases before `prefetch_cas_paths` fans the msgpackr decode + integrity check across rayon. Latest CI benchmark on this branch ([job 75462998251](https://github.com/pnpm/pacquet/actions/runs/25701679172/job/75462998251)): | Scenario | pacquet@HEAD | pacquet@main | Relative | |---|---|---|---| | Frozen Lockfile | 2.074s ± 102ms | 2.026s ± 70ms | +2.4% (within noise) | | Frozen Lockfile (Hot Cache) | **479.6ms ± 38ms** | **535.8ms ± 37ms** | **~10% faster than main** | The follow-up #417 splits manifests into a dedicated `package_manifests` SQLite table so the unfiltered `package_index` prefetch payload stays at its original size and the manifest fetch becomes a targeted SELECT against just the `hasBin: true` subset. --------- Co-authored-by: Zoltan Kochan <z@kochan.io>
a9df2a2 to
ddee5b6
Compare
Bundled manifests were embedded in each `package_index` row, making the cas-paths prefetch carry the full BundledManifest JSON tree through SQLite + msgpackr for every package on every install — ~1-5 KB extra per row, times ~1k rows on a real lockfile. The warm-cache install path doesn't need most of those manifests (only the ~5% of packages with `hasBin: true` ever get read at bin-link time), and decoding them eagerly serialised a big chunk of CPU under a single `spawn_blocking`. Schema change: add a `package_manifests` table to `index.db`, keyed identically to `package_index`. `StoreIndex::set` / `set_many` now write the cas-files row to `package_index` and the manifest row to `package_manifests` in the same transaction. `encode_package_files_index` no longer emits the `manifest` field; the standalone manifest is encoded by a new `encode_bundled_manifest` that lives in the same msgpackr-records module and round-trips through `decode_bundled_manifest`. Read side gains a targeted batched API: `StoreIndex::get_many_manifests(keys) -> Vec<(key, raw_bytes)>` SELECTs only `package_manifests`. The bin-link path in `CreateVirtualStore::run` now: 1. Filters snapshot entries against the lockfile's `hasBin` flag *before* hitting SQLite. 2. Fires the cas-paths and manifest prefetches concurrently via `tokio::join!`. Each one does a SQLite SELECT, then a rayon-parallel decode pass once the mutex releases. Effect: the unfiltered `package_index` payload is back to its pre-perf-fix size, and the manifest fetch is a much smaller targeted SELECT. pnpm v12 plans to adopt the same schema. Wire compat note: pacquet's writes no longer drop the manifest into pnpm's expected location (`pkgFilesIndex.manifest`); pnpm reading a pacquet-written row will see `manifest: undefined` and fall back to its disk-read path. pacquet reading a pnpm-written row works fine since `encode_package_files_index` already ignored `manifest` on the read side (the field decodes into the struct but is no longer used). Both tools converge once pnpm v12 ships with pacquet's schema. All 444 workspace tests pass.
ddee5b6 to
540dc6a
Compare
|
The difference is barely noticeable. It doesn't look this breaking change is justified in pnpm v12. Maybe we'll do it in a future version. |
Stacks on top of #333. Target branch is that PR's head, so the diff here is just the schema-split commit (
a9df2a2).What changes
index.dbgains a second table,package_manifests. The bundled manifest comes out of everypackage_indexrow and lives there instead. Both reads and writes split accordingly:StoreIndex::set/set_manyinsert the cas-files part intopackage_index(now manifest-free) and the manifest intopackage_manifests, in the same transaction. Themanifestfield onPackageFilesIndexsurvives as an input butencode_package_files_indexno longer emits it; a newencode_bundled_manifeststandalone-encodes the value with msgpackr-records wrapping so the on-wire shape stays a JS Object (not Map) underuseRecords: true.StoreIndex::get_many_manifests(keys) -> Vec<(key, raw_bytes)>SELECTs onlypackage_manifests. Callers decode viadecode_bundled_manifest.Why
PR #333's existing perf work already routes the install through a SQLite-backed manifest cache. The remaining problem visible in my local benchmarks was that the
package_indexrows were now ~3x bigger than before (full BundledManifest embedded), and the prefetch had to read+decode all of them on every install — even though only ~5% of packages declare abinfield.Splitting the tables means:
package_indexreturns the same bytes pnpm reads when it doesn't care about manifests.hasBin: true. On the integrated-benchmark fixture that's ~50–100 rows out of ~1267.tokio::join!, each ending with a rayon-parallel msgpackr decode pass after theSharedReadonlyStoreIndexmutex releases.Mirrors pnpm's filter at
building/during-install/src/index.ts:283(pnpm/pnpm@4750fd370c) on the read side; on the write side the schema change is breaking but @zkochan suggested adopting the same change in pnpm v12 where pacquet will ship with the pnpm CLI.Wire compat
manifestintopackage_index, so pnpm reading them seesbundledManifest: undefinedand falls back to its disk-read path. Not a crash, just suboptimal until pnpm v12.encode_package_files_indexalready ignored themanifestfield on the read side (the struct decodes it butStoreIndex::setre-encodes without it). pacquet would never see a manifest in pnpm-writtenpackage_indexrows after this lands, butdecode_bundled_manifestis the entry point pacquet uses anyway.Once pnpm v12 ships with pacquet's schema, both tools converge.
Tests
444 workspace tests pass.
crates/store-dir/src/msgpackr_records/tests.rscover the standalone encoder/decoder: simple round-trip, nested-object record-encoding (d4 72def count assertions), same-shape slot reuse, all JSON value kinds (covering the slot-byte fixint range).encode_package_files_index_drops_manifest_fieldpins the new contract: even if the input struct carries a manifest, thepackage_indexpayload doesn't.Status
Draft because the perf gain is best measured on Linux CI, not my local Mac. Pushing this so the CI machine can run the integrated-benchmark on the full stack.
Written by an agent (Claude Code, claude-opus-4-7).