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:
Zoltan Kochan
2026-05-22 02:07:59 +02:00
parent 3f9c1bb56f
commit e7b3e6ca45
4 changed files with 81 additions and 58 deletions

View File

@@ -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

View File

@@ -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,

View File

@@ -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))
}

View File

@@ -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(),
&registry,
&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: &registry,
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();