From 63042d7709c8735e2ff27a2d5d2abafbb32420dc Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 30 Apr 2026 19:42:02 +0800 Subject: [PATCH 1/2] perf(pm): introduce VersionsRef<'a> view for resolve_target_version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor the version-resolution read path to take a borrowed view instead of two separate `&HashMap<…>` / `&[String]` arguments. The resolution logic only needs read access to a versions list + dist-tag map; everything else on a `FullManifest` (raw bytes, time, maintainers) is irrelevant. ```rust pub struct VersionsRef<'a> { pub versions: &'a [String], pub dist_tags: &'a HashMap, } impl<'a> From<&'a FullManifest> for VersionsRef<'a> { ... } impl<'a> From<&'a VersionsInfo> for VersionsRef<'a> { ... } impl<'a> From<&'a Versions> for VersionsRef<'a> { ... } ``` `resolve_target_version` now takes `VersionsRef<'_>`. Callers construct via `From` based on whichever in-memory shape they have: ```rust // 200-path: read directly from FullManifest let resolved = resolve_target_version((&*full).into(), spec)?; // 304-path: read from cached VersionsInfo let resolved = resolve_target_version((&*info).into(), spec)?; ``` ## Why this design (and what was rejected along the way) The prior `mem::take`-based approach (PR #2877) tried to dodge the deep clones at `resolve_full_manifest`'s 200 path by moving `versions` / `dist_tags` out of the cached `Arc` into the `VersionsInfo` slot. Gemini correctly identified that this broke two public API contracts: - `pub fn resolve_from_manifest(&FullManifest, …)` would silently return `NoVersions` if handed a cache-returned manifest. - `RegistryClient::fetch_full_manifest` trait contract is violated — its returned `Arc` would have empty fields, breaking external consumers and the default `fetch_versions_info` impl. The view abstraction sidesteps both problems: - `FullManifest::versions` / `dist_tags` stay as owned `Vec` / `HashMap` — fields remain populated, no API break, no `mem::take`. - The duplicate-storage bug (cache holds versions+dist_tags in BOTH `full_manifests` and `versions_info` slots, requiring a deep clone to fill the second) is now isolated to the *write* side. A follow-up PR can remove the duplicate fill on the 200 path; readers already dispatch via the view, so swapping the source is source-compat for them. ## Drop redundant `manifest.versions.clone()` along the way Four call sites had `let version_list: Vec = manifest.versions.clone();` just to feed the old `&[String]` parameter — now redundant since `VersionsRef::from` borrows. Dropped: `traits/registry.rs` (default `fetch_version_manifest` + non-semver `resolve_package`), `resolver/registry.rs::resolve_from_manifest`, and `pm/cmd/view.rs`. Saves ~80 String allocs × ~600 packages = ~48k allocs per ant-design install. Worker-pool baseline parallelises this work, so isolated bench signal is small (within σ of round-10 baseline) — the value is in the architectural cleanup that unblocks the follow-up. ## 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] `cargo test -p utoo-pm`: 245 + 10 doctests pass - [ ] CI `pm-bench-phases-linux` — neutral on p1_resolve (within σ), lays groundwork for the follow-up that does the actual storage consolidation. --- crates/pm/src/cmd/view.rs | 4 +- crates/ruborist/src/lib.rs | 4 +- crates/ruborist/src/model/manifest.rs | 38 ++++++++++++++++++ crates/ruborist/src/resolver/registry.rs | 6 +-- crates/ruborist/src/resolver/version.rs | 50 +++++++++++++++--------- crates/ruborist/src/service/cache.rs | 17 +++++++- crates/ruborist/src/service/registry.rs | 13 ++---- crates/ruborist/src/traits/registry.rs | 8 ++-- 8 files changed, 98 insertions(+), 42 deletions(-) diff --git a/crates/pm/src/cmd/view.rs b/crates/pm/src/cmd/view.rs index d8355e9922..e5bfaad79b 100644 --- a/crates/pm/src/cmd/view.rs +++ b/crates/pm/src/cmd/view.rs @@ -27,10 +27,8 @@ 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).into(), version_spec, ) .map_err(|e| { diff --git a/crates/ruborist/src/lib.rs b/crates/ruborist/src/lib.rs index 0ea3b3b42f..3ccd6937c5 100644 --- a/crates/ruborist/src/lib.rs +++ b/crates/ruborist/src/lib.rs @@ -41,7 +41,9 @@ pub mod graph { /// Package manifest types. pub mod manifest { - pub use crate::model::manifest::{CoreVersionManifest, Dist, FullManifest, VersionManifest}; + pub use crate::model::manifest::{ + CoreVersionManifest, Dist, FullManifest, VersionManifest, VersionsRef, + }; pub use crate::model::package_json::{ DepsView, EnginesView, IdentityView, PackageInstallView, PackageJson, PublishConfig, ScriptsView, diff --git a/crates/ruborist/src/model/manifest.rs b/crates/ruborist/src/model/manifest.rs index dea63dc7bc..e56168ee05 100644 --- a/crates/ruborist/src/model/manifest.rs +++ b/crates/ruborist/src/model/manifest.rs @@ -9,6 +9,44 @@ use std::sync::Arc; use serde::{Deserialize, Deserializer, Serialize}; use serde_json::Value; +/// Borrowed view of the data needed to resolve a version spec — a slice of +/// available versions plus a dist-tag map. +/// +/// The version-resolution logic ([`crate::resolver::version::resolve_target_version`]) +/// only needs read access to these two pieces of data; everything else on a +/// `FullManifest` (raw bytes, time map, maintainers, …) is irrelevant. By +/// borrowing them through a unified view we can serve the same resolver from +/// multiple in-memory shapes (a freshly-fetched `FullManifest`, a 304-cached +/// `VersionsInfo`, a disk-loaded `Versions`) without cloning data or +/// duplicating the resolution code. +/// +/// The lifetime parameter ties the view to whatever the caller is holding, +/// so the borrow checker statically rejects any attempt to keep the view +/// alive past its source. In practice every call site uses the view inside +/// a single function body — the lifetime never escapes. +/// +/// Construct via the `From` impls (defined alongside each source type): +/// ```ignore +/// // From a freshly-fetched manifest: +/// let view = VersionsRef::from(&*full_manifest); +/// // From the 304-path versions cache: +/// let view = VersionsRef::from(&*versions_info); +/// ``` +#[derive(Debug, Clone, Copy)] +pub struct VersionsRef<'a> { + pub versions: &'a [String], + pub dist_tags: &'a HashMap, +} + +impl<'a> From<&'a FullManifest> for VersionsRef<'a> { + fn from(m: &'a FullManifest) -> Self { + Self { + versions: &m.versions, + dist_tags: &m.dist_tags, + } + } +} + /// Skip on error - try to deserialize, return None if fails. /// This handles malformed npm registry data gracefully. fn skip_on_error<'de, D, T>(deserializer: D) -> Result, D::Error> diff --git a/crates/ruborist/src/resolver/registry.rs b/crates/ruborist/src/resolver/registry.rs index 6edeb9069c..145923160c 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.into(), 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/resolver/version.rs b/crates/ruborist/src/resolver/version.rs index 63c6046abe..8ca56bd8a7 100644 --- a/crates/ruborist/src/resolver/version.rs +++ b/crates/ruborist/src/resolver/version.rs @@ -1,10 +1,10 @@ //! Version resolution from dist-tags and version lists. -use std::collections::HashMap; +use crate::model::manifest::VersionsRef; use super::semver::{matches, max_satisfying}; -/// Resolve target version from dist-tags and version list. +/// Resolve target version from a [`VersionsRef`] view. /// /// This is the core version resolution logic that matches npm's behavior: /// 1. If spec is a dist-tag, return the tagged version @@ -12,13 +12,14 @@ use super::semver::{matches, max_satisfying}; /// 3. Otherwise, find the maximum satisfying version /// /// # Arguments -/// * `dist_tags` - Map of tag names to versions (e.g., {"latest": "1.2.3"}) -/// * `version_list` - List of all available versions +/// * `view` - Borrowed view of the versions list + dist-tag map. Construct via +/// `VersionsRef::from(&full_manifest)` or `VersionsRef::from(&versions_info)`. /// * `spec` - Version specification to resolve /// /// # Examples /// ``` /// use std::collections::HashMap; +/// use utoo_ruborist::manifest::VersionsRef; /// use utoo_ruborist::resolver::version::resolve_target_version; /// /// let mut dist_tags = HashMap::new(); @@ -27,15 +28,17 @@ use super::semver::{matches, max_satisfying}; /// let versions = vec!["1.0.0".to_string(), "1.2.3".to_string(), "1.5.0".to_string()]; /// /// // Prefer latest when it satisfies the spec -/// let result = resolve_target_version(&dist_tags, &versions, "^1.0.0"); +/// let view = VersionsRef { versions: &versions, dist_tags: &dist_tags }; +/// let result = resolve_target_version(view, "^1.0.0"); /// assert_eq!(result, Ok("1.2.3".to_string())); /// ``` -pub fn resolve_target_version( - dist_tags: &HashMap, - version_list: &[String], - spec: &str, -) -> Result { - if version_list.is_empty() { +pub fn resolve_target_version(view: VersionsRef<'_>, spec: &str) -> Result { + let VersionsRef { + versions, + dist_tags, + } = view; + + if versions.is_empty() { return Err(ResolveError::NoVersionsAvailable); } @@ -54,12 +57,12 @@ pub fn resolve_target_version( latest.to_string() }) .or_else(|| { - max_satisfying(version_list.iter().map(|s| s.as_str()), spec).map(|v| v.to_string()) + max_satisfying(versions.iter().map(|s| s.as_str()), spec).map(|v| v.to_string()) }); version.ok_or_else(|| ResolveError::NoMatchingVersion { spec: spec.to_string(), - available_count: version_list.len(), + available_count: versions.len(), }) } @@ -95,8 +98,17 @@ impl std::error::Error for ResolveError {} #[cfg(test)] mod tests { + use std::collections::HashMap; + use super::*; + fn view<'a>(versions: &'a [String], dist_tags: &'a HashMap) -> VersionsRef<'a> { + VersionsRef { + versions, + dist_tags, + } + } + #[test] fn test_resolve_dist_tag() { let mut dist_tags = HashMap::new(); @@ -111,11 +123,11 @@ mod tests { // Resolve dist-tag directly assert_eq!( - resolve_target_version(&dist_tags, &versions, "latest"), + resolve_target_version(view(&versions, &dist_tags), "latest"), Ok("1.2.3".to_string()) ); assert_eq!( - resolve_target_version(&dist_tags, &versions, "beta"), + resolve_target_version(view(&versions, &dist_tags), "beta"), Ok("2.0.0-beta.1".to_string()) ); } @@ -133,7 +145,7 @@ mod tests { // Should prefer latest (1.5.0) over max_satisfying (1.9.0) assert_eq!( - resolve_target_version(&dist_tags, &versions, "^1.0.0"), + resolve_target_version(view(&versions, &dist_tags), "^1.0.0"), Ok("1.5.0".to_string()) ); } @@ -151,7 +163,7 @@ mod tests { // latest doesn't satisfy ^2.0.0, should use max_satisfying (2.1.0) assert_eq!( - resolve_target_version(&dist_tags, &versions, "^2.0.0"), + resolve_target_version(view(&versions, &dist_tags), "^2.0.0"), Ok("2.1.0".to_string()) ); } @@ -161,7 +173,7 @@ mod tests { let dist_tags = HashMap::new(); let versions = vec!["1.0.0".to_string()]; - let result = resolve_target_version(&dist_tags, &versions, "^2.0.0"); + let result = resolve_target_version(view(&versions, &dist_tags), "^2.0.0"); assert!(result.is_err()); assert!(matches!( result, @@ -174,7 +186,7 @@ mod tests { let dist_tags = HashMap::new(); let versions: Vec = vec![]; - let result = resolve_target_version(&dist_tags, &versions, "^1.0.0"); + let result = resolve_target_version(view(&versions, &dist_tags), "^1.0.0"); assert_eq!(result, Err(ResolveError::NoVersionsAvailable)); } } diff --git a/crates/ruborist/src/service/cache.rs b/crates/ruborist/src/service/cache.rs index 7587b04f87..a983f4e447 100644 --- a/crates/ruborist/src/service/cache.rs +++ b/crates/ruborist/src/service/cache.rs @@ -41,7 +41,7 @@ use std::sync::{Arc, LazyLock}; use dashmap::DashMap; use serde::{Deserialize, Serialize}; -use crate::model::manifest::{CoreVersionManifest, FullManifest}; +use crate::model::manifest::{CoreVersionManifest, FullManifest, VersionsRef}; /// Lightweight versions info, persisted by `ManifestStore` for ETag validation. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -51,6 +51,12 @@ pub struct VersionsInfo { pub last_updated: u64, } +impl<'a> From<&'a VersionsInfo> for VersionsRef<'a> { + fn from(info: &'a VersionsInfo) -> Self { + VersionsRef::from(&info.versions) + } +} + /// Version list and dist-tags. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Versions { @@ -59,6 +65,15 @@ pub struct Versions { pub dist_tags: HashMap, } +impl<'a> From<&'a Versions> for VersionsRef<'a> { + fn from(v: &'a Versions) -> Self { + Self { + versions: &v.version_list, + dist_tags: &v.dist_tags, + } + } +} + // ============================================================================ // Memory cache (lock-free reads via DashMap) // ============================================================================ diff --git a/crates/ruborist/src/service/registry.rs b/crates/ruborist/src/service/registry.rs index 0022a72e5e..335f7d8fa5 100644 --- a/crates/ruborist/src/service/registry.rs +++ b/crates/ruborist/src/service/registry.rs @@ -432,9 +432,8 @@ impl UnifiedRegistry { if full.versions.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((&*full).into(), 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 {}", @@ -454,12 +453,8 @@ impl UnifiedRegistry { let versions_info = self.cache.get_versions(name).ok_or_else(|| { RegistryError(anyhow!("Versions cache not found for {}", name)) })?; - 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 resolved_version = resolve_target_version((&*versions_info).into(), spec) + .map_err(|e| RegistryError(anyhow!("{}@{}: {}", name, spec, e)))?; let manifest = manifest::fetch_version_manifest(manifest::FetchVersionManifestOptions { registry_url: &self.registry_url, diff --git a/crates/ruborist/src/traits/registry.rs b/crates/ruborist/src/traits/registry.rs index df8adac00f..48e29ad920 100644 --- a/crates/ruborist/src/traits/registry.rs +++ b/crates/ruborist/src/traits/registry.rs @@ -157,10 +157,9 @@ 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) + let resolved_version = resolve_target_version((&*manifest).into(), spec) .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; manifest @@ -227,9 +226,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 @@ -238,7 +236,7 @@ pub trait RegistryClient { } let resolved_version = - resolve_target_version(&full_manifest.dist_tags, &version_list, &fetch_spec) + resolve_target_version((&*full_manifest).into(), &fetch_spec) .map_err(|e| RegistryError(anyhow::anyhow!("{}@{}: {}", name, spec, e)))?; let version_manifest = full_manifest From 0f85360785352b69187d0e16c99b132750995f2a Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 30 Apr 2026 20:00:51 +0800 Subject: [PATCH 2/2] perf(pm): drop redundant in-memory `versions_info` fill on 200 path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to PR #2878 (`VersionsRef<'a>` view). Now that `resolve_via_full_manifest`'s Full branch reads `versions` / `dist_tags` directly from the cached `Arc` via the view, the in-memory `versions_info` cache slot is no longer needed on the 200 path — the `full_manifests` slot is the single source of truth in memory for fresh fetches. The `versions_info` slot now serves a single purpose: warm-cache storage for the 304 path (populated either by a previous run's disk store or by the current run's 304 hit). ## What changed `resolve_full_manifest`'s 200 branch and the corruption-fallback inside the 304 branch both stop calling `self.cache.set_versions(name, …)` after a successful 200 fetch. The `Arc` is still constructed for the disk-persist task — that path is unchanged, only the in-memory double-fill is gone. ## Discriminator unchanged The `resolve_full_manifest` post-gate discriminator still works: - 200 path: `full_manifests` populated → returns `Full` ✅ - 304 path: only `versions_info` populated (filled in 304 branch from disk-loaded `store_versions`) → returns `NotModified` ✅ ## Bench expectation Saves one `Arc::clone` per fetch (the in-memory `set_versions` call). The deep clones for disk persist are still in the worker closure — that's a separate, more invasive optimization (would require `ManifestStore::store_versions` API change to defer the clone into the spawned task) and is left for a future PR if perf becomes the goal. This PR's value is architectural: eliminating the duplicate in-memory representation of versions/dist_tags. Bench signal will be small (within σ); the cleanup is what matters. ## Test plan - [x] `cargo clippy --all-targets -- -D warnings --no-deps` clean - [x] `cargo test -p utoo-ruborist`: 162 + 10 doctests pass - [x] `cargo test -p utoo-pm`: 245 pass --- crates/ruborist/src/service/registry.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/ruborist/src/service/registry.rs b/crates/ruborist/src/service/registry.rs index 335f7d8fa5..2e0f6b720f 100644 --- a/crates/ruborist/src/service/registry.rs +++ b/crates/ruborist/src/service/registry.rs @@ -245,6 +245,15 @@ impl UnifiedRegistry { .map_err(RegistryError)? { manifest::FetchManifestResult::Ok(manifest, new_etag) => { + // Build a `VersionsInfo` strictly for the disk-persist + // task. We do NOT also fill the in-memory + // `versions_info` cache slot on the 200 path: readers + // (`resolve_via_full_manifest::Full`) now go through + // `VersionsRef::from(&Arc)` for this + // case, so the `full_manifests` slot is the single + // source of truth in memory. The `versions_info` slot + // is reserved for the 304 path (or disk-loaded warm + // cache from a previous run). let versions_info = Arc::new(VersionsInfo { versions: Versions { version_list: manifest.versions.clone(), @@ -255,8 +264,6 @@ impl UnifiedRegistry { }); self.cache .set_full_manifest(name.to_string(), Arc::new(manifest)); - self.cache - .set_versions(name.to_string(), Arc::clone(&versions_info)); // Fire-and-forget: store may spawn its own task. self.store.store_versions(name, versions_info); } @@ -277,6 +284,10 @@ impl UnifiedRegistry { .await .map_err(RegistryError)?; + // Same shape as the 200 branch: only the + // canonical `full_manifests` slot is filled in + // memory; the disk-persist task gets its own + // `Arc`. let versions_info = Arc::new(VersionsInfo { versions: Versions { version_list: manifest.versions.clone(), @@ -287,8 +298,6 @@ impl UnifiedRegistry { }); self.cache .set_full_manifest(name.to_string(), Arc::new(manifest)); - self.cache - .set_versions(name.to_string(), Arc::clone(&versions_info)); self.store.store_versions(name, versions_info); } }