perf(pm): make unified registry stateless#3046
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors UnifiedRegistry to be stateless by removing the in-memory PackageCache and associated synchronization primitives, transitioning to a job-based manifest resolution model. Reviewers identified a critical bug where fetch_full_manifest fails on 304 Not Modified responses because the registry can no longer retrieve cached manifests. Further feedback notes potential performance regressions due to redundant I/O and network requests for non-exact version specs, and recommends restoring deleted unit tests to ensure logic coverage.
| async fn fetch_full_manifest(&self, name: &str) -> Result<Arc<FullManifest>, Self::Error> { | ||
| match self.resolve_full_manifest(name).await? { | ||
| FullManifestResult::Full(manifest) => Ok(manifest), | ||
| FullManifestResult::NotModified => { | ||
| // 304 in trait context: caller doesn't have versions cache access, | ||
| // so we return an error indicating the manifest is unchanged. | ||
| Err(RegistryError(anyhow!( | ||
| "No versions available for {} (304 Not Modified but no full manifest cached)", | ||
| name | ||
| ))) | ||
| } | ||
| match self | ||
| .execute_manifest_job(ManifestJob::Full { | ||
| name: name.to_string(), | ||
| spec: None, | ||
| }) | ||
| .await? | ||
| { | ||
| ManifestJobDone::Full { | ||
| data: | ||
| ManifestFullData::Full { | ||
| manifest, | ||
| speculative: _, | ||
| }, | ||
| .. | ||
| } => Ok(manifest), | ||
| ManifestJobDone::Full { | ||
| data: ManifestFullData::Versions(_), | ||
| .. | ||
| } => Err(RegistryError(anyhow!( | ||
| "No full manifest available for {} (304 Not Modified)", | ||
| name | ||
| ))), | ||
| ManifestJobDone::Version { .. } => Err(RegistryError(anyhow!( | ||
| "provider returned version manifest for full job {name}" | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of fetch_full_manifest is broken when a persistent store is used and the server returns a 304 Not Modified response.
Because UnifiedRegistry is now stateless and does not maintain an in-memory cache of full manifests, it cannot satisfy the RegistryClient contract (which requires returning an Arc<FullManifest>) when the server indicates the manifest hasn't changed. Since execute_manifest_job automatically uses the ETag from the store, subsequent calls to fetch_full_manifest for the same package will consistently fail with an error until the manifest is updated on the server.
For this compatibility wrapper, it is better to bypass the ETag-based job execution and fetch a fresh manifest if one is explicitly requested, or ensure the store can provide the full manifest bytes.
async fn fetch_full_manifest(&self, name: &str) -> Result<Arc<FullManifest>, Self::Error> {
let (manifest, etag) = manifest::fetch_full_manifest_fresh(
&self.registry_url,
name,
manifest::MetadataFormat::Abbreviated,
)
.await
.map_err(RegistryError)?;
let manifest = Arc::new(manifest);
let versions = Arc::new(VersionsInfo {
versions: Versions {
version_list: manifest.versions.clone(),
dist_tags: manifest.dist_tags.clone(),
},
etag,
last_updated: current_timestamp_secs(),
});
self.store.store_versions(name, versions);
Ok(manifest)
}| if deno_semver::Version::parse_from_npm(&fetch_spec).is_ok() | ||
| && let Some(manifest) = | ||
| self.store.load_version_manifest(&name, &fetch_spec).await | ||
| { | ||
| let manifest = Arc::new(manifest); | ||
| return Ok(ManifestJobDone::Version { | ||
| name, | ||
| spec, | ||
| manifest, | ||
| }); | ||
| } |
There was a problem hiding this comment.
For semver-supporting registries, resolution requests using tags (e.g., latest) or ranges will now always trigger a network request because they are not exact versions and thus skip the ManifestStore lookup.
Previously, these were cached in memory via inflight_version and PackageCache. While the resolver loop is expected to handle its own caching, this change significantly impacts the performance of direct RegistryClient calls for these registries. If this is intended, it might be worth documenting that UnifiedRegistry should be used with an external caching layer for optimal performance.
| let manifest = Arc::new( | ||
| manifest::parse_json_vec_off_runtime::<CoreVersionManifest>(bytes).await?, | ||
| ); | ||
| self.store_version_manifest(&name, Arc::clone(&manifest)); |
There was a problem hiding this comment.
The removal of stored_version deduplication state means that store_version_manifest is now called on every successful fetch or extraction. While the store operations are intended to be fire-and-forget, this can lead to a significant number of redundant asynchronous I/O tasks if multiple concurrent resolution jobs (e.g., for different ranges that resolve to the same version) finish around the same time.
If the caller (the resolver loop) does not handle deduplication of these side effects, consider if some form of lightweight deduplication is still needed here to protect the persistence layer.
| @@ -728,44 +488,7 @@ impl RegistryClient for UnifiedRegistry { | |||
|
|
|||
| #[cfg(test)] | |||
| mod tests { | |||
There was a problem hiding this comment.
Several valuable tests were removed in this PR, including test_unified_registry_executes_extract_manifest_provider_job which verified the core logic of execute_manifest_job. Even though the registry is now stateless, it is important to maintain tests that exercise the provider job execution paths and verify that they interact correctly with the ManifestStore and network utilities.
I recommend restoring or adapting these tests to use the new stateless architecture.
ef1f15b to
3f47d7e
Compare
e77fe0b to
a1e1d86
Compare
3f47d7e to
6f349ab
Compare
2d30f46 to
bdd2790
Compare
6f349ab to
8b02fd5
Compare
8b02fd5 to
ed9ee58
Compare
bdd2790 to
44add1d
Compare
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.54s | 0.29s | 10.50s | 10.56s | 760M | 348.3K |
| utoo-next | 8.16s | 0.27s | 10.63s | 12.18s | 920M | 124.8K |
| utoo-npm | 9.01s | 0.89s | 11.01s | 12.78s | 1016M | 124.6K |
| utoo | 7.99s | 0.12s | 11.27s | 12.39s | 1005M | 141.7K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 14.6K | 19.0K | 1.19G | 6M | 1.88G | 1.76G | 1M |
| utoo-next | 108.0K | 72.9K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo-npm | 130.2K | 94.4K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo | 106.8K | 61.0K | 1.16G | 6M | 1.71G | 1.70G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 1.96s | 0.10s | 4.12s | 1.02s | 514M | 158.3K |
| utoo-next | 2.86s | 0.03s | 5.42s | 1.71s | 611M | 92.9K |
| utoo-npm | 3.08s | 0.03s | 5.66s | 2.04s | 612M | 79.0K |
| utoo | 2.41s | 0.04s | 6.12s | 1.72s | 637M | 117.4K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 8.1K | 4.6K | 203M | 3M | 107M | - | 1M |
| utoo-next | 46.9K | 66.3K | 200M | 2M | 7M | 3M | 2M |
| utoo-npm | 71.0K | 88.2K | 200M | 2M | 7M | 3M | 2M |
| utoo | 15.6K | 20.1K | 202M | 3M | 7M | 3M | 3M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.84s | 0.13s | 6.32s | 10.06s | 609M | 207.0K |
| utoo-next | 6.03s | 0.07s | 5.06s | 10.96s | 477M | 63.3K |
| utoo-npm | 6.10s | 0.16s | 5.12s | 10.91s | 498M | 63.2K |
| utoo | 5.90s | 0.14s | 4.96s | 10.83s | 490M | 57.9K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 4.6K | 7.0K | 1019M | 3M | 1.76G | 1.76G | 1M |
| utoo-next | 96.0K | 47.4K | 988M | 3M | 1.70G | 1.70G | 2M |
| utoo-npm | 97.4K | 50.4K | 988M | 2M | 1.70G | 1.70G | 2M |
| utoo | 90.8K | 49.1K | 988M | 2M | 1.70G | 1.70G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.40s | 0.04s | 0.22s | 2.38s | 135M | 32.6K |
| utoo-next | 2.52s | 0.07s | 0.51s | 3.88s | 79M | 18.4K |
| utoo-npm | 2.41s | 0.06s | 0.53s | 3.83s | 79M | 18.3K |
| utoo | 2.38s | 0.03s | 0.53s | 3.87s | 79M | 18.4K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 507 | 26 | 5M | 33K | 1.91G | 1.73G | 1M |
| utoo-next | 42.5K | 20.4K | 2K | 9K | 1.70G | 1.70G | 2M |
| utoo-npm | 41.2K | 18.4K | 2K | 9K | 1.70G | 1.70G | 2M |
| utoo | 43.0K | 20.5K | 2K | 25K | 1.71G | 1.70G | 2M |
npmmirror.com: no output captured.
Summary
UnifiedRegistryownership ofPackageCache, OnceMap inflight gates, and stored-version dedupe stateRegistryClientcompatibility wrappersValidation
cargo fmtcargo check -p utoo-ruboristcargo test -p utoo-ruborist service::registry::tests -- --nocapturecargo test -p utoo-ruborist resolver::builder::tests::test_build -- --nocapturecargo clippy --all-targets -- -D warnings --no-depsSplit Plan
Part of the resolver stack split from source PR #3028. This follows the demand mainloop PR so cache/inflight ownership lives only in the resolver loop.