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>
19b95c0 to
ea0b013
Compare
There was a problem hiding this comment.
Code Review
This pull request implements several performance optimizations, including chunked parallel writes for extraction, a multi-thread worker pool for manifest preloading, and reduced allocations in semver normalization. It also parallelizes workspace package discovery and adjusts logging levels to reduce tracing overhead. A review comment identified an inefficient count calculation in the preloading logic that should be replaced with a direct call to len() to avoid unnecessary allocations and cloning.
| stats.total_processed = { | ||
| let mut set: HashSet<String> = HashSet::with_capacity(processed.len()); | ||
| for entry in processed.iter() { | ||
| set.insert(entry.key().clone()); | ||
| } | ||
| set.len() | ||
| }; |
There was a problem hiding this comment.
This block of code to calculate the total number of processed items is inefficient. It allocates a new HashSet, iterates over the entire DashSet, clones every key, and re-inserts them just to get the count.
The DashSet::len() method provides an exact count and is much more efficient as it avoids allocations and cloning. While it's an O(N) operation, it's significantly faster than the current implementation.
Given that this pull request is focused on performance improvements, this change introduces a regression.
stats.total_processed = processed.len();
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 8.87s | 0.26s | 10.02s | 9.81s | 737M | 326.9K |
| utoo-next | 9.46s | 0.04s | 11.45s | 13.01s | 1.34G | 176.9K |
| utoo-npm | 9.89s | 0.31s | 11.41s | 12.92s | 1.23G | 158.1K |
| utoo | 8.97s | 1.57s | 11.25s | 12.12s | 1.26G | 150.8K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 14.3K | 16.8K | 1.16G | 6M | 1.83G | 1.72G | 1M |
| utoo-next | 166.9K | 138.5K | 1.13G | 4M | 1.68G | 1.68G | 2M |
| utoo-npm | 168.6K | 138.2K | 1.14G | 4M | 1.68G | 1.68G | 2M |
| utoo | 119.0K | 61.7K | 1.13G | 3M | 1.68G | 1.68G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 1.89s | 0.01s | 3.88s | 0.97s | 486M | 159.8K |
| utoo-next | 4.96s | 0.06s | 6.11s | 1.41s | 441M | 70.4K |
| utoo-npm | 5.10s | 0.10s | 5.84s | 1.08s | 432M | 68.7K |
| utoo | 2.79s | 0.04s | 5.68s | 1.56s | 565M | 76.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 7.8K | 4.3K | 200M | 3M | 104M | - | 1M |
| utoo-next | 94.5K | 8.3K | 197M | 2M | 7M | 3M | 2M |
| utoo-npm | 64.0K | 2.9K | 202M | 2M | 9M | 5M | 2M |
| utoo | 37.5K | 18.6K | 196M | 987K | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 7.76s | 1.58s | 6.10s | 9.82s | 645M | 209.6K |
| utoo-next | 7.44s | 1.66s | 5.32s | 11.22s | 913M | 120.6K |
| utoo-npm | 7.89s | 2.57s | 5.56s | 11.43s | 935M | 108.8K |
| utoo | 5.81s | 0.34s | 5.22s | 10.57s | 885M | 105.3K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 5.5K | 5.9K | 993M | 3M | 1.73G | 1.73G | 1M |
| utoo-next | 109.8K | 72.0K | 963M | 3M | 1.67G | 1.67G | 2M |
| utoo-npm | 117.7K | 75.1K | 963M | 3M | 1.67G | 1.67G | 2M |
| utoo | 89.6K | 50.0K | 962M | 2M | 1.67G | 1.67G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.29s | 0.07s | 0.21s | 2.33s | 135M | 32.2K |
| utoo-next | 2.34s | 0.05s | 0.60s | 3.92s | 85M | 19.2K |
| utoo-npm | 2.31s | 0.11s | 0.61s | 3.92s | 85M | 19.9K |
| utoo | 2.41s | 0.27s | 0.60s | 3.92s | 85M | 19.3K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 301 | 24 | 7M | 18K | 1.88G | 1.72G | 1M |
| utoo-next | 48.6K | 22.0K | 439K | 13K | 1.67G | 1.67G | 2M |
| utoo-npm | 49.8K | 21.8K | 444K | 14K | 1.67G | 1.67G | 2M |
| utoo | 49.0K | 20.2K | 443K | 35K | 1.68G | 1.67G | 2M |
npmmirror.com: no output captured.
Summary
Bisect bundle from #2870. Stack on top of #2869 (worker-pool). Adds only the non-overlapping external optimizations from PR #2818, keeping worker-pool's resolver internals unchanged.
Bisect rationale
#2870 regressed wall +17% and σ blew up 4.7×. Suspect modules:
service/dns.rs— per-family DNS rotationservice/http.rs— TLS instrumentation overheadresolver/preload.rspost-7e64c597evolution — was tuned for perf(pm): sequential writes within tarball extraction #2818's full stack, not optimal in isolationThis PR keeps all of those at worker-pool's clean form. Adds only:
pm/util/extractor.rs— rayon::spawn per package + par_chunks(32) intra-package writes (split feat: use rust fmt #2)pm/util/logger.rs— drop indicatif Mutex from preload hot pathruborist/resolver/semver.rs—normalize_specreturnsCowruborist/resolver/workspace.rs— parallelise workspace package.json reads (~150ms saved)Hypothesis
If alloc cleanup + workspace parallel alone improve over #2869's 3.13s, the regression in #2870 is attributable to the deferred service/* + resolver/preload.rs evolution. If this PR also doesn't improve, the alloc cleanup itself is value-neutral on this Linux baseline (Cloudflare throttle dominates).
Test plan
🤖 Generated with Claude Code