Skip to content

perf(pm): execute resolver provider jobs#3066

Closed
elrrrrrrr wants to merge 4 commits into
perf/pm-review-resolver-provider-foundationfrom
perf/pm-review-resolver-provider-jobs
Closed

perf(pm): execute resolver provider jobs#3066
elrrrrrrr wants to merge 4 commits into
perf/pm-review-resolver-provider-foundationfrom
perf/pm-review-resolver-provider-jobs

Conversation

@elrrrrrrr
Copy link
Copy Markdown
Contributor

Summary

  • Implement provider job execution and placement helper sharing.
  • Require ManifestProvider at the builder boundary.
  • Thread warm project cache into resolver config.

Review Focus

  • Job result shape and warm-cache ownership before demand mainloop lands.

@elrrrrrrr elrrrrrrr added A-Pkg Manager Area: Package Manager benchmark Run pm-bench on PR labels May 25, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several structural changes to the resolver and registry services to support a new manifest provider model. Key changes include the addition of a warm_project_cache to the build configuration, the extraction of dependency processing logic into reusable helpers like process_dependency_with_resolved and handle_processed, and the implementation of the ManifestProvider trait for UnifiedRegistry. Feedback highlights that the new warm_project_cache appears unused in the current resolver logic and that process_dependency should be refactored to use the new shared helpers to ensure consistent semantics. Additionally, there are concerns regarding missing cache updates in execute_manifest_job and a regression in the store_version_manifest helper which now bypasses deduplication logic, potentially leading to redundant persistent storage writes.

/// Key `""` = default catalog, other keys = named catalogs.
pub catalogs: Catalogs,
/// Host-provided project cache used to seed resolver manifest state.
pub warm_project_cache: Option<ProjectCacheData>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The warm_project_cache field is added to BuildDepsConfig and set in api.rs, but it appears to be unused within the resolver logic in builder.rs. If it is intended to be used by the upcoming demand resolver, it might be better to keep it out of the core configuration until it has a consumer, or at least document where it is expected to be read.

///
/// The demand resolver uses this after a manifest job completes; the legacy
/// resolver path also goes through it so placement semantics stay shared.
pub fn process_dependency_with_resolved(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The PR description mentions that the legacy resolver path also goes through this new process_dependency_with_resolved helper to share placement semantics. However, the existing process_dependency function (which still drives the BFS phase in run_bfs_phase) has not been refactored to use this helper in this PR. To avoid code duplication and ensure consistent semantics, process_dependency should be updated to call this function after resolution.

let (manifest, speculative) =
manifest::parse_full_manifest_with_core_off_runtime(bytes, spec)
.await?;
let manifest = Arc::new(manifest);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When a fresh full manifest is fetched in execute_manifest_job, it is returned to the caller but not added to the provider's internal self.cache. Since UnifiedRegistry is also used as a RegistryClient in the current BFS resolver, failing to update the in-memory cache here will result in redundant network requests or re-parsing if the same package is subsequently resolved via the RegistryClient interface.

                        let manifest = Arc::new(manifest);
                        self.cache.set_full_manifest(name.to_string(), Arc::clone(&manifest));

Comment on lines +299 to +302
fn store_version_manifest(&self, name: &str, manifest: Arc<CoreVersionManifest>) {
let version = manifest.version.clone();
self.store.store_version_manifest(name, &version, manifest);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The new store_version_manifest helper bypasses the stored_version deduplication set. This is a regression compared to the legacy resolve_version_manifest path (see line 554), as it will perform redundant synchronous writes to the persistent store for the same package version across different jobs.

    fn store_version_manifest(&self, name: &str, manifest: Arc<CoreVersionManifest>) {
        let version = manifest.version.clone();
        if self.stored_version.insert((name.to_string(), version.clone())) {
            self.store.store_version_manifest(name, &version, manifest);
        }
    }

@elrrrrrrr elrrrrrrr force-pushed the perf/pm-review-resolver-provider-foundation branch from ffee403 to 9a331cb Compare May 25, 2026 09:44
@elrrrrrrr elrrrrrrr force-pushed the perf/pm-review-resolver-provider-jobs branch from f1d2e5f to 016f723 Compare May 25, 2026 09:45
@github-actions
Copy link
Copy Markdown

📊 pm-bench-phases · bd90e85 · 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.60s 0.41s 10.71s 10.58s 767M 341.7K
utoo-next 8.31s 0.16s 10.97s 12.66s 966M 126.6K
utoo-npm 8.58s 0.19s 11.31s 12.84s 1013M 127.4K
utoo 8.90s 0.25s 11.46s 12.70s 1001M 127.7K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 14.8K 18.7K 1.22G 6M 1.92G 1.81G 1M
utoo-next 114.8K 79.8K 1.19G 5M 1.77G 1.77G 2M
utoo-npm 121.4K 89.1K 1.19G 5M 1.77G 1.77G 2M
utoo 120.4K 66.3K 1.19G 6M 1.77G 1.77G 2M

p1_resolve

PM wall ±σ user sys RSS pgMinor
bun 1.92s 0.02s 4.25s 1.07s 515M 164.3K
utoo-next 2.96s 0.03s 5.58s 1.73s 623M 87.8K
utoo-npm 3.17s 0.11s 5.76s 2.12s 616M 79.9K
utoo 3.38s 0.04s 6.12s 2.00s 628M 91.1K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 7.8K 5.0K 206M 3M 109M - 1M
utoo-next 48.8K 72.6K 202M 2M 7M 3M 2M
utoo-npm 72.7K 92.2K 202M 2M 7M 3M 2M
utoo 56.1K 76.6K 205M 3M 7M 3M 2M

p3_cold_install

PM wall ±σ user sys RSS pgMinor
bun 7.00s 0.25s 6.62s 10.49s 614M 203.1K
utoo-next 6.45s 0.74s 5.19s 11.25s 479M 64.0K
utoo-npm 6.91s 1.29s 5.33s 11.23s 486M 63.2K
utoo 6.43s 1.04s 5.20s 11.02s 488M 65.6K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 4.7K 7.3K 1.02G 4M 1.82G 1.82G 1M
utoo-next 103.1K 50.9K 1017M 3M 1.76G 1.76G 2M
utoo-npm 102.3K 49.7K 1017M 3M 1.76G 1.76G 2M
utoo 93.2K 50.5K 1017M 2M 1.76G 1.76G 2M

p4_warm_link

PM wall ±σ user sys RSS pgMinor
bun 3.37s 0.09s 0.21s 2.42s 136M 33.1K
utoo-next 2.29s 0.14s 0.54s 3.94s 81M 18.9K
utoo-npm 2.52s 0.05s 0.53s 3.96s 82M 19.1K
utoo 2.31s 0.03s 0.57s 3.90s 81M 19.0K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 281 26 5M 10K 1.98G 1.81G 1M
utoo-next 41.8K 20.3K 343K 12K 1.76G 1.76G 2M
utoo-npm 42.1K 20.3K 341K 20K 1.76G 1.76G 2M
utoo 43.0K 20.9K 343K 28K 1.77G 1.76G 2M

npmmirror.com: no output captured.

@elrrrrrrr
Copy link
Copy Markdown
Contributor Author

Superseded: re-splitting #3028 from scratch (mirroring the #3029 installer split into #3075/#3076). Closing this draft to avoid a stale parallel split stack.

@elrrrrrrr elrrrrrrr closed this May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Pkg Manager Area: Package Manager benchmark Run pm-bench on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant