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

perf(store-dir)!: split bundled manifests into a dedicated SQLite table#417

Closed
zkochan wants to merge 1 commit into
mainfrom
claude/perf/manifest-table-split
Closed

perf(store-dir)!: split bundled manifests into a dedicated SQLite table#417
zkochan wants to merge 1 commit into
mainfrom
claude/perf/manifest-table-split

Conversation

@zkochan

@zkochan zkochan commented May 11, 2026

Copy link
Copy Markdown
Member

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.db gains a second table, package_manifests. The bundled manifest comes out of every package_index row and lives there instead. Both reads and writes split accordingly:

  • Write sideStoreIndex::set / set_many insert the cas-files part into package_index (now manifest-free) and the manifest into package_manifests, in the same transaction. The manifest field on PackageFilesIndex survives as an input but encode_package_files_index no longer emits it; a new encode_bundled_manifest standalone-encodes the value with msgpackr-records wrapping so the on-wire shape stays a JS Object (not Map) under useRecords: true.
  • Read side — new StoreIndex::get_many_manifests(keys) -> Vec<(key, raw_bytes)> SELECTs only package_manifests. Callers decode via decode_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_index rows 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 a bin field.

Splitting the tables means:

  1. The unfiltered cas-paths prefetch payload is back to its pre-perf-fix size — the SELECT on package_index returns the same bytes pnpm reads when it doesn't care about manifests.
  2. The manifest fetch becomes a targeted SELECT against just the keys whose lockfile metadata sets hasBin: true. On the integrated-benchmark fixture that's ~50–100 rows out of ~1267.
  3. Both prefetches fire concurrently via tokio::join!, each ending with a rayon-parallel msgpackr decode pass after the SharedReadonlyStoreIndex mutex 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

  • pacquet → pnpm reads: pacquet rows no longer write manifest into package_index, so pnpm reading them sees bundledManifest: undefined and falls back to its disk-read path. Not a crash, just suboptimal until pnpm v12.
  • pnpm → pacquet reads: pacquet's encode_package_files_index already ignored the manifest field on the read side (the struct decodes it but StoreIndex::set re-encodes without it). pacquet would never see a manifest in pnpm-written package_index rows after this lands, but decode_bundled_manifest is the entry point pacquet uses anyway.

Once pnpm v12 ships with pacquet's schema, both tools converge.

Tests

444 workspace tests pass.

  • Bundled-manifest round-trip tests in crates/store-dir/src/msgpackr_records/tests.rs cover the standalone encoder/decoder: simple round-trip, nested-object record-encoding (d4 72 def count assertions), same-shape slot reuse, all JSON value kinds (covering the slot-byte fixint range).
  • encode_package_files_index_drops_manifest_field pins the new contract: even if the input struct carries a manifest, the package_index payload 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).

@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b07373c4-7b3d-45d8-b258-5d95c8288a0c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/perf/manifest-table-split

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 46.92308% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.60%. Comparing base (6862e70) to head (540dc6a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/store-dir/src/store_index.rs 45.31% 35 Missing ⚠️
crates/tarball/src/lib.rs 28.20% 28 Missing ⚠️
crates/package-manager/src/create_virtual_store.rs 62.50% 6 Missing ⚠️
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.
📢 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 May 11, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01     16.2±0.28ms   267.5 KB/sec    1.00     16.0±0.16ms   271.0 KB/sec

@github-actions

github-actions Bot commented May 11, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.445 ± 0.070 2.372 2.563 1.00 ± 0.04
pacquet@main 2.438 ± 0.068 2.360 2.585 1.00
pnpm 5.969 ± 0.093 5.829 6.120 2.45 ± 0.08
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 666.1 ± 30.4 637.0 747.2 1.00
pacquet@main 687.9 ± 25.2 665.0 751.0 1.03 ± 0.06
pnpm 2466.7 ± 80.8 2372.3 2593.7 3.70 ± 0.21
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
      ]
    }
  ]
}

@zkochan zkochan mentioned this pull request May 11, 2026
zkochan added a commit that referenced this pull request May 12, 2026
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>
Base automatically changed from claude/link-dependency-binaries-JWwJN to main May 12, 2026 00:07
@zkochan zkochan force-pushed the claude/perf/manifest-table-split branch from a9df2a2 to ddee5b6 Compare May 12, 2026 00:11
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.
@zkochan zkochan force-pushed the claude/perf/manifest-table-split branch from ddee5b6 to 540dc6a Compare May 12, 2026 00:13
@zkochan

zkochan commented May 12, 2026

Copy link
Copy Markdown
Member Author

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.

@zkochan zkochan closed this May 12, 2026
@zkochan zkochan deleted the claude/perf/manifest-table-split branch May 12, 2026 00:24
@zkochan zkochan added the performance Tasks that improve the overall performance of the project label May 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

performance Tasks that improve the overall performance of the project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant