From e7b3e6ca45d0f53a495d5bdb3d2c4858d2d85820 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 22 May 2026 02:07:59 +0200 Subject: [PATCH] refactor(pacquet): address review comments on the perf branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/install_with_fresh_lockfile.rs | 18 +++-- .../src/resolve_dependency_tree.rs | 18 ++--- .../src/named_registry_resolver.rs | 22 ++--- .../src/npm_resolver.rs | 81 ++++++++++++------- 4 files changed, 81 insertions(+), 58 deletions(-) diff --git a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs index 3bf97b0b50..f832cbe292 100644 --- a/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs @@ -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 diff --git a/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs b/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs index 42c0adac2f..2434111d0d 100644 --- a/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs +++ b/pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs @@ -122,15 +122,15 @@ impl From 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( resolver: &Chain, manifest: &PackageManifest, diff --git a/pacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rs b/pacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rs index 2a732782b4..653fe86019 100644 --- a/pacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rs +++ b/pacquet/crates/resolving-npm-resolver/src/named_registry_resolver.rs @@ -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 NamedRegistryResolver { // `@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)) } diff --git a/pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs b/pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs index 40d73451e1..5e788e7645 100644 --- a/pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs +++ b/pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs @@ -220,17 +220,17 @@ impl NpmResolver { 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 NpmResolver { 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>, + 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>, - published_by_exclude: Option<&PackageVersionPolicy>, - registry: &str, - picked_manifest_cache: &crate::PickedManifestCache, + args: BuildResolveResult<'_>, ) -> Result { + 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();