mirror/pnpm
1
0
Fork 0
mirror of https://github.com/pnpm/pnpm.git synced 2026-05-25 09:08:43 -04:00
Code Issues Packages Projects Releases Wiki Activity

perf(pacquet/package-manager): retain prefetched manifests for fresh-install bin linking

Browse Source 
The fresh-lockfile install path was passing `None` for both
`snapshots` and `package_manifests` to `LinkVirtualStoreBins`,
falling back to the readdir-driven `run_with_readdir` that walks
`<virtual_store_dir>` (or `<store_dir>/links` under GVS) and reads
each child's `package.json` from disk per slot. The
warm-cache prefetch already pulled every snapshot's bundled
manifest out of the SQLite store index ([`PrefetchResult::manifests`]),
so the disk reads were redundant.

Wire the prefetched manifests through:

- Destructure `PrefetchResult` so `manifests` survives past the
  `await` (was being dropped on `let prefetched_cas_paths =
  prefetch.cas_paths`).
- Always build the freshly-resolved lockfile structure (was gated
  on GVS); both branches now reuse one `built_lockfile`. The build
  is ~3 ms on the alotta-files fixture and is what we save below
  anyway.
- Project `prefetched_manifests` (keyed by
  `<integrity>\t<pkg_id>`) into a `PackageManifests` map
  (`PkgNameVerPeer.without_peer() → Arc<Value>`) by walking the
  resolved graph and computing the cache key per node — same key
  shape as `pacquet_store_dir::store_index_key`.
- Drive `LinkVirtualStoreBins` with `snapshots: Some(...)` +
  `package_manifests: &map`; cold-batch packages (cache miss →
  downloaded fresh in this run) fall through to a per-child disk
  read just like before.

`packages: None` on purpose: the freshly-built lockfile's
`packages:` rows carry an incomplete `has_bin` because the
resolver's `PackageVersion` deserializer does not include the
`bin` field. Trusting the empty-by-omission `has_bin_set` here
would filter out every child and skip bin linking entirely.
Once `bin` is threaded through `PackageVersion`, the call site
can switch to `built_lockfile.packages.as_ref()` to recover the
~95% slot short-circuit the frozen-lockfile path enjoys.

Updated snapshots reflect the now-present
`<slot>/node_modules/<pkg>/node_modules/.bin/<pkg>` self-shim
the lockfile-driven path writes per pnpm's
`linkBinsOfDependencies`. The readdir path skipped it via
`link_bins_excluding` — that was the divergence the existing
`same_global_virtual_store_layout_*` parity tests called out;
the fresh path now matches.
This commit is contained in:
Zoltan Kochan
2026-05-22 01:47:49 +02:00
parent 57c3094e2d
commit b0bf597049
5 changed files with 109 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
pacquet/crates/cli/tests/snapshots/add__should_install_all_dependencies.snap
View File

@@ -1,6 +1,6 @@
---
source: crates/cli/tests/add.rs
assertion_line: 29
source: pacquet/crates/cli/tests/add.rs
assertion_line: 31
expression: get_all_folders(&workspace)
---
[
@@ -18,6 +18,8 @@ expression: get_all_folders(&workspace)
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules/.bin",
"node_modules/@pnpm.e2e",
"node_modules/@pnpm.e2e/hello-world-js-bin-parent",
]

6
pacquet/crates/cli/tests/snapshots/add__should_symlink_correctly.snap
View File

@@ -1,6 +1,6 @@
---
source: crates/cli/tests/add.rs
assertion_line: 61
source: pacquet/crates/cli/tests/add.rs
assertion_line: 68
expression: get_all_folders(&workspace)
---
[
@@ -18,6 +18,8 @@ expression: get_all_folders(&workspace)
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules/.bin",
"node_modules/@pnpm.e2e",
"node_modules/@pnpm.e2e/hello-world-js-bin-parent",
]

6
pacquet/crates/cli/tests/snapshots/install__should_install_dependencies.snap
View File

@@ -1,6 +1,6 @@
---
source: crates/cli/tests/install.rs
assertion_line: 48
source: pacquet/crates/cli/tests/install.rs
assertion_line: 49
expression: "(workspace_folders, store_files)"
---
(
@@ -19,6 +19,8 @@ expression: "(workspace_folders, store_files)"
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules",
"node_modules/.pnpm/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules/.bin",
"node_modules/@pnpm.e2e",
"node_modules/@pnpm.e2e/hello-world-js-bin-parent",
],

3
pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snap
View File

@@ -1,5 +1,6 @@
---
source: pacquet/crates/package-manager/src/install/tests.rs
assertion_line: 95
expression: get_all_folders(&project_root)
---
[
@@ -29,6 +30,8 @@ expression: get_all_folders(&project_root)
"node_modules/.pacquet/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules",
"node_modules/.pacquet/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e",
"node_modules/.pacquet/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin",
"node_modules/.pacquet/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules",
"node_modules/.pacquet/@pnpm.e2e+hello-world-js-bin@1.0.0/node_modules/@pnpm.e2e/hello-world-js-bin/node_modules/.bin",
"node_modules/@pnpm",
"node_modules/@pnpm/x",
"node_modules/@pnpm/xyz",

133
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
View File

@@ -527,13 +527,24 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList>
SharedVerifiedFilesCache::clone(&verified_files_cache),
)
.await;
let prefetched_cas_paths = prefetch.cas_paths;
// `side_effects_maps` is intentionally dropped: the fresh-
// lockfile path skips the build phase today (see the
// `importing_done` emit at the tail of this function), so
// there is no `is_built` gate to feed. Keep the binding name
// explicit so a future port that wires builds in does not
// miss the source.
let pacquet_tarball::PrefetchResult {
cas_paths: prefetched_cas_paths,
manifests: prefetched_manifests,
side_effects_maps: _,
} = prefetch;
tracing::info!(
target: "pacquet::install::phase",
phase = "prefetch_cas_paths",
elapsed_ms = phase_start.elapsed().as_millis() as u64,
cache_keys = cache_keys_len,
hits = prefetched_cas_paths.len(),
manifest_hits = prefetched_manifests.len(),
"phase complete",
);
@@ -574,20 +585,27 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList>
// way.
let allow_build_policy = AllowBuildPolicy::from_config(config)
.map_err(InstallWithFreshLockfileError::AllowBuildsPolicy)?;
// Build the freshly-resolved lockfile structure
// unconditionally: GVS needs `snapshots:` / `packages:` to
// compute the layout, and the bin-link pass below uses the
// same maps to drive the lockfile-driven
// `LinkVirtualStoreBins` path (skipping the per-slot
// `read_dir` enumeration and the per-child `package.json`
// read when the prefetched manifest is available). The build
// is cheap — ~3 ms on the alotta-files fixture — and it is
// what we end up saving below anyway.
//
// Named `built_lockfile` to keep it distinct from
// [`Self::wanted_lockfile`], which is the *previous* run's
// lockfile threaded in for preferred-versions seeding.
let phase_start = std::time::Instant::now();
let layout_lockfile = if config.enable_global_virtual_store {
Some(build_fresh_lockfile(config, manifest, &importer_result))
} else {
None
};
if config.enable_global_virtual_store {
tracing::info!(
target: "pacquet::install::phase",
phase = "build_fresh_lockfile",
elapsed_ms = phase_start.elapsed().as_millis() as u64,
"phase complete",
);
}
let built_lockfile = build_fresh_lockfile(config, manifest, &importer_result);
tracing::info!(
target: "pacquet::install::phase",
phase = "build_fresh_lockfile",
elapsed_ms = phase_start.elapsed().as_millis() as u64,
"phase complete",
);
let engine_name: Option<String> = if config.enable_global_virtual_store {
tokio::task::spawn_blocking(|| {
pacquet_graph_hasher::detect_node_major()
@@ -603,8 +621,8 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList>
let layout = VirtualStoreLayout::new(
config,
engine_name.as_deref(),
layout_lockfile.as_ref().and_then(|lockfile| lockfile.snapshots.as_ref()),
layout_lockfile.as_ref().and_then(|lockfile| lockfile.packages.as_ref()),
built_lockfile.snapshots.as_ref(),
built_lockfile.packages.as_ref(),
Some(&allow_build_policy),
);
if config.enable_global_virtual_store {
@@ -676,27 +694,69 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList>
link_bins::<Host>(&config.modules_dir, &config.modules_dir.join(".bin"))
.map_err(InstallWithFreshLockfileError::LinkBins)?;
// No prefetched manifests are available — fall back to the
// legacy readdir-driven path (slots discovered by walking
// `<virtual_store_dir>` or `<store_dir>/links` per the active
// layout, child manifests read from disk). The frozen-lockfile
// path skips both via [`LinkVirtualStoreBins::snapshots`] /
// `package_manifests`.
// Walk the resolved graph once and project the prefetched
// bundled-manifest map into a [`crate::PackageManifests`]
// keyed by `PkgNameVerPeer.without_peer()` — the same key
// shape the lockfile-driven bin linker uses. Peer-variants of
// the same `pkgIdWithPatchHash` share one prefetched entry
// because they share the same tarball / store-index row, so
// the first insert wins via `entry().or_insert_with`. Cold-
// batch packages (cache miss → downloaded fresh in this run)
// are absent from the map; `run_lockfile_driven` falls back
// to a per-child `package.json` read for those.
let package_manifests: crate::PackageManifests = {
let mut map: crate::PackageManifests =
HashMap::with_capacity(prefetched_manifests.len());
for node in importer_result.peers_result.graph.values() {
let pacquet_lockfile::LockfileResolution::Tarball(tarball) =
&node.resolve_result.resolution
else {
continue;
};
if tarball.git_hosted == Some(true) {
continue;
}
let Some(integrity) = tarball.integrity.as_ref() else { continue };
let Some(name_ver) = node.resolve_result.name_ver.as_ref() else { continue };
let pkg_id = format!("{}@{}", name_ver.name, name_ver.suffix);
let cache_key = pacquet_store_dir::store_index_key(&integrity.to_string(), &pkg_id);
let Some(manifest) = prefetched_manifests.get(&cache_key) else { continue };
let Ok(package_key) = node.dep_path.as_str().parse::<PackageKey>() else {
continue;
};
map.entry(package_key.without_peer()).or_insert_with(|| Arc::clone(manifest));
}
map
};
// Drive the lockfile-driven `LinkVirtualStoreBins` path. The
// bin linker iterates `snapshots:` (no per-slot `read_dir`)
// and reads each child's manifest from `package_manifests`
// (no per-child `package.json` disk read on warm hits) —
// same shape the frozen-lockfile path uses via
// [`crate::CreateVirtualStore::run`].
//
// The bin linker reuses the install-scoped `layout` above so
// GVS installs walk the shared `<store_dir>/links/...`
// directory instead of the project-local
// `<virtual_store_dir>/.pnpm` one.
let empty_manifests = std::collections::HashMap::new();
// `packages: None` on purpose: the freshly-built lockfile's
// `packages:` rows carry an incomplete `has_bin` because the
// resolver's `PackageVersion` deserializer does not include
// the `bin` field. Trusting the empty-by-omission
// `has_bin_set` here would filter out every child and skip
// bin linking entirely. With `packages: None` the bin linker
// falls through to "process every child" and lets each
// child's actual manifest (`bin` present or not) decide.
// Threading `bin` through `PackageVersion` is the proper
// fix; once that lands, pass
// `built_lockfile.packages.as_ref()` here to recover the
// ~95% slot short-circuit the frozen path enjoys.
//
// The fresh-lockfile path has no installability check yet
// (no `packages:` constraint eval), so the skip set is empty.
let empty_skipped = crate::SkippedSnapshots::new();
LinkVirtualStoreBins {
layout: &layout,
snapshots: None,
snapshots: built_lockfile.snapshots.as_ref(),
packages: None,
package_manifests: &empty_manifests,
// The without-lockfile path has no installability check
// (no `packages:` metadata to evaluate constraints
// against), so the skip set is empty by definition.
package_manifests: &package_manifests,
skipped: &empty_skipped,
}
.run()
@@ -722,16 +782,11 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList>
// current-lockfile pointing at an incomplete install — needs
// `.modules.yaml` to land first.
let wanted_lockfile = if config.lockfile {
// GVS already built one above for the layout — reuse it
// rather than walking the resolver graph again. When GVS
// is off, `layout_lockfile` is `None` and we build here.
let lockfile_to_save = layout_lockfile
.unwrap_or_else(|| build_fresh_lockfile(config, manifest, &importer_result));
let target = lockfile_dir.join(Lockfile::FILE_NAME);
lockfile_to_save
built_lockfile
.save_to_path(&target)
.map_err(InstallWithFreshLockfileError::SaveWantedLockfile)?;
Some(lockfile_to_save)
Some(built_lockfile)
} else {
None
};