perf(pm): introduce VersionsRef<'a> view for resolve_target_version by elrrrrrrr · Pull Request #2878 · utooland/utoo

elrrrrrrr · 2026-04-30T11:42:38Z

Summary

Refactor the version-resolution read path to take a borrowed view (VersionsRef<'a>) 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.

pub struct VersionsRef<'a> {
    pub versions: &'a [String],
    pub dist_tags: &'a HashMap<String, String>,
}

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:

// 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)?;

8 files changed, +98/−42 lines.

Why this design (replaces the closed PR #2877)

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<FullManifest> 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<FullManifest> 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.

Why `VersionsRef<'a>` (not `'static` or `Arc<…>`)

Approach	Hot path cost	API impact	Issue
`'static` references	zero	API rigid	Forces `Box::leak` (memory leak on every fetch) or `LazyLock` (single global)
`Arc<Vec<String>>` field	atomic ref-bump per resolve	type signature widens	OK but ergonomics tax forever
`VersionsRef<'a>`	zero (borrow)	new type, callers adapt	None

Lifetime parameter is the idiomatic Rust solution: zero-cost, borrow-checker enforces correctness, and the lifetime never escapes a single function call (no propagation to long-lived structs). Most callers won't even need to write the 'a thanks to elision.

Drop redundant `manifest.versions.clone()` along the way

Four call sites had let version_list: Vec<String> = manifest.versions.clone(); just to feed the old &[String] parameter — now redundant since VersionsRef::from borrows. Dropped at:

traits/registry.rs — default fetch_version_manifest + non-semver resolve_package
resolver/registry.rs::resolve_from_manifest
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 will be small (within σ); the architectural cleanup is the main value here.

Naming choice

Considered VersionsView first but rejected — that conflicts with the *View family in model/package_json.rs (DepsView, IdentityView, PackageInstallView, ScriptsView, EnginesView) which are owned schema-deserialization subsets. VersionsRef follows Rust's standard borrowed/owned naming pair (OsStr/OsString, Path/PathBuf, str/String) — pairs naturally with the existing owned Versions struct in service/cache.rs.

Test plan

cargo clippy -p utoo-ruborist -p utoo-pm --all-targets -- -D warnings --no-deps clean
cargo test -p utoo-ruborist: 162 + 10 doctests pass
cargo test -p utoo-pm: 245 + 10 doctests pass
CI pm-bench-phases-linux — expecting neutral on p1_resolve (within σ); lays groundwork for the follow-up that consolidates the duplicate-storage on the 200 path.

🤖 Generated with Claude Code

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<String, String>, } 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<FullManifest>` 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<FullManifest>` 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<String> = 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.

gemini-code-assist

Code Review

This pull request introduces a VersionsRef struct to provide a unified, borrowed view of package versions and distribution tags. This refactoring allows the version resolution logic to operate on various manifest types—such as full manifests and cached version info—without unnecessary data cloning or code duplication across the codebase. I have no feedback to provide.

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<FullManifest>` 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<VersionsInfo>` 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

killagu

LGTM

`dd5d9583 perf(pm): gate ProgressReceiver on IsTerminal (...) (#2876)` landed on `next` while the umbrella PR was iterating. Conflict in `pm/util/logger.rs` was purely doc-comment wording — both branches shipped identical IS_TTY gating logic. Take next's wording (the canonical merged-PR text). The umbrella branch's `b2498e91 perf(pm): drop redundant Vec<String>/HashMap clones on resolve hot path` is now part of the diff vs `next`; the architectural follow-up (PR #2878 VersionsRef view + drop double-fill) is separately tracked.

github-actions · 2026-04-30T12:37:57Z

📊 pm-bench-phases · `b8823af` · linux (`ubuntu-latest`)

Workflow run — ant-design

PMs: utoo (this branch) · utoo-npm (latest published) · bun (latest)

npmjs.org

p0_full_cold

PM	wall	±σ	user	sys	RSS	pgMinor
bun	9.04s	0.15s	10.07s	10.00s	676M	317.8K
utoo-next	9.74s	0.91s	11.43s	13.17s	1.33G	165.8K
utoo-npm	11.36s	2.34s	11.63s	13.29s	1.23G	164.7K
utoo	10.28s	0.63s	11.28s	13.15s	1.20G	138.2K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	14.8K	17.4K	1.16G	6M	1.83G	1.72G	1M
utoo-next	178.7K	162.4K	1.13G	4M	1.68G	1.68G	2M
utoo-npm	178.3K	157.3K	1.14G	5M	1.68G	1.68G	2M
utoo	179.1K	119.2K	1.13G	4M	1.68G	1.67G	2M

p1_resolve

PM	wall	±σ	user	sys	RSS	pgMinor
bun	1.99s	0.07s	3.92s	1.03s	506M	164.9K
utoo-next	4.92s	0.04s	5.95s	1.44s	441M	75.3K
utoo-npm	5.07s	0.08s	5.83s	1.09s	433M	69.2K
utoo	5.04s	0.08s	5.94s	1.57s	424M	75.1K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	8.8K	4.1K	203M	3M	106M	-	1M
utoo-next	94.7K	8.6K	197M	2M	7M	3M	2M
utoo-npm	65.4K	3.0K	203M	2M	9M	5M	2M
utoo	94.4K	8.9K	197M	2M	7M	3M	2M

p3_cold_install

PM	wall	±σ	user	sys	RSS	pgMinor
bun	7.94s	1.27s	6.04s	9.75s	629M	207.3K
utoo-next	7.42s	1.58s	5.35s	11.22s	902M	121.1K
utoo-npm	7.01s	1.41s	5.43s	11.07s	966M	111.7K
utoo	9.97s	3.17s	5.54s	12.02s	923M	116.2K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	7.1K	6.1K	993M	4M	1.73G	1.73G	1M
utoo-next	107.1K	64.1K	964M	2M	1.67G	1.67G	2M
utoo-npm	105.0K	66.7K	964M	2M	1.67G	1.67G	2M
utoo	147.1K	100.4K	965M	4M	1.67G	1.67G	2M

p4_warm_link

PM	wall	±σ	user	sys	RSS	pgMinor
bun	3.24s	0.12s	0.20s	2.32s	136M	32.6K
utoo-next	2.31s	0.20s	0.60s	3.91s	83M	18.9K
utoo-npm	2.25s	0.09s	0.60s	3.91s	82M	19.0K
utoo	2.20s	0.12s	0.58s	3.92s	82M	18.5K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	324	38	7M	13K	1.88G	1.72G	1M
utoo-next	50.4K	21.9K	36K	11K	1.67G	1.67G	2M
utoo-npm	49.2K	22.0K	36K	10K	1.67G	1.67G	2M
utoo	49.2K	21.9K	37K	28K	1.68G	1.67G	2M

npmmirror.com: no output captured.

…fill Upstream `24d2e441 perf(pm): introduce VersionsRef<'a> view for resolve_target_version (#2878)` landed (covers both 63042d7 + 0f85360 that previously diverged on this umbrella branch). Conflict resolution: - Took next's `traits/registry.rs`, `service/registry.rs`, `resolver/registry.rs`, `pm/cmd/view.rs` versions — they have the canonical VersionsRef API. - **Re-applied** `MaybeSend` / `MaybeSync` trait shim (worker-pool needs `+ Send` on `tokio::spawn`'d futures, but trait can't carry that bound on wasm). - **Re-applied** sibling-spec short-circuit in `UnifiedRegistry::resolve_via_full_manifest` (umbrella-only feature borrowed from PR #2874): cache lookup before `get_core_version` reparse; return type tightens to `(String, Arc<CoreVersionManifest>)` to share the cached Arc. - **Re-applied** `MaybeSend/MaybeSync` bounds on `fetch_full_manifest`, `fetch_version_manifest`, `resolve_package` trait methods. - **Re-applied** `#[derive(Clone)]` on `MockRegistryClient` and `MockPackage` (worker-pool requires `R: Clone`). - Discarded the umbrella's `mem::take` versions/dist_tags (`b2498e91`) — superseded by next's drop-in-memory-double-fill which preserves `Arc<FullManifest>` integrity. cargo clippy --all-targets -- -D warnings clean. 245 pm + 162 ruborist tests pass (+ flaky `test_view_twice_no_304_error` unrelated to merge).

elrrrrrrr added the benchmark Run pm-bench on PR label Apr 30, 2026

gemini-code-assist Bot reviewed Apr 30, 2026

View reviewed changes

elrrrrrrr mentioned this pull request Apr 30, 2026

perf(pm): drop redundant in-memory versions_info fill on 200 path #2879

Merged

3 of 4 tasks

elrrrrrrr requested a review from killagu April 30, 2026 12:08

killagu approved these changes Apr 30, 2026

View reviewed changes

elrrrrrrr added the A-Pkg Manager Area: Package Manager label Apr 30, 2026

Merge branch 'next' into perf/versions-ref

86a230c

elrrrrrrr enabled auto-merge (squash) April 30, 2026 12:44

elrrrrrrr merged commit 24d2e44 into next Apr 30, 2026
32 checks passed

elrrrrrrr deleted the perf/versions-ref branch April 30, 2026 12:48

elrrrrrrr mentioned this pull request Apr 30, 2026

perf(pm): manifest resolve hot-path — rayon offload + network layer (h1+pool+dns) #2874

Merged

5 tasks done

elrrrrrrr mentioned this pull request Apr 30, 2026

perf(pm): rayon-only best snapshot (repro, do not merge) #2881

Closed

Navigation Menu

Repository navigation

perf(pm): introduce VersionsRef<'a> view for resolve_target_version - #2878

perf(pm): introduce VersionsRef<'a> view for resolve_target_version#2878
elrrrrrrr merged 3 commits into
nextfrom
perf/versions-ref

elrrrrrrr commented Apr 30, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

killagu left a comment

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

elrrrrrrr commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why this design (replaces the closed PR #2877)

Why VersionsRef<'a> (not 'static or Arc<…>)

Drop redundant manifest.versions.clone() along the way

Naming choice

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

killagu left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 30, 2026

📊 pm-bench-phases · b8823af · linux (ubuntu-latest)

npmjs.org

p0_full_cold

p1_resolve

p3_cold_install

p4_warm_link

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

elrrrrrrr commented Apr 30, 2026 •

edited

Loading

Why `VersionsRef<'a>` (not `'static` or `Arc<…>`)

Drop redundant `manifest.versions.clone()` along the way

📊 pm-bench-phases · `b8823af` · linux (`ubuntu-latest`)