mirror of
https://github.com/pnpm/pnpm.git
synced 2026-05-24 16:46:06 -04:00
refactor(pacquet): address review comments on the perf branch
Three small cleanups flagged on #11837: 1. `install_with_fresh_lockfile.rs` — the `drop(resolver); drop(npm_resolver);` block didn't actually free the resolver-side caches because the local `meta_cache`, `fetch_locker`, and `picked_manifest_cache` `Arc` bindings each still held one strong reference of their own. Drop those bindings explicitly too so the cache shrinks before the install pass starts pulling tarballs. 2. `resolve_dependency_tree.rs` — the doc comment on `resolve_dependency_tree` said later visitors "skip the recursion," but the `Some(existing)` branch only AND-folds the `optional` flag and the code then unconditionally allocates a fresh `DependenciesTreeNode` and recurses on the children. Reword the comment to match what the code actually does (only the `ResolvedPackage` envelope is shared). 3. `npm_resolver.rs` — `build_resolve_result` had 9 positional arguments under `#[allow(clippy::too_many_arguments)]`. Drop the suppression and take a `BuildResolveResult` struct instead so the three call sites read as named-field literals and adding a future field doesn't require touching every caller.
This commit is contained in:
@@ -469,15 +469,19 @@ impl<'a, DependencyGroupList> InstallWithFreshLockfile<'a, DependencyGroupList>
|
||||
);
|
||||
|
||||
// Drop the resolver (and its packument cache) before the
|
||||
// install pass. Dropping `resolver` releases the strong
|
||||
// reference held by the `ArcResolver` wrapper; the standalone
|
||||
// `npm_resolver` binding holds a second strong reference
|
||||
// because the deno- and bun-resolvers were handed a clone of
|
||||
// the same `Arc` for their version-selection delegate. Drop
|
||||
// both so the `NpmResolver`'s meta cache is freed before the
|
||||
// install pass starts pulling tarballs into the CAFS.
|
||||
// install pass. Each `Arc` is cloned twice during resolver
|
||||
// construction (once into `NpmResolver`, once into
|
||||
// `NamedRegistryResolver`, then again into the deno- / bun-
|
||||
// resolvers via `Arc::clone(&npm_resolver)`), and the local
|
||||
// bindings each still hold one strong reference of their
|
||||
// own. Releasing every reference takes an explicit drop on
|
||||
// each binding — letting the cache shrink before the install
|
||||
// pass starts pulling tarballs into the CAFS.
|
||||
drop(resolver);
|
||||
drop(npm_resolver);
|
||||
drop(meta_cache);
|
||||
drop(fetch_locker);
|
||||
drop(picked_manifest_cache);
|
||||
|
||||
// Open the read-only SQLite index, spawn the batched writer,
|
||||
// and allocate the install-scoped `verifiedFilesCache`. Same
|
||||
|
||||
@@ -122,15 +122,15 @@ impl From<PatchKeyConflictError> for ResolveDependencyTreeError {
|
||||
///
|
||||
/// Resolves siblings in parallel via `try_join_all` at every level.
|
||||
/// The per-package dedupe gate is a shared `HashMap` behind a
|
||||
/// [`std::sync::Mutex`]: a sibling already resolving an id `X` makes
|
||||
/// later visitors skip the recursion the in-flight task is running and
|
||||
/// reuse the eventually-populated `ResolvedPackage`. The critical
|
||||
/// sections are short `HashMap` inserts with no `await` inside, so
|
||||
/// a sync mutex is the right tool — tokio's async mutex adds
|
||||
/// per-acquire overhead that the resolve hot path was paying once
|
||||
/// per visit per ctx field.
|
||||
/// Per-occurrence tree nodes are still allocated for each visit —
|
||||
/// only the `ResolvedPackage` envelope is shared.
|
||||
/// [`std::sync::Mutex`]: a second visitor to the same resolved id `X`
|
||||
/// AND-folds its `optional` flag into the existing
|
||||
/// [`ResolvedPackage`] envelope and reuses it. It still allocates a
|
||||
/// fresh [`DependenciesTreeNode`] for the current occurrence and
|
||||
/// recurses on `X`'s children — only the resolver-side envelope is
|
||||
/// shared. The critical sections are short `HashMap` inserts with no
|
||||
/// `await` inside, so a sync mutex is the right tool — tokio's async
|
||||
/// mutex adds per-acquire overhead that the resolve hot path was
|
||||
/// paying once per visit per ctx field.
|
||||
pub async fn resolve_dependency_tree<DependencyGroupList, Chain>(
|
||||
resolver: &Chain,
|
||||
manifest: &PackageManifest,
|
||||
|
||||
@@ -29,7 +29,7 @@ use pacquet_resolving_resolver_base::{
|
||||
};
|
||||
|
||||
use crate::{
|
||||
npm_resolver::{PickedFromRegistry, build_resolve_result},
|
||||
npm_resolver::{BuildResolveResult, PickedFromRegistry, build_resolve_result},
|
||||
parse_bare_specifier::{
|
||||
NamedRegistryPackageSpec, parse_named_registry_specifier_to_registry_package_spec,
|
||||
},
|
||||
@@ -147,17 +147,17 @@ impl<Cache: PackageMetaCache + 'static> NamedRegistryResolver<Cache> {
|
||||
// `@acme/private`), not the local alias. Callers that omit
|
||||
// an explicit alias (`pnpm add gh:@acme/foo`) still get the
|
||||
// right entry in `node_modules` and the lockfile.
|
||||
let result = build_resolve_result(
|
||||
&picked.meta,
|
||||
&picked.version,
|
||||
&spec,
|
||||
Some(spec.name.as_str()),
|
||||
NAMED_REGISTRY_RESOLVED_VIA,
|
||||
opts.published_by,
|
||||
opts.published_by_exclude.as_ref(),
|
||||
let result = build_resolve_result(BuildResolveResult {
|
||||
meta: &picked.meta,
|
||||
picked: &picked.version,
|
||||
spec: &spec,
|
||||
alias: Some(spec.name.as_str()),
|
||||
resolved_via: NAMED_REGISTRY_RESOLVED_VIA,
|
||||
registry,
|
||||
&self.picked_manifest_cache,
|
||||
)?;
|
||||
published_by: opts.published_by,
|
||||
published_by_exclude: opts.published_by_exclude.as_ref(),
|
||||
picked_manifest_cache: &self.picked_manifest_cache,
|
||||
})?;
|
||||
|
||||
Ok(Some(result))
|
||||
}
|
||||
|
||||
@@ -220,17 +220,17 @@ impl<Cache: PackageMetaCache + 'static> NpmResolver<Cache> {
|
||||
None => return Ok(None),
|
||||
};
|
||||
|
||||
let result = build_resolve_result(
|
||||
&picked.meta,
|
||||
&picked.version,
|
||||
&spec,
|
||||
wanted_dependency.alias.as_deref(),
|
||||
NPM_REGISTRY_RESOLVED_VIA,
|
||||
opts.published_by,
|
||||
opts.published_by_exclude.as_ref(),
|
||||
®istry,
|
||||
&self.picked_manifest_cache,
|
||||
)?;
|
||||
let result = build_resolve_result(BuildResolveResult {
|
||||
meta: &picked.meta,
|
||||
picked: &picked.version,
|
||||
spec: &spec,
|
||||
alias: wanted_dependency.alias.as_deref(),
|
||||
resolved_via: NPM_REGISTRY_RESOLVED_VIA,
|
||||
registry: ®istry,
|
||||
published_by: opts.published_by,
|
||||
published_by_exclude: opts.published_by_exclude.as_ref(),
|
||||
picked_manifest_cache: &self.picked_manifest_cache,
|
||||
})?;
|
||||
|
||||
Ok(Some(result))
|
||||
}
|
||||
@@ -268,17 +268,17 @@ impl<Cache: PackageMetaCache + 'static> NpmResolver<Cache> {
|
||||
None => return Ok(None),
|
||||
};
|
||||
|
||||
let result = build_resolve_result(
|
||||
&picked.meta,
|
||||
&picked.version,
|
||||
&jsr_spec.spec,
|
||||
Some(jsr_spec.jsr_pkg_name.as_str()),
|
||||
JSR_REGISTRY_RESOLVED_VIA,
|
||||
opts.published_by,
|
||||
opts.published_by_exclude.as_ref(),
|
||||
let result = build_resolve_result(BuildResolveResult {
|
||||
meta: &picked.meta,
|
||||
picked: &picked.version,
|
||||
spec: &jsr_spec.spec,
|
||||
alias: Some(jsr_spec.jsr_pkg_name.as_str()),
|
||||
resolved_via: JSR_REGISTRY_RESOLVED_VIA,
|
||||
registry,
|
||||
&self.picked_manifest_cache,
|
||||
)?;
|
||||
published_by: opts.published_by,
|
||||
published_by_exclude: opts.published_by_exclude.as_ref(),
|
||||
picked_manifest_cache: &self.picked_manifest_cache,
|
||||
})?;
|
||||
|
||||
Ok(Some(result))
|
||||
}
|
||||
@@ -387,18 +387,37 @@ pub(crate) struct PickedFromRegistry {
|
||||
pub(crate) version: PackageVersion,
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
/// Input bundle for [`build_resolve_result`]. Grouped so the
|
||||
/// 9-field signature stays a struct literal at the (3) call sites
|
||||
/// instead of a positional argument list that clippy flags as
|
||||
/// `too_many_arguments` (and that's painful to extend when the
|
||||
/// next field lands).
|
||||
pub(crate) struct BuildResolveResult<'a> {
|
||||
pub meta: &'a Package,
|
||||
pub picked: &'a PackageVersion,
|
||||
pub spec: &'a RegistryPackageSpec,
|
||||
pub alias: Option<&'a str>,
|
||||
pub resolved_via: &'a str,
|
||||
pub registry: &'a str,
|
||||
pub published_by: Option<DateTime<Utc>>,
|
||||
pub published_by_exclude: Option<&'a PackageVersionPolicy>,
|
||||
pub picked_manifest_cache: &'a crate::PickedManifestCache,
|
||||
}
|
||||
|
||||
pub(crate) fn build_resolve_result(
|
||||
meta: &Package,
|
||||
picked: &PackageVersion,
|
||||
spec: &RegistryPackageSpec,
|
||||
alias: Option<&str>,
|
||||
resolved_via: &str,
|
||||
published_by: Option<DateTime<Utc>>,
|
||||
published_by_exclude: Option<&PackageVersionPolicy>,
|
||||
registry: &str,
|
||||
picked_manifest_cache: &crate::PickedManifestCache,
|
||||
args: BuildResolveResult<'_>,
|
||||
) -> Result<ResolveResult, ResolveError> {
|
||||
let BuildResolveResult {
|
||||
meta,
|
||||
picked,
|
||||
spec,
|
||||
alias,
|
||||
resolved_via,
|
||||
registry,
|
||||
published_by,
|
||||
published_by_exclude,
|
||||
picked_manifest_cache,
|
||||
} = args;
|
||||
let pkg_name =
|
||||
PkgName::parse(picked.name.as_str()).map_err(|err| Box::new(err) as ResolveError)?;
|
||||
let version_str = picked.version.to_string();
|
||||
|
||||
Reference in New Issue
Block a user