Skip to content

perf(pm): full PR #2818 perf bundle on top of worker-pool - #2870

Closed
elrrrrrrr wants to merge 2 commits into
nextfrom
perf/pm-perf-bundle-v2
Closed

elrrrrrrr wants to merge 2 commits into
nextfrom
perf/pm-perf-bundle-v2

Conversation

@elrrrrrrr

Copy link
Copy Markdown
Contributor

Summary

Stack on top of #2869 (worker-pool). Bundles all remaining perf modules from PR #2818 (perf/extract-sequential-writes) that haven't merged via #2856/#2862/#2866/#2869.

Built by net-diff approach: take PR #2818's final-state file content, drop reverted experiments and bench infra, resolve overlap with #2866 ManifestStore. 21 files changed, +761/−341.

What's in this bundle (vs #2869 alone)

PM-side:

  • extractor.rs — rayon::spawn per package + par_chunks(32) intra-package writes
  • cloner.rs — cap blocking pool at worker_threads
  • logger.rs — drop indicatif Mutex from preload hot path (avg_conc 56 → 95 at cap=128)
  • user_config.rs / main.rs — cap tunings exposed via CLI/env
  • install.rs — phase timings

Ruborist-side:

  • resolver/semver.rs — normalize_spec returns Cow, no alloc on common path
  • resolver/workspace.rs — parallelise workspace package.json reads (~150ms saved)
  • resolver/preload.rs — refinements on top of worker-pool (Notify+enable parking, AcqRel pairings)
  • service/dns.rs — per-family DNS rotation
  • service/http.rs — TLS provider configuration + diagnostic instrumentation
  • traits/registry.rs — MaybeSend/MaybeSync inline (was separate module)

What's NOT included

Hypothesis to validate via CI bench

PR #2818's final p1_resolve target was 2.65s (mac+linux). Worker-pool alone (#2869) hit 3.13s on both. Difference ≈ 480ms attributable to:

  • alloc cleanup (Cow normalize_spec, drop indicatif, log filter)
  • workspace parallel reads (~150ms in setup phase)
  • DNS rotation + TLS tuning

Expected p1_resolve: ~2.6-2.8s if #2818's stack reproduces.

Test plan

  • cargo build -p utoo-ruborist -p utoo-pm clean
  • cargo clippy --all-targets -- -D warnings --no-deps clean
  • cargo test -p utoo-ruborist 162 passed
  • cargo test -p utoo-pm 245 passed
  • CI pm-e2e-bench — the real signal
  • CI utooweb-ci-build-wasm (validates MaybeSend on wasm32)

🤖 Generated with Claude Code

elrrrrrrr and others added 2 commits April 29, 2026 23:25
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>
Stack of perf optimizations from PR #2818 (perf/extract-sequential-writes)
that are not yet on next, applied via final-state diff (not commit-by-commit
cherry-pick) to skip experiments that were reverted in #2818's history and
optimizations that are already implicit in #2866's ManifestStore refactor.

Modules integrated on top of worker-pool (#2869 / 6b5304f):

PM-side:
- extractor.rs: rayon::spawn per package + par_chunks(32) intra-package writes
- cloner.rs: cap blocking pool at worker_threads
- downloader.rs: instrumentation
- logger.rs: drop indicatif inc()/inc_length() and per-package messages
  from preload hot path (avg_conc 56 → 95 at cap=128)
- user_config.rs: cap tunings exposed via CLI/env
- install.rs: phase timings
- helper/lock.rs: serializer micro-fixes
- main.rs: instrumentation flag plumbing

Ruborist-side:
- resolver/semver.rs: normalize_spec returns Cow, no alloc on common path
- resolver/workspace.rs: parallelise workspace package.json reads (200x
  parallel instead of serial; saved ~150ms in setup phase)
- resolver/preload.rs: refinements on top of worker-pool (Notify+enable
  parking pattern, AcqRel ordering pairings, inline parse paths)
- resolver/builder.rs: thread MaybeSend/MaybeSync bounds through 5 entry
  points; supersedes worker-pool's bounds with PR #2818's final form
- traits/registry.rs: MaybeSend/MaybeSync moved inline (was in separate
  module); fetch_full_manifest returns Arc<FullManifest> from #2866 +
  carries + MaybeSend
- service/dns.rs: per-family DNS rotation
- service/http.rs: TLS provider configuration, diagnostic instrumentation
- service/manifest.rs: minor refactors

What's NOT included (intentionally):
- crates/manifest-bench: standalone diagnostic tool, not perf-critical
- bench infra (workflows + e2e/pm-bench-*.sh): already merged via #2824
- bb256ec "drop versions.keys.clone": already implicit in #2866 refactor

Test plan:
- cargo build -p utoo-ruborist -p utoo-pm: clean
- cargo clippy --all-targets -- -D warnings --no-deps: clean
- cargo test -p utoo-ruborist: 162 passed
- cargo test -p utoo-pm: 245 passed
- CI pm-e2e-bench: pending

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@elrrrrrrr elrrrrrrr added the benchmark Run pm-bench on PR label Apr 30, 2026

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

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.

Code Review

This pull request introduces significant performance optimizations to the package manager, notably by replacing the FuturesUnordered preloading mechanism with a worker pool and implementing round-robin DNS rotation to improve connection distribution. Other improvements include batching file writes during extraction, moving JSON parsing inline to async workers, and adding comprehensive diagnostic tracing for HTTP and parsing operations. Review feedback highlights that the new cap on blocking threads may be overly restrictive on machines with few cores, identifies a potential hang in the worker pool if the result channel is closed, and points out redundant diagnostic timestamps resulting from the move to inline parsing.

Comment thread crates/pm/src/main.rs
// pool warm across runs. Observed locally: default pool gave
// bimodal walls (2.7s fast / 6.9s thrash on M2); capping at
// 32 eliminates the thrash peak.
.max_blocking_threads(worker_threads)

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

Capping max_blocking_threads to worker_threads (which typically defaults to the number of CPU cores) may be too restrictive. While the goal is to reduce thread-creation thrash, a very low cap (e.g., 2 or 4 on low-core machines) can lead to starvation for blocking operations like those used in tokio::fs. The comment mentions that "~32 threads is enough" and that "capping at 32 eliminates the thrash peak," which suggests that a fixed minimum or a higher cap was intended. Given that JSON parsing has been moved "inline" to async workers, the primary source of blocking pool pressure has already been mitigated.

Suggested change
.max_blocking_threads(worker_threads)
.max_blocking_threads(worker_threads.max(32))

Comment on lines +198 to +201
if result_tx.send((name, result, elapsed_ms)).is_err() {
// Main task dropped the receiver — done collecting.
break;
}

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

If result_tx.send fails (which happens if the main task drops the receiver, e.g., due to cancellation), the worker exits the loop without incrementing the completed counter. This can prevent other workers from ever reaching the termination condition (done == dispatched), causing them to hang indefinitely while parked on notified.await. Since ruborist may be used as a library, this could lead to a background task leak. Consider checking result_tx.is_closed() in the parking logic or using a CancellationToken to ensure all workers can exit when the main task is gone.

Comment on lines +113 to +114
let queued_at = traced.then(std::time::Instant::now);
let exec_start = queued_at.map(|_| std::time::Instant::now());

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 diagnostic timing for queued_at and exec_start is captured sequentially on the same thread. Since parsing is now performed "inline" rather than being dispatched to a blocking pool, these two timestamps will always be nearly identical. This makes the "queue wait" measurement (calculated as exec_start - queued_at in log_parse_diagnostics) meaningless. You should simplify this to a single start timestamp to measure the parse duration.

Suggested change
let queued_at = traced.then(std::time::Instant::now);
let exec_start = queued_at.map(|_| std::time::Instant::now());
let traced = parse_trace_enabled();
let parse_start = traced.then(std::time::Instant::now);
let mut parse_buf = raw_bytes.clone();
let mut manifest: FullManifest =
simd_json::serde::from_slice(&mut parse_buf)
.map_err(|e| FetchError::Permanent(anyhow!("JSON parse error: {e}")))?;
manifest.raw = std::sync::Arc::from(raw_bytes);
if let Some(s) = parse_start {
record_parse_interval(s, s, std::time::Instant::now());
}

elrrrrrrr added a commit that referenced this pull request Apr 30, 2026
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>
@elrrrrrrr elrrrrrrr added benchmark Run pm-bench on PR and removed benchmark Run pm-bench on PR labels Apr 30, 2026
@github-actions

Copy link
Copy Markdown

📊 pm-bench-phases · 7c7e5e7 · 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 11.44s 1.54s 7.74s 7.47s 741M 314.5K
utoo-npm 11.91s 0.86s 8.72s 9.82s 1.27G 160.6K
utoo 13.56s 4.56s 8.92s 9.06s 1.43G 155.0K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 19.6K 16.4K 1.16G 6M 1.83G 1.72G 1M
utoo-npm 171.4K 124.3K 1.14G 4M 1.68G 1.68G 2M
utoo 121.1K 40.3K 1.14G 6M 1.68G 1.68G 2M

p1_resolve

PM wall ±σ user sys RSS pgMinor
bun 1.78s 0.07s 3.06s 0.88s 529M 175.4K
utoo-npm 4.24s 0.07s 3.89s 1.40s 432M 71.0K
utoo 2.03s 0.07s 4.54s 1.25s 579M 95.5K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 10.8K 3.7K 202M 3M 104M - 1M
utoo-npm 65.4K 2.6K 203M 2M 9M 5M 2M
utoo 21.6K 15.3K 199M 3M 7M 3M 2M

p3_cold_install

PM wall ±σ user sys RSS pgMinor
bun 10.46s 3.03s 4.74s 7.08s 674M 216.1K
utoo-npm 10.55s 3.57s 4.43s 8.72s 824M 116.9K
utoo 7.55s 1.74s 4.18s 7.74s 872M 97.4K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 8.2K 5.0K 993M 4M 1.73G 1.73G 1M
utoo-npm 146.8K 70.6K 965M 4M 1.67G 1.67G 2M
utoo 84.7K 25.4K 964M 3M 1.67G 1.67G 2M

p4_warm_link

PM wall ±σ user sys RSS pgMinor
bun 4.90s 0.99s 0.15s 1.93s 136M 31.2K
utoo-npm 3.33s 0.95s 0.45s 2.92s 82M 18.4K
utoo 3.04s 0.53s 0.34s 2.57s 59M 13.1K
PM vCtx iCtx netRX netTX cache node_mod lock
bun 218 26 27K 27K 1.84G 1.73G 1M
utoo-npm 47.2K 19.1K 25K 37K 1.67G 1.67G 2M
utoo 15.6K 7.6K 19K 15K 1.68G 1.67G 2M

npmmirror.com: no output captured.

@elrrrrrrr

Copy link
Copy Markdown
Contributor Author

Superseded by #2874 (rayon-only path). #2874 absorbs the network-layer subset (h1+pool+dns) of #2818's perf bundle without the worker-pool refactor + MaybeSend/MaybeSync shim. Closing.

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

Labels

benchmark Run pm-bench on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant