perf(pm): drop redundant version_list / dist_tags clones on resolve hot path by elrrrrrrr · Pull Request #2877 · utooland/utoo

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

Summary

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.3× tighter), the biggest single-commit gain of the round-6 → round-12 saga.

4 files changed, +44/−24 lines.

(1) Drop redundant `manifest.versions.clone()`

`resolve_target_version` takes `&[String]`, so 4 call sites that prefixed 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.

Bench evidence (CI Linux ant-design / npmjs)

commit	indicatif	short-circuit	alloc cleanup	p1_resolve	σ
round-10 (`f122478b`)	TTY-gated	❌	❌	2.58s	0.07
round-11 (`5ca65e70`)	TTY-gated	✅	❌	2.59s	0.25
round-12 (`b2498e91`, this PR's content)	TTY-gated	✅	✅	2.46s	0.11

vs `utoo-next` baseline (5.19s ±0.14): −2.73s (−53%).

Test plan

`cargo build --release -p utoo-pm` clean
`cargo clippy -p utoo-ruborist -p utoo-pm --all-targets -- -D warnings --no-deps` clean
`cargo test -p utoo-ruborist`: 162 + 10 doctests pass
CI `pm-bench-phases-linux` on umbrella PR perf(pm): sequential writes within tarball extraction #2818 round-12 (`b2498e91`): p1=2.46s ±0.11 (commit ID `7b40566`)
CI `pm-bench-phases-linux` on this PR (clean against `next`) — expecting independent reproduction

🤖 Generated with Claude Code

…ot path 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<String> = 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<String>) and `manifest.dist_tags` (HashMap) per package fetch to fill the `versions_info` cache slot, while the same fields also moved into `Arc<FullManifest>` 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<FullManifest>` 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<FullManifest>` 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

gemini-code-assist

Code Review

This pull request aims to improve performance by using std::mem::take to move version and distribution tag data into a cache instead of cloning it. However, this approach causes the cached FullManifest objects to have empty fields, which breaks public functions and trait implementations that rely on that data. Additionally, the new architecture adds a DashMap lookup to the resolution path, which may negate the performance benefits of avoiding clones.

gemini-code-assist · 2026-04-30T11:18:16Z

-    let version_list: Vec<String> = manifest.versions.clone();
-
-    if version_list.is_empty() {
+    if manifest.versions.is_empty() {


This check (and the subsequent resolve_target_version call) will incorrectly fail with NoVersions if the manifest was obtained from a UnifiedRegistry cache. The mem::take optimization introduced in service/registry.rs empties the versions field of the cached manifest, breaking compatibility with this public function and any other external consumers of the FullManifest struct.

gemini-code-assist · 2026-04-30T11:18:16Z

+                        let version_list = std::mem::take(&mut manifest.versions);
+                        let dist_tags = std::mem::take(&mut manifest.dist_tags);


Using std::mem::take to move versions and dist_tags out of the FullManifest breaks the integrity of the cached Arc<FullManifest>. While internal methods like get_core_version work by parsing the raw bytes, any code accessing the versions or dist_tags fields directly on the manifest returned by fetch_full_manifest will now receive empty data. This is a breaking change for the public API and violates the expected behavior of the FullManifest struct.

Additionally, since UnifiedRegistry does not override the default implementation of fetch_versions_info in the RegistryClient trait (which calls fetch_full_manifest), that method is now also broken and will return empty lists when serving from cache.

gemini-code-assist · 2026-04-30T11:18:16Z

+                let versions_info = self.cache.get_versions(name).ok_or_else(|| {
+                    RegistryError(anyhow!("Versions cache not found for {}", name))
+                })?;


This introduces an additional DashMap lookup (involving hashing and shard locking) on the resolution hot path. Previously, this data was available directly on the FullManifest fields. While this lookup is required by the new architecture where fields are moved out of the manifest, it partially offsets the performance gains from avoiding clones by adding sequential overhead to the aggregator task.

elrrrrrrr · 2026-04-30T11:21:51Z

Closing per Gemini's review — the mem::take approach breaks two public API contracts:

pub fn resolve_from_manifest(&FullManifest, ...) silently returns NoVersions if the caller hands us a cache-returned manifest (now empty versions).
RegistryClient::fetch_full_manifest() trait contract is violated — its returned Arc<FullManifest> has empty versions / dist_tags fields, breaking any external consumer and the default fetch_versions_info impl that calls self.fetch_full_manifest().clone().

Both are real BC breaks, not just internal refactors.

Will re-open in two cleaner forms:

(A) strip to just the 4-site let version_list = manifest.versions.clone() removal — strictly correct, zero API impact, modest gain.
(C) lift FullManifest::versions to Arc<Vec<String>> (and dist_tags to Arc<HashMap<…>>) so cloning is a ref-count bump — preserves the API contract while keeping the −0.13s gain that came from the mem::take saving.

Bench evidence (PR #2818 round-12, p1=2.46s ±0.11 vs round-11 p1=2.59s ±0.25) carries forward.

github-actions · 2026-04-30T11:29:23Z

📊 pm-bench-phases · `9651431` · 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.22s	0.31s	10.07s	10.15s	674M	315.0K
utoo-next	9.48s	0.04s	11.28s	13.12s	1.28G	164.3K
utoo-npm	10.07s	0.09s	11.52s	12.95s	1.26G	153.3K
utoo	11.85s	2.39s	11.52s	13.42s	1.31G	169.3K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	15.2K	17.7K	1.17G	6M	1.85G	1.73G	1M
utoo-next	173.0K	150.7K	1.13G	4M	1.68G	1.68G	2M
utoo-npm	170.4K	158.0K	1.14G	4M	1.68G	1.68G	2M
utoo	194.2K	138.1K	1.13G	5M	1.68G	1.68G	2M

p1_resolve

PM	wall	±σ	user	sys	RSS	pgMinor
bun	2.22s	0.06s	3.87s	1.03s	495M	174.4K
utoo-next	5.50s	0.24s	6.14s	1.36s	436M	80.4K
utoo-npm	5.34s	0.25s	5.81s	1.01s	430M	75.3K
utoo	5.51s	0.20s	6.15s	1.44s	424M	75.0K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	10.0K	3.8K	202M	3M	106M	-	1M
utoo-next	97.3K	8.0K	197M	2M	7M	3M	2M
utoo-npm	66.5K	2.5K	203M	2M	9M	5M	2M
utoo	97.1K	8.5K	197M	2M	7M	3M	2M

p3_cold_install

PM	wall	±σ	user	sys	RSS	pgMinor
bun	7.02s	0.83s	6.09s	9.69s	593M	208.2K
utoo-next	7.19s	1.37s	5.34s	11.10s	871M	113.9K
utoo-npm	8.47s	3.07s	5.52s	11.42s	813M	111.6K
utoo	8.62s	3.44s	5.44s	11.47s	893M	113.1K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	5.7K	7.2K	993M	4M	1.73G	1.73G	1M
utoo-next	111.9K	78.2K	964M	3M	1.67G	1.67G	2M
utoo-npm	131.0K	91.9K	965M	3M	1.67G	1.67G	2M
utoo	129.7K	92.0K	965M	3M	1.67G	1.67G	2M

p4_warm_link

PM	wall	±σ	user	sys	RSS	pgMinor
bun	3.31s	0.25s	0.22s	2.29s	136M	32.3K
utoo-next	2.39s	0.06s	0.57s	3.80s	81M	19.2K
utoo-npm	2.42s	0.17s	0.59s	3.78s	83M	19.1K
utoo	2.32s	0.04s	0.56s	3.78s	83M	18.9K

PM	vCtx	iCtx	netRX	netTX	cache	node_mod	lock
bun	504	27	7M	32K	1.88G	1.70G	1M
utoo-next	48.6K	21.3K	17K	11K	1.67G	1.67G	2M
utoo-npm	49.1K	20.3K	16K	34K	1.67G	1.67G	2M
utoo	47.9K	20.5K	15K	11K	1.68G	1.67G	2M

npmmirror.com: no output captured.

…2878) * perf(pm): introduce VersionsRef<'a> view for resolve_target_version 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. * perf(pm): drop redundant in-memory `versions_info` fill on 200 path 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

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 closed this Apr 30, 2026

This was referenced Apr 30, 2026

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

Merged

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

Merged

Navigation Menu

Repository navigation

perf(pm): drop redundant version_list / dist_tags clones on resolve hot path - #2877

perf(pm): drop redundant version_list / dist_tags clones on resolve hot path#2877
elrrrrrrr wants to merge 1 commit into
nextfrom
perf/drop-version-list-clones

elrrrrrrr commented Apr 30, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Apr 30, 2026

Uh oh!

gemini-code-assist Bot Apr 30, 2026

Uh oh!

gemini-code-assist Bot Apr 30, 2026

Uh oh!

elrrrrrrr commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		let version_list = std::mem::take(&mut manifest.versions);
		let dist_tags = std::mem::take(&mut manifest.dist_tags);

Conversation

elrrrrrrr commented Apr 30, 2026

Summary

(1) Drop redundant `manifest.versions.clone()`

(2) `mem::take` version_list / dist_tags into VersionsInfo

Why this is more than alloc-cleanup

Bench evidence (CI Linux ant-design / npmjs)

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

elrrrrrrr commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

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

npmjs.org

p0_full_cold

p1_resolve

p3_cold_install

p4_warm_link

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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