From 0f85360785352b69187d0e16c99b132750995f2a Mon Sep 17 00:00:00 2001 From: elrrrrrrr Date: Thu, 30 Apr 2026 20:00:51 +0800 Subject: [PATCH] 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); } }