Repository navigation
Conversation
N long-lived `tokio::spawn` workers pull work from a shared lock-free `SegQueue`. Each worker is independent on tokio's multi-thread runtime, so when one worker is parsing manifest JSON (CPU-bound, simd_json), other workers continue driving their network IO and other parses run on different cores. Replaces `FuturesUnordered` where the main task owned all preload futures and polled them cooperatively — every per- future await continuation (including parse) ran serialised in that single task. The architectural fix alone (proven nop in PR #2830 with spawn_local) needs the network-layer drivers (aws-lc-rs + OnceMap, earlier in this stack) to actually express its benefit. With them: TLS handshake CPU is fast (aws-lc-rs), duplicate fetches are deduped (OnceMap), so worker-pool's N independent task slots are doing actual unique work. `tokio::spawn` requires `Future: Send + 'static`, which forces `RegistryClient` trait surface to grow `+ Send` on its `impl Future` returns + `Self: Sync` / `Self::Error: Send` on default impls. Native uses real Send/Sync; wasm32 uses no-op marker traits because `JsFuture` is `!Send` and tasks run via `wasm_bindgen_futures::spawn_local`. `PreloadRegistry` trait alias bundles `RegistryClient + Clone + MaybeSend + MaybeSync + 'static` so the 5 resolver entry points (`build_deps*`, `resolve*`, `run_preload_phase`) read clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bisect from #2870 (full bundle, regressed +17% wall on Linux p1_resolve). Keeps worker-pool's resolver/builder/registry/traits/preload.rs unchanged from #2869. Adds only the non-overlapping external optimizations from PR #2818: - pm/util/extractor.rs: rayon::spawn per package + par_chunks(32) - pm/util/logger.rs: drop indicatif Mutex from preload hot path - ruborist/resolver/semver.rs: normalize_spec returns Cow - ruborist/resolver/workspace.rs: parallelise workspace package.json reads Skips (deferred — were in #2870, suspect regression source): - service/dns.rs (per-family DNS rotation) - service/http.rs (TLS instrumentation overhead) - service/manifest.rs - resolver/preload.rs (post-7e64c597 evolution; tuned for #2818's full stack, not optimal in isolation per #2870 data) - resolver/builder.rs / registry.rs / traits/registry.rs Test plan: - cargo build / clippy / cargo test -p utoo-ruborist (162 passed) - 1 pre-existing flaky test (test_update_package_binary_fsevents) fails only when interleaved; passes in isolation. Unrelated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stack on top of #2871 (alloc cleanup bisect). Adds the resolver-side worker-pool refinements from PR #2818 that go beyond the original 7e64c59 cherry-pick (#2869): - crates/ruborist/src/resolver/preload.rs (PR #2818 final form) - crates/ruborist/src/resolver/builder.rs (PreloadRegistry trait alias) - crates/ruborist/src/resolver/registry.rs (MaybeSync bound on resolve_package) - crates/ruborist/src/traits/registry.rs (MaybeSend/MaybeSync inline) - crates/ruborist/src/lib.rs (drop separate maybe_send mod) Diagnostic instrumentation (start_http_trace / start_parse_trace + log_http_diagnostics / log_parse_diagnostics in service/http.rs and preload.rs) deliberately stripped — these are dev-only tracing that PR #2818 used during the multi-week investigation, not perf-critical. Also keeps service/http.rs at next's form (no TLS instrumentation overhead, which was suspect in #2870 regression). Skips (intentional, separate iteration if needed): - service/dns.rs (per-family DNS rotation) - service/http.rs (TLS provider config + instrumentation) - service/manifest.rs Hypothesis: preload.rs evolution adds Notify+enable parking, ordered AcqRel pairings, and refined termination on top of 7e64c59's basic worker-pool. May reduce σ further than #2869's clean form. Test plan: - cargo build / clippy / 162+245 tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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 several performance optimizations, including chunked parallel writes for tarball extraction, a worker-pool architecture for manifest preloading, and parallelized workspace discovery. It also reduces overhead by optimizing string allocations in semver normalization and minimizing mutex contention during progress updates. Feedback highlights a critical race condition in the preloading termination logic where a non-linearizable check could cause premature shutdown.
Comment on lines
+203
to
+207
| if done == dispatched.load(Ordering::Acquire) && pending.is_empty() { | ||
| shutdown.store(true, Ordering::Release); | ||
| notify.notify_waiters(); | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
This termination check in the hot path introduces a race condition that could lead to premature shutdown and incomplete dependency resolution.
The crossbeam_queue::SegQueue::is_empty() method is not linearizable and can return a stale true value even if another thread has just pushed new work into the queue. If a worker sees done == dispatched and a stale is_empty() == true, it will initiate a shutdown by setting the shutdown flag. This would cause other workers to exit and the main loop to terminate, leaving unprocessed items in the work queue.
The termination logic in the idle path (when pending.pop() returns None) is safer and sufficient. I recommend removing this check from the hot path to eliminate the race condition.
elrrrrrrr
added
benchmark
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 12.89s | 0.42s | 7.77s | 7.72s | 772M | 342.5K |
| utoo-next | 15.40s | 0.68s | 8.85s | 10.12s | 1.26G | 179.7K |
| utoo-npm | 15.23s | 0.97s | 8.75s | 10.18s | 1.20G | 167.5K |
| utoo | 22.06s | 7.43s | 8.69s | 9.66s | 1.28G | 158.3K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 18.6K | 16.1K | 1.17G | 6M | 1.81G | 1.70G | 1M |
| utoo-next | 202.2K | 119.9K | 1.13G | 4M | 1.68G | 1.68G | 2M |
| utoo-npm | 186.0K | 129.5K | 1.14G | 5M | 1.68G | 1.68G | 2M |
| utoo | 183.9K | 61.3K | 1.13G | 4M | 1.68G | 1.68G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 1.76s | 0.12s | 3.08s | 0.85s | 495M | 163.6K |
| utoo-next | 4.17s | 0.17s | 3.98s | 1.63s | 442M | 76.5K |
| utoo-npm | 4.64s | 0.53s | 3.92s | 1.35s | 431M | 66.5K |
| utoo | 3.11s | 0.40s | 4.26s | 1.31s | 544M | 74.7K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 10.5K | 3.8K | 202M | 3M | 104M | - | 1M |
| utoo-next | 98.4K | 8.3K | 197M | 2M | 7M | 3M | 2M |
| utoo-npm | 65.1K | 2.8K | 208M | 2M | 9M | 5M | 2M |
| utoo | 47.5K | 16.2K | 197M | 1M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 15.51s | 9.25s | 4.61s | 7.16s | 692M | 225.3K |
| utoo-next | 13.80s | 5.56s | 4.51s | 8.38s | 820M | 116.4K |
| utoo-npm | 14.02s | 4.25s | 4.51s | 8.61s | 850M | 114.3K |
| utoo | 8.74s | 1.26s | 4.27s | 8.24s | 779M | 107.0K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 9.7K | 5.3K | 993M | 4M | 1.73G | 1.73G | 1M |
| utoo-next | 146.8K | 71.4K | 965M | 4M | 1.67G | 1.67G | 2M |
| utoo-npm | 152.7K | 72.6K | 965M | 4M | 1.67G | 1.67G | 2M |
| utoo | 116.3K | 49.6K | 964M | 3M | 1.67G | 1.67G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.26s | 1.15s | 0.18s | 1.90s | 135M | 33.4K |
| utoo-next | 4.06s | 1.26s | 0.46s | 2.94s | 83M | 18.9K |
| utoo-npm | 4.88s | 0.96s | 0.44s | 2.96s | 82M | 18.9K |
| utoo | 4.24s | 1.19s | 0.42s | 2.94s | 82M | 18.7K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 341 | 31 | 7M | 23K | 1.88G | 1.70G | 1M |
| utoo-next | 48.3K | 18.8K | 47K | 32K | 1.67G | 1.67G | 2M |
| utoo-npm | 47.8K | 19.2K | 41K | 19K | 1.67G | 1.67G | 2M |
| utoo | 46.8K | 19.0K | 43K | 22K | 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
Bisect v4 — stack on top of #2871. Adds PR #2818's resolver-side refinements (preload.rs/builder.rs/registry.rs/traits/registry.rs final form) but excludes service/dns.rs, service/http.rs, service/manifest.rs which were suspect in #2870 regression.
Bisect ladder
Test plan
🤖 Generated with Claude Code