perf(pm): add resolver demand manifest store#3079
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a demand-driven resolver state machine, staging the per-run manifest state, cache, waiters, and fetch queues. Two key issues were identified in the review comments: first, a logic error in pop_next where checking self.queued instead of self.active for demand jobs causes the prefetch concurrency limit check to always evaluate to false; second, an optimization opportunity in with_project_cache to reuse Arc allocations when multiple specs resolve to the same version, preventing redundant cloning of CoreVersionManifest.
elrrrrrrr
force-pushed
the
perf/pm-resolver-state
branch
from
523b1ea to
161a2d3
Compare
elrrrrrrr
changed the title
elrrrrrrr
force-pushed
the
perf/pm-resolver-state
branch
from
c92b972 to
92b0071
Compare
elrrrrrrr
changed the title
elrrrrrrr
force-pushed
the
perf/pm-resolver-state
branch
from
92b0071 to
c399b80
Compare
demand/state.rs: ManifestState — the per-run manifest store. ManifestSlot<K,V> (cache / waiters / failures) per manifest kind (full keyed by name, version by (name,spec)) + versions_cache, with get/set accessors, seeded(), and neutral ResolverManifestCache output. Pure storage: no fetch scheduling (queue) and no orchestration (driver) — those land in the follow-up PRs. Format-agnostic: speaks neutral (name,spec,manifest) tuples, so it never knows the on-disk ProjectCacheData format. Staged with #![allow(dead_code)] until the driver consumes it; exercised by its own unit tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
force-pushed
the
perf/pm-resolver-state
branch
from
c399b80 to
0e0e0fa
Compare
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.40s | 0.48s | 9.97s | 10.27s | 713M | 329.2K |
| utoo-next | 8.33s | 0.26s | 10.50s | 12.29s | 880M | 122.8K |
| utoo-npm | 9.54s | 1.85s | 10.68s | 12.49s | 901M | 119.8K |
| utoo | 8.43s | 0.55s | 10.38s | 12.36s | 847M | 128.8K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 15.7K | 17.7K | 1.11G | 6M | 1.77G | 1.66G | 1M |
| utoo-next | 118.3K | 94.8K | 1.08G | 5M | 1.62G | 1.61G | 2M |
| utoo-npm | 138.6K | 107.6K | 1.08G | 5M | 1.62G | 1.61G | 2M |
| utoo | 122.9K | 80.5K | 1.08G | 5M | 1.62G | 1.61G | 3M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.22s | 0.09s | 4.12s | 1.06s | 491M | 171.4K |
| utoo-next | 2.97s | 0.02s | 5.42s | 1.62s | 613M | 77.2K |
| utoo-npm | 3.14s | 0.02s | 5.59s | 1.97s | 617M | 78.2K |
| utoo | 3.00s | 0.04s | 5.42s | 1.70s | 618M | 84.0K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 9.5K | 4.3K | 205M | 3M | 110M | - | 1M |
| utoo-next | 48.7K | 83.9K | 202M | 2M | 7M | 3M | 2M |
| utoo-npm | 71.8K | 109.7K | 202M | 2M | 7M | 3M | 2M |
| utoo | 47.6K | 88.2K | 202M | 2M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.90s | 0.27s | 5.85s | 9.85s | 558M | 184.1K |
| utoo-next | 7.14s | 1.69s | 4.80s | 10.90s | 483M | 63.5K |
| utoo-npm | 7.41s | 1.87s | 4.96s | 11.00s | 482M | 56.0K |
| utoo | 7.63s | 2.63s | 4.80s | 10.85s | 487M | 60.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 5.2K | 7.0K | 934M | 3M | 1.67G | 1.67G | 1M |
| utoo-next | 101.7K | 53.2K | 905M | 3M | 1.61G | 1.61G | 2M |
| utoo-npm | 112.1K | 56.1K | 905M | 3M | 1.61G | 1.61G | 2M |
| utoo | 107.4K | 59.6K | 905M | 3M | 1.61G | 1.61G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.49s | 0.07s | 0.21s | 2.38s | 134M | 33.7K |
| utoo-next | 2.61s | 0.30s | 0.53s | 3.86s | 80M | 18.6K |
| utoo-npm | 2.32s | 0.08s | 0.50s | 3.81s | 80M | 18.6K |
| utoo | 2.36s | 0.07s | 0.51s | 3.79s | 80M | 18.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 464 | 23 | 5M | 21K | 1.82G | 1.66G | 1M |
| utoo-next | 43.0K | 19.1K | 47K | 7K | 1.61G | 1.61G | 2M |
| utoo-npm | 42.4K | 19.3K | 49K | 11K | 1.61G | 1.61G | 2M |
| utoo | 40.6K | 18.8K | 48K | 23K | 1.62G | 1.61G | 2M |
npmmirror.com: no output captured.
This was referenced May 27, 2026
elrrrrrrr
added a commit
that referenced
this pull request
May 28, 2026
Type-level scaffolding for the demand-driven resolver rework (#3028 / #3084). The trait the demand driver dispatches through — `service::ManifestProvider` — and its registry-backed adapter (`impl ManifestProvider for UnifiedRegistry` in the new `service::registry::provider` module) land here unreferenced. The driver that consumes the trait is the next PR in this stack (`resolver/demand/driver.rs`, ~700 lines). The flip of the public entry-points (`build_deps_*` and `resolve_*` in `resolver::builder`, plus the `api::build_lockfile` host call site) from the legacy `RegistryClient` bound to the new `ManifestProvider` bound is the third PR, and the two runtime tunings — the HTTP-client pool that fans connections across Cloudflare edge IPs, and the resolver-side `get_resolver_manifests_concurrency_limit` knob that raises the in-flight cap for non-semver registries (npmjs) from the existing 64 (tarball-side `get_manifests_concurrency_limit`) to 256 — ride along with the cutover. They're scoped that way because their payoff is the demand driver's single-flight de-duplication of concurrent fetches for the same package: landing the same tunings on the legacy two-phase resolver overcommits its non-deduplicated per-edge concurrency at the npmjs front door and regresses the resolve phase. The bench-vs-`utoo-next` comparison on this PR is expected to sit at noise — the active runtime path of `utoo install` is byte-identical to `next` here, because nothing in this PR is reachable from `build_deps_with_config`'s body or from any other live entry, the HTTP client stays at its single shared instance, and `Context.concurrency` keeps reading the existing tarball-side knob. File by file: * `service/manifest_provider.rs` (new, +106 lines): the trait definition. One async method that takes a `ManifestJob` and returns a `ManifestJobDone` (the typed job-and-result shape; see `traits/registry.rs` below). Carries `Send + Sync` under a `#[cfg_attr(not(target_arch = "wasm32"), async_trait)]` and the `?Send` form for wasm — the native build can `tokio::spawn` the job futures across the multi-threaded runtime (which is where the demand driver's perf comes from), and the single-threaded wasm runtime still works via `spawn_local`. * `service/registry/` (rename + new sibling): the existing flat `service/registry.rs` becomes `service/registry/mod.rs` so a new sibling file `service/registry/provider.rs` (+179 lines) can hold the `impl ManifestProvider for UnifiedRegistry` without bloating `mod.rs` and without widening the visibility of `UnifiedRegistry`'s private fields (`store`, `registry_url`, the supports-semver flag) — child modules already see private items of their parent module. The `UnifiedRegistry` struct body itself is unchanged. * `traits/registry.rs` (+67 lines): the trait's job and error shapes — `RegistryError`, `ManifestJob` (the `Versions`, `Full`, `ExactVersion` job kinds the driver issues), the paired `ManifestJobDone` and the `ManifestFullData` payload the full-manifest kind returns, and the `MetadataFormat` enum for the response-content-type negotiation (`application/vnd.npm.install-v1+json` vs the full form). Pulled out as named items so the trait's signatures don't leak any of the existing `service::fetch` module's internal types. * `service/manifest.rs` (+183 lines): two new helpers the adapter uses to keep `simd_json::serde::from_slice`'s in-place buffer mutation off the tokio runtime. The existing `parse_json_off_runtime` (a borrowing form that copies the buffer inside the worker) gets a buffer-consuming sibling `parse_json_vec_off_runtime(Vec<u8>)` whose callers can hand ownership of the response-body bytes straight in. The full-manifest parse picks up a sibling `parse_full_manifest_with_core_off_runtime(bytes, spec)` that returns both the parsed `FullManifest` and, when a spec was supplied that names an exact version, the `CoreVersionManifest` slice for that version — so the adapter's `ManifestJob::ExactVersion` path can hand the per-version result back to the driver without a second pass over the full document. Both helpers dispatch the CPU-bound parse to `rayon::spawn` on native and inline it on wasm via a `#[cfg(target_arch)]` switch. * `service/cache.rs` (+43 lines): two methods on `ProjectCacheData` that bridge the on-disk shape (a per-package map of specs and resolved-version manifests, the format the host serializes to the lockfile sidecar) and the resolver-owned `(name, spec, manifest)` tuples the demand loop emits. `resolved_manifests(&self)` flattens the on-disk map into the neutral tuple form for seeding a warm resolver run; `from_resolved(tuples)` rebuilds the on-disk shape from the tuples the resolver returned. The impl block carries `#[allow(dead_code)]` until the cutover PR points `api::build_lockfile` at them — same dead-code-staging pattern as the earlier #3079 (state) and #3083 (select) splits. * `service/mod.rs` (+4 lines): public re-exports of `ManifestProvider` and the supporting job types at the `crate::service::*` level so neither the demand module nor the pm binary has to reach into the sub-module path. * `model/manifest.rs` (+5 lines) and `model/mod.rs` (+6 lines): the small additions on the model side that the new parse helpers consume — a flat-list shape for the versions-only abbreviated-metadata response and the corresponding `pub use`. * `.github/workflows/pm-e2e-bench.yml` (+8 / -2): the bench-baseline build step (which overlays `origin/next`'s tracked files on top of the PR's tree with `git checkout origin/next -- .` so a `cargo build` against next's resolver runs against the PR's e2e harness) gets a cleanup of the paths the PR adds that don't exist in next: `git diff --no-renames --diff-filter=A --name-only origin/next HEAD -- crates/ | xargs -r rm -f`. Without it, this PR's new `service/registry/` directory ends up side-by-side with the overlay's flat `service/registry.rs`, and the build hits rustc's E0761 ("file for module `registry` found at both `mod.rs` and `registry.rs`"). The `--no-renames` flag is the load-bearing detail — under default rename detection git pairs the `registry.rs ↔ registry/mod.rs` rename as a single change, and the `--diff-filter=A` for the added side then reports zero added paths and misses the directory. The benchmark label on the PR is on so the bench gate runs. The two scaffolding tunings the perf model needs — the HTTP-client pool and the resolver-side concurrency cap of 256 for non-semver registries — live in the cutover PR alongside the entry-point bound flip, because the demand driver's single-flight is what makes the higher cap a win rather than a wall-clock regression vs `next`. The bench numbers on this PR are expected to sit at the `next` baseline within noise. Part 1/3 of the #3084 split. The remaining two are the demand driver (Part 2) and the entry-point cutover + the runtime tunings + the dead-code annotations coming off (Part 3, the bench-gated one). Refs #3028, #3084. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added a commit
that referenced
this pull request
May 28, 2026
Lands the demand-driven BFS resolver loop on top of the `ManifestProvider` trait from the preceding PR in the stack. The driver and its graph-building helpers exist as dead code in this PR — the entry-point switch that points `api.rs` and `builder`'s public `build_deps_*` / `resolve_*` chain at them is the third PR. Same dead-code-staging idiom as the earlier `state.rs` (#3079) and `select.rs` (#3083) splits. What lands here, file by file: * `resolver/demand/driver.rs` (new, ~700 lines): the `run_main_loop_bfs` entry — owns the per-run `ManifestState` (the cache + waiters + failures store from #3079) and the `FetchQueues` scheduler (the push/pop/complete state machine from #3080), pumps the `ManifestProvider` job stream through a `FuturesUnordered` of `tokio::task::JoinHandle`s (the multi-core spawn that gives the resolver native fan-out — `tokio::spawn` on native targets, the single-threaded `tokio::task::spawn_local` on wasm via the `#[cfg_attr]` toggle on the trait's `Send + Sync` bound). The `apply_fetch_result` glue feeds resolved manifests back into the graph through the new helpers in `builder.rs` (see below); the `select_edge` decision step from #3083 picks the next action per-edge (cache hit, version-cache hit, wait on an in-flight job, fail). The `handle_processed` wrapper around the graph-mutation step emits the existing `BuildEvent::Resolved` / `Failed` so progress receivers don't see a discontinuity once the cutover lands. A `#[cfg(test)]` module at the bottom holds the driver's unit-test scaffolding (`MockRegistryClient`, `CountingRegistry` wrapper for the single-flight property, the `create_*_manifest` helpers). One of those tests — `test_non_semver_exact_version_extract_single_flight` — is `#[ignore]`d in this PR with a reason string: it asserts on the `ManifestProvider` job count produced by a full `resolve(pkg, registry)` pipeline, which still routes through the legacy `RegistryClient::fetch_version_manifest` path in this PR. The cutover PR removes the `#[ignore]` once `resolve` is pointed at the demand driver. The other driver tests cover the loop's invariants in isolation (state transitions, waiter wake-up, schedule fairness) and pass under PR-B. * `resolver/demand/mod.rs`, `resolver/demand/queue.rs`: the small re-export and visibility adjustments to expose `run_main_loop_bfs` and `ResolverManifestCache` at the `crate::resolver::demand` level so `builder.rs` can name them, and the queue's `FetchKey` /`FetchDone` types in the shape the driver consumes. * `resolver/demand/state.rs`: a single attribute — `#[allow(dead_code)]` on the `ResolverManifestCache.entries` field. The driver writes the field via `ManifestState::into_resolver_cache()` at the end of each run; the reader is `ProjectCacheData::from_resolved` in the cutover PR's `api.rs` edit. Mirrors the symmetric annotation on the `ProjectCacheData` bridges in `service/cache.rs` from PR-A — both annotations come off when the entry-point switch wires the writer-chain to the reader-chain in PR-C. * `resolver/builder.rs`: four new graph-building helpers extracted from `process_dependency`'s internal logic so the driver can reuse them without going back through the legacy entry-points, plus the new `pub(crate) async fn build_deps_with_config_output` that wraps the demand loop with the existing tracing + receiver wiring and returns the `ResolverManifestCache` the host needs to persist: - `pub(crate) fn try_reuse_dependency(...)`: hits the graph's existing-node index before issuing a fetch, so repeat references to the same `(name, resolved-version)` share one node. - `pub fn process_dependency_with_resolved(...)`: the edge-resolution tail that runs once a manifest is in hand — creates or reuses the dependent node, attaches the edge, forwards the resolution mode flags. - `pub(crate) fn chain_err(...)`: lifts a `RegistryError` from the provider's job stream into the resolver's `ResolveError::WithChain` so the CLI's chain-aware error renderer still gets the parent → child causality string when the demand path fails the same way the legacy path used to. - `pub(crate) async fn handle_resolved_registry_manifest(...)`: the integration point between a resolved `CoreVersionManifest` and the graph — caches under both the spec and the resolved version (so later lookups by either key hit memory), spawns the dependent-edge collection, fires `BuildEvent::Resolved`. All four are reachable only from the driver in this PR; the legacy `process_dependency` keeps its inline form and the legacy entry chain (`build_deps` / `build_deps_with_*` / `resolve` / `resolve_with_options`) keeps its old `R: RegistryClient` signatures. The new `build_deps_with_config_output` is the demand-side entry the cutover PR will route `build_deps_with_config` and `api.rs` through; it carries an `#[allow(dead_code)]` for this interim state with a one-line comment naming the next PR as its caller. The three import-line tweaks at the top of `builder.rs` — `CoreVersionManifest` joining the `crate::model::manifest` brace-group, the new `use` of `ResolverManifestCache` and `run_main_loop_bfs` from `crate::resolver::demand`, and `ManifestProvider` joining the `crate::service` brace-group — are the only edits to existing lines in this file. The orphaned preload-era functions (`gather_preload_deps`, `run_preload_phase`, `run_bfs_phase`) keep their existing signatures and live call paths — the cutover PR is what `#[allow(dead_code)]`-annotates them and the cleanup PR after the cutover deletes them. The benchmark label is on this PR so the bench gate runs. Because the active resolver pipeline is unchanged in this PR (`resolve` still calls preload-then-BFS through the legacy `RegistryClient` interface), the expected bench numbers match PR-A on the standard npmjs workspace. The full `p1_resolve ≈ 2.4s / vCtx ≈ 18K` win shows up in PR-C alongside the entry-point flip. Part 2/3 of the #3084 split. Refs #3028 #3083 #3084 #3085 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added a commit
that referenced
this pull request
May 28, 2026
* perf(pm): demand resolver edge-selection module (select) Third leaf module of the demand resolver, after `state` (#3079) and `queue` (#3080). `select` is the pure per-edge resolution decision: fn select_edge(state, edge, name, spec, mode) -> EdgeStep // EdgeStep = Resolve | Skip | Fail | Park { wait, fetch } It only reads `ManifestState` and returns a decision value — no `&mut`, no async, no I/O, no graph mutation — so it's unit-testable in isolation (7 tests covering semver/full-manifest cache hits, recorded failures, parks, client-side version resolution, and optional-skip). Dead-code-staged (`#![allow(dead_code)]`); the driver that consumes it lands in the follow-up PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(pm): unify the failure/cache check in select via settled_step Address review: drop the one-line `resolve_version_from_full_manifest` wrapper (inline `resolve_version_from_versions`), and collapse the duplicated "recorded failure? -> cached manifest?" probe — which appeared in the semver path, the full-manifest early check, and the resolved-version check — into a single `settled_step(state, name, lookup_key, spec)`. The alias is now derived (`lookup_key != spec`) instead of hand-set per call. No behaviour change: the 7 select unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(pm): model a package's version source as one enum (PackageVersions) Address review: deciding an edge had to probe three separate maps (`full.cache`, `full.failures`, `versions_cache`) to learn a package's status. A package has at most one of those, so fold them into one enum-keyed map: enum PackageVersions { Failed(String), Full(Arc<FullManifest>), List(Arc<VersionsInfo>) } state.packages: HashMap<String, PackageVersions> `select_full_manifest` becomes a single `state.package(name)` lookup + a `match`, and the "at most one source" invariant is now enforced by the type. `full.waiters` becomes `package_waiters`. Also fixes a latent precedence bug: a cached version manifest now resolves the edge even if the package's full-manifest fetch later failed (the old order let the package failure shadow a usable cached manifest). New test pins it. 178 tests pass (8 select unit tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Part of the resolver demand-loop rework (split from the old combined #3079). The per-run manifest store, an independent module.
crates/ruborist/src/resolver/demand/state.rs:ManifestSlot<K,V>—{ cache, waiters, failures }per manifest kind (full keyed by name, version by(name, spec)), withis_settled/wake.ManifestState— the slots +versions_cache, with get/set accessors (version_manifest/version_failure/is_version_settled/cache_version/fail_version/wake_version),seeded(...), and neutralResolverManifestCacheoutput.Pure storage: no fetch scheduling (that is #3080,
queue) and no orchestration (that is the driver, #3078). Format-agnostic — speaks neutral(name, spec, manifest)tuples; theProjectCacheData↔ tuples adapter lives onProjectCacheDataitself (in #3078), so the store never knows the on-disk format.Bottom of the stack: #3080 (scheduler) and #3078 (driver) build on this.
🤖 Generated with Claude Code