Repository navigation
perf(pm): drop redundant in-memory versions_info fill on 200 path - #2879
Merged
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the UnifiedRegistry to ensure that the full_manifests cache slot serves as the single source of truth in memory when a full manifest is fetched. Redundant calls to set_versions in the cache have been removed for both the 200 OK and manifest update paths, while maintaining the creation of VersionsInfo specifically for disk persistence. I have no feedback to provide as there were no review comments to evaluate.
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.30s | 0.27s | 10.23s | 10.38s | 719M | 318.8K |
| utoo-next | 10.02s | 0.53s | 11.70s | 13.47s | 1.24G | 165.7K |
| utoo-npm | 12.08s | 0.57s | 12.08s | 13.99s | 1.26G | 152.7K |
| utoo | 9.95s | 0.68s | 11.73s | 13.37s | 1.27G | 161.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 15.1K | 17.3K | 1.16G | 6M | 1.83G | 1.71G | 1M |
| utoo-next | 169.6K | 141.0K | 1.13G | 4M | 1.68G | 1.68G | 2M |
| utoo-npm | 188.7K | 174.6K | 1.14G | 5M | 1.68G | 1.68G | 2M |
| utoo | 170.9K | 118.5K | 1.13G | 4M | 1.68G | 1.67G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.15s | 0.18s | 3.96s | 1.06s | 479M | 166.6K |
| utoo-next | 5.06s | 0.07s | 6.20s | 1.51s | 439M | 80.4K |
| utoo-npm | 5.16s | 0.10s | 5.94s | 1.20s | 431M | 68.7K |
| utoo | 5.21s | 0.02s | 6.27s | 1.60s | 429M | 74.9K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 8.7K | 4.1K | 201M | 3M | 104M | - | 1M |
| utoo-next | 95.0K | 8.6K | 197M | 2M | 7M | 3M | 2M |
| utoo-npm | 64.6K | 3.2K | 201M | 2M | 9M | 5M | 2M |
| utoo | 94.9K | 8.7K | 197M | 2M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 7.81s | 1.11s | 6.11s | 10.04s | 638M | 216.3K |
| utoo-next | 9.16s | 2.36s | 5.62s | 12.08s | 909M | 116.7K |
| utoo-npm | 7.22s | 1.15s | 5.58s | 11.33s | 870M | 111.4K |
| utoo | 7.15s | 2.16s | 5.44s | 11.49s | 874M | 116.5K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 6.3K | 6.3K | 993M | 3M | 1.73G | 1.73G | 1M |
| utoo-next | 135.9K | 94.1K | 965M | 3M | 1.67G | 1.67G | 2M |
| utoo-npm | 106.3K | 63.9K | 964M | 2M | 1.67G | 1.67G | 2M |
| utoo | 112.6K | 84.2K | 964M | 3M | 1.67G | 1.67G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.42s | 0.12s | 0.23s | 2.34s | 137M | 32.9K |
| utoo-next | 2.43s | 0.08s | 0.64s | 3.94s | 82M | 18.5K |
| utoo-npm | 2.30s | 0.09s | 0.60s | 3.94s | 82M | 18.7K |
| utoo | 2.22s | 0.13s | 0.65s | 3.96s | 82M | 18.9K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 353 | 22 | 7M | 28K | 1.88G | 1.72G | 1M |
| utoo-next | 48.6K | 21.2K | 22K | 16K | 1.67G | 1.67G | 2M |
| utoo-npm | 48.4K | 21.3K | 21K | 10K | 1.67G | 1.67G | 2M |
| utoo | 49.4K | 22.2K | 23K | 12K | 1.68G | 1.67G | 2M |
npmmirror.com: no output captured.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to PR #2878 (
VersionsRef<'a>view abstraction). Now thatresolve_via_full_manifest's Full branch readsversions/dist_tagsdirectly from the cachedArc<FullManifest>via the view, the in-memoryversions_infocache slot is no longer needed on the 200 path — thefull_manifestsslot is the single source of truth in memory for fresh fetches.The
versions_infoslot 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).Stacked on top of #2878. Base:
next(logically depends on #2878 landing first since the read-side decoupling is what makes this safe).1 file changed, +13/−4 lines.
What changed
resolve_full_manifest's 200 branch and the corruption-fallback inside the 304 branch both stop callingself.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.Why this is safe (discriminator unchanged)
resolve_full_manifest's post-gate discriminator still works correctly:full_manifestspopulated → returnsFull(manifest)✅versions_infopopulated (filled in the 304 branch from the disk-loadedstore_versions) → returnsNotModified✅resolve_via_full_manifest::Fullreads throughVersionsRef::from(&Arc<FullManifest>)— doesn't touch theversions_infoslot.resolve_via_full_manifest::NotModifiedreadscache.get_versions(name)— only runs in the 304 case whereversions_infoIS filled.Bench expectation
Saves one
Arc::cloneper fetch (the in-memoryset_versionscall). The deep clones for disk persist are still in the worker closure — that's a separate, more invasive optimization (would requireManifestStore::store_versionsAPI 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, isolating
versions_infoslot to its single legitimate use case (304 / cross-run warm cache). Bench signal will be small (within σ); the cleanup is what matters.Why a separate PR
Per the saga-of-#2877:
mem::take, which brokeArc<FullManifest>field integrity → BC break that Gemini correctly flagged.VersionsRef<'a>view) so readers don't depend on which slot holds the data — but kept the write-side double-fill intact for safety.Each PR is independently reviewable and bench-able; the chain decouples the architectural concern (read-side abstraction in #2878) from the storage cleanup (this PR).
Test plan
cargo clippy -p utoo-ruborist -p utoo-pm --all-targets -- -D warnings --no-depscleancargo test -p utoo-ruborist: 162 + 10 doctests passcargo test -p utoo-pm: 245 passpm-bench-phases-linux— expecting neutral on p1_resolve (within σ); architectural cleanup, not perf optimization🤖 Generated with Claude Code