From ab3684ed167a4ceba677b24ade5eff028f04741f Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 30 Apr 2026 19:15:11 +0800 Subject: [PATCH] perf(pm): drop redundant version_list / dist_tags clones on resolve hot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two orthogonal alloc-cleanup wins on the dependency-resolve hot path — neither touches behaviour or trait surface. CI bench-phases-linux on the umbrella branch (PR #2818 round-12 vs round-11) measured **−0.13s on p1_resolve with σ tightening from 0.25 → 0.11** (>2× tighter), the biggest single-commit gain of the round-6 → round-12 saga. ## (1) Drop redundant `manifest.versions.clone()` `resolve_target_version` takes `&[String]`, so 4 call sites prefixing it with `let version_list: Vec = manifest.versions.clone();` were paying for a pointless deep copy of the entire version list per resolve (50-200 entries on popular packages, ~600 packages on ant-design). Pass `&manifest.versions` / `&full_manifest.versions` directly. Saves ~80 String allocs × ~600 packages = ~48k allocs per ant-design install. Sites: - `crates/ruborist/src/traits/registry.rs` — default `fetch_version_manifest` + default `resolve_package` non-semver branch - `crates/ruborist/src/resolver/registry.rs` — `resolve_from_manifest` - `crates/pm/src/cmd/view.rs` — `utoo view` command ## (2) `mem::take` version_list / dist_tags into VersionsInfo The 200 path of `resolve_full_manifest` previously deep-cloned `manifest.versions` (Vec) and `manifest.dist_tags` (HashMap) per package fetch to fill the `versions_info` cache slot, while the same fields also moved into `Arc` for the `full_manifests` slot — two owners holding identical copies of the same data. Use `std::mem::take` to MOVE both fields out of `manifest` directly into the new `VersionsInfo`. The cached `Arc` keeps `raw` + scalar metadata; the canonical `version_list` / `dist_tags` now live in the `versions_info` cache slot only. `resolve_via_full_manifest`'s Full branch reads them via `cache.get_versions(name)` (same code path that filled the slot, atomic w.r.t. the inflight gate). `get_core_version(&resolved)` still works on the cached `Arc` because it parses the single version on demand from `raw` (preserved) — not from the moved-out `versions` field. Saves another ~80 String + 1 HashMap clone × ~600 packages per install. ## Why this is more than alloc-cleanup The `versions.clone()` / `dist_tags.clone()` calls run inside `resolve_full_manifest`'s `inflight_full.get_or_init` worker closure on the **main task / aggregator task** — not in the per-package resolver spawned futures. So even with worker-pool architecture (PR #2869), this clone bottleneck is sequential. `mem::take` directly removes that sequential work, which is why bench shows real wall-time savings instead of the noise-bound result alloc-cleanups normally produce in a parallel-resolve baseline. ## Test plan - [x] `cargo clippy -p utoo-ruborist -p utoo-pm --all-targets -- -D warnings --no-deps` clean - [x] `cargo test -p utoo-ruborist`: 162 + 10 doctests pass - [x] CI `pm-bench-phases-linux` on the umbrella branch (PR #2818 round-12, `b2498e91`): p1_resolve **2.46s ±0.11** vs round-11 (`5ca65e70`) **2.59s ±0.25** — −0.13s and σ tightened 2.3× - [ ] CI `pm-bench-phases-linux` on this PR (clean against `next`) — expecting independent reproduction --- crates/pm/src/cmd/view.rs | 3 +- crates/ruborist/src/resolver/registry.rs | 6 ++-- crates/ruborist/src/service/registry.rs | 41 ++++++++++++++++++------ crates/ruborist/src/traits/registry.rs | 18 ++++++----- 4 files changed, 44 insertions(+), 24 deletions(-) diff --git a/crates/pm/src/cmd/view.rs b/crates/pm/src/cmd/view.rs index d8355e9922..d5632a69af 100644 --- a/crates/pm/src/cmd/view.rs +++ b/crates/pm/src/cmd/view.rs @@ -27,10 +27,9 @@ pub async fn view(package_spec: &str) -> Result<()> { tracing::debug!("Fetched package info: {full_manifest:?}"); // Resolve version and get full VersionManifest (with all display fields) - let version_list: Vec = full_manifest.versions.clone(); let resolved_version = utoo_ruborist::resolver::version::resolve_target_version( &full_manifest.dist_tags, - &version_list, + &full_manifest.versions, version_spec, ) .map_err(|e| { diff --git a/crates/ruborist/src/resolver/registry.rs b/crates/ruborist/src/resolver/registry.rs index 6edeb9069c..91e2e47310 100644 --- a/crates/ruborist/src/resolver/registry.rs +++ b/crates/ruborist/src/resolver/registry.rs @@ -137,14 +137,12 @@ pub fn resolve_from_manifest( manifest: &FullManifest, spec: &str, ) -> Result> { - let version_list: Vec = manifest.versions.clone(); - - if version_list.is_empty() { + if manifest.versions.is_empty() { return Err(ResolveError::NoVersions(manifest.name.clone())); } // Resolve version using shared logic - let resolved_version = resolve_target_version(&manifest.dist_tags, &version_list, spec) + let resolved_version = resolve_target_version(&manifest.dist_tags, &manifest.versions, spec) .map_err(|e| ResolveError::Version(format!("{}@{}: {}", manifest.name, spec, e)))?; // Get manifest for resolved version (lazy: parse from Value on demand) diff --git a/crates/ruborist/src/service/registry.rs b/crates/ruborist/src/service/registry.rs index 0022a72e5e..4f946b171b 100644 --- a/crates/ruborist/src/service/registry.rs +++ b/crates/ruborist/src/service/registry.rs @@ -244,11 +244,19 @@ impl UnifiedRegistry { .await .map_err(RegistryError)? { - manifest::FetchManifestResult::Ok(manifest, new_etag) => { + manifest::FetchManifestResult::Ok(mut manifest, new_etag) => { + // MOVE version_list / dist_tags out of `manifest` + // into `VersionsInfo` instead of cloning. The cached + // `Arc` keeps `raw` + scalar metadata; + // canonical version_list / dist_tags now live in the + // `versions_info` cache slot only. Readers of these + // fields go through `cache.get_versions(name)`. + let version_list = std::mem::take(&mut manifest.versions); + let dist_tags = std::mem::take(&mut manifest.dist_tags); let versions_info = Arc::new(VersionsInfo { versions: Versions { - version_list: manifest.versions.clone(), - dist_tags: manifest.dist_tags.clone(), + version_list, + dist_tags, }, etag: new_etag, last_updated: current_timestamp_secs(), @@ -269,7 +277,7 @@ impl UnifiedRegistry { self.cache.set_versions(name.to_string(), versions_info); } else { // Persistent store corrupted/missing, fetch fresh (without etag). - let (manifest, new_etag) = manifest::fetch_full_manifest_fresh( + let (mut manifest, new_etag) = manifest::fetch_full_manifest_fresh( &self.registry_url, name, manifest::MetadataFormat::Abbreviated, @@ -277,10 +285,12 @@ impl UnifiedRegistry { .await .map_err(RegistryError)?; + let version_list = std::mem::take(&mut manifest.versions); + let dist_tags = std::mem::take(&mut manifest.dist_tags); let versions_info = Arc::new(VersionsInfo { versions: Versions { - version_list: manifest.versions.clone(), - dist_tags: manifest.dist_tags.clone(), + version_list, + dist_tags, }, etag: new_etag, last_updated: current_timestamp_secs(), @@ -429,12 +439,23 @@ impl UnifiedRegistry { ) -> Result<(String, CoreVersionManifest), RegistryError> { match self.resolve_full_manifest(name).await? { FullManifestResult::Full(full) => { - if full.versions.is_empty() { + // Canonical `version_list` / `dist_tags` live in the + // `versions_info` cache slot — the 200 path moved them + // there with `std::mem::take`. Read through the cache + // instead of the (now-empty) fields on the cached + // `Arc`. + let versions_info = self.cache.get_versions(name).ok_or_else(|| { + RegistryError(anyhow!("Versions cache not found for {}", name)) + })?; + if versions_info.versions.version_list.is_empty() { return Err(RegistryError(anyhow!("No versions available for {}", name))); } - let resolved_version = - resolve_target_version(&full.dist_tags, &full.versions, spec) - .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; + let resolved_version = resolve_target_version( + &versions_info.versions.dist_tags, + &versions_info.versions.version_list, + spec, + ) + .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; let core = full.get_core_version(&resolved_version).ok_or_else(|| { RegistryError(anyhow!( "Version {} not found in manifest for {}", diff --git a/crates/ruborist/src/traits/registry.rs b/crates/ruborist/src/traits/registry.rs index df8adac00f..c6ad7d532a 100644 --- a/crates/ruborist/src/traits/registry.rs +++ b/crates/ruborist/src/traits/registry.rs @@ -157,11 +157,11 @@ pub trait RegistryClient { ) -> impl Future, Self::Error>> { async move { let manifest = self.fetch_full_manifest(name).await?; - let version_list: Vec = manifest.versions.clone(); // Resolve version using shared logic - let resolved_version = resolve_target_version(&manifest.dist_tags, &version_list, spec) - .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; + let resolved_version = + resolve_target_version(&manifest.dist_tags, &manifest.versions, spec) + .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; manifest .get_core_version(&resolved_version) @@ -227,9 +227,8 @@ pub trait RegistryClient { fetch_spec ); let full_manifest = self.fetch_full_manifest(&fetch_name).await?; - let version_list: Vec = full_manifest.versions.clone(); - if version_list.is_empty() { + if full_manifest.versions.is_empty() { return Err(RegistryError(anyhow::anyhow!( "No versions available for {}", fetch_name @@ -237,9 +236,12 @@ pub trait RegistryClient { .into()); } - let resolved_version = - resolve_target_version(&full_manifest.dist_tags, &version_list, &fetch_spec) - .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; + let resolved_version = resolve_target_version( + &full_manifest.dist_tags, + &full_manifest.versions, + &fetch_spec, + ) + .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; let version_manifest = full_manifest .get_core_version(&resolved_version)