Repository navigation
Conversation
Replace intra-package `par_iter` with a sequential loop when writing extracted tar entries to disk. Each tar entry is typically small and writes complete in microseconds, so splitting them into rayon tasks was causing heavy work-stealing (futex park/unpark) and dominating context switches on large dep graphs. Cross-package parallelism is preserved by the outer `rayon::spawn` in `extract_tarball`. 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 modifies the tarball extraction logic in crates/pm/src/util/extractor.rs to process entries sequentially instead of in parallel. This change aims to reduce excessive context switching caused by rayon work-stealing on large dependency graphs, while maintaining cross-package parallelism. Feedback suggests consuming the entries collection during iteration to optimize memory usage by dropping file buffers immediately after they are written.
- Cold bench: drop `| tail -1` so hyperfine's full summary (mean, stddev, range) reaches the log. Failure detection now uses exit status instead of piping. - `BENCH_WARM_RUNS=0` skips the warm phase entirely (previously the warm function always ran and hyperfine would reject --runs 0). - Result aggregator tolerates empty or malformed export-json files (e.g. when a PM's cold install fails): the offending file is reported and skipped instead of crashing the whole summary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the sequential `for` loop over extracted tar entries with `par_chunks(WRITE_CHUNK_SIZE)` — each rayon task writes a contiguous run of 32 files sequentially. This retains multi-core IO overlap for large packages while cutting the rayon task count (and its work- stealing futex traffic) by the chunk factor versus a per-file par_iter. Cross-package parallelism is preserved by the outer rayon::spawn in extract_tarball. Local (macOS, antd-test, 3 runs avg): before par_iter: wall 17.2s sys 6.18s ivcsw 208k for-loop: wall 15.3s sys 2.36s ivcsw 61k par_chunks(32): wall 13.9s sys 5.77s ivcsw 191k chunks wins wall but loses the ctx-switch reduction relative to the pure sequential version; CI with a large dep graph (ant-design-x) is the authoritative measurement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Accumulate wall microseconds for download, extract, and clone across
all packages during install. Print a one-line summary alongside the
existing `added / reused / downloaded` counts, e.g.
+ 513 added · 3017 reused · 123 downloaded
download 135.8s · extract 2.3s · clone 0.4s · 19.0 MB fetched
The sums are non-exclusive across cores: dividing by wall clock
gives the effective concurrency for each phase, and the ratio
between phases shows where cold-install CPU time actually lands.
Overhead is three atomics per downloaded tarball.
Local antd-test (macOS, npmmirror, 77 packages, wall 16s): download
dominates 98% of the CPU budget, extract 1.6%, clone 0.3% — reshapes
where we should look for cold-install wins.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Needed so the per-phase timings line (`download · extract · clone · bytes`) printed at the end of each install reaches the CI log. Trade-off is noisier logs — registry INFO/WARN lines come through — but that's the price for visibility into where cold-install CPU actually lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Separates three independent measurements for utoo vs bun so each phase's improvement can be judged on its own baseline: Phase 1 · resolve utoo deps / bun install --lockfile-only Phase 3 · cold install utoo install / bun install (empty cache) Phase 4 · warm link utoo install / bun install (cache warm) Phase 3 uses the lockfile generated by phase 1, with cache reset between iterations. Phase 4 resets only node_modules so only the cache → node_modules link step is measured. Uses hyperfine --show-output so utoo's phase-timings line (\`download · extract · clone · bytes\`) reaches the CI log alongside the wall-clock summary. Triggered via workflow_dispatch with configurable project / registry / runs. Defaults to ant-design against npmjs.org, 3 runs per phase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…anch merge Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous inline bash -c prepare was silently no-op on CI: utoo's run 2/3 showed '3280 reused' meaning the cache wasn't actually cleared, and bun hit InvalidNPMLockfile because utoo's package-lock.json leaked across iterations. Now each phase writes a dedicated prepare shell script per-PM that: - always drops node_modules (incl. workspace package trees), - clears exactly the lockfiles that would confuse this PM, - wipes the right cache for this phase, - prints a '[prep]' line so the CI log proves prepare ran. Also factored out seed_for_phase so lockfile / cache warmup happens once before the benchmark, not leaking into the measurement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
bench-phases
…che wipe Path-based rm -rf of $HOME/.cache/nm wasn't actually emptying the cache on the CI runner — utoo runs 2/3 of phase 3 still showed '3280 reused', wall was 0.8-1.1s instead of the 10s cold-install baseline, hyperfine itself warned about caches not being filled until after run 1. Let each PM clean its own cache via its CLI so we don't rely on guessing where it stores things. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
bench-phases
`utoo clean` / `bun pm cache rm` didn't empty the cache on the CI runner either — so now use explicit bench-local paths the rm -rf prepare can guarantee to wipe: utoo: --cache-dir=/tmp/utoo-bench-cache on every invocation bun: BUN_INSTALL_CACHE_DIR=/tmp/bun-bench-cache (env var) Gets us deterministic cold/warm state between hyperfine iterations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
bench-phases
Drop into diagnostic mode to figure out why hyperfine's --prepare still leaves utoo's cache intact across iterations despite the explicit --cache-dir. Prints the generated prepare script, and logs each per-iteration invocation's before/after du -sh of both caches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
bench-phases
The case $phase in p1) p3) p4) \-style patterns never matched against actual phase strings like "p1_resolve" / "p3_cold_install" / "p4_warm_link". Result: write_prepare produced a script containing only the common header and no phase-specific cache-wipe logic, so every run after the first hit a warm cache and timings collapsed. Same off-by-name bug in seed_for_phase: "p3:utoo" pattern never matched "p3_cold_install:utoo", skipping lockfile seeding and warm-cache priming. Switched both to "p*_*" globs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
bench-phases
The cache-size before/after logs + generated-script dumps were diagnostic scaffolding used to trace the p* vs p*_resolve pattern mismatch. With that fixed, keep the plain hyperfine --prepare invocation so CI logs are readable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
bench-phases
…time Each hyperfine iteration now runs inside a metrics wrapper that greps /usr/bin/time -v output for RSS, voluntary/involuntary context switches, page faults, and IO read/write counts. Per-PM per-phase averages across the 3 runs are shown alongside the wall-clock table so we can see, e.g., whether utoo's resolve phase costs more syscalls than bun's, or whether its warm-link advantage comes at a memory cost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 30, 2026
`crates/manifest-bench` was the standalone HTTP-only fetch tool used in PR #2818's driver-hunt to verify the HTTP stack ceiling (manifest-bench ≈ 2.10s vs ruborist ≈ 2.15s on cap=128, proving ruborist matched the network ceiling). Now that worker-pool / OnceMap / aws-lc-rs are split into their own targeted PRs, the standalone bench tool no longer carries its weight as part of #2818 — it can ship as a separate "diagnostics" PR if useful, but doesn't belong on the perf branch's critical path. Also reverts our local-only edits to `bench/pm-bench.sh` (66-line drift that doesn't carry value separately) and the root `Cargo.toml` / `Cargo.lock` workspace-member entry that was only there to wire manifest-bench in. 402 lines (manifest-bench) + 84 lines (bench script + workspace deps) = ~486 lines removed from the diff vs next.
Two clean-ups requested as we narrow the PR's diff against `next`: 1. Drop all `record_*_interval` / `*_trace` plumbing left over from the driver-hunt phase. These were never meant to ship — they captured per-request HTTP `(send, end)` and per-parse `(queued, exec_start, exec_end)` timestamps for diagnosing the FuturesUnordered → worker-pool transition. The story is in PR #2818's commit log; the resulting code is now the authoritative answer, so the scaffolding can go. Removed: - `service/http.rs`: HTTP_TRACE / PARSE_TRACE statics + 7 fns (`start_http_trace` / `finish_http_trace` / `record_http_interval` / `start_parse_trace` / `finish_parse_trace` / `parse_trace_enabled` / `record_parse_interval`) - `service/manifest.rs`: 5 instrumentation call-sites in the two fetch loops and their `Instant::now` book-keeping - `resolver/preload.rs`: imports + `start_*_trace()` / `finish_*_trace()` pair + `log_http_diagnostics` (~73 lines) + `log_parse_diagnostics` (~58 lines) + the per-stats `tracing::debug!` summary line 2. Restore the indicatif `PreloadStart` / `Queued` / `Fetching` / `Progress` event-handling that PR #2818's `2b89d0b9 perf(pm): drop indicatif inc()/inc_length() from preload hot path` removed. That optimisation belongs in the alloc-cleanup driver-bundle PR alongside the other small Mutex/clone hot-path wins (~0.31 s on p1_resolve per the PR doc timeline). Bringing it back here lets us measure the worker-pool's contribution in isolation, and keeps the minimum-viable bundle's split clean. cargo clippy -p utoo-pm -p utoo-ruborist --all-targets -- -D warnings --no-deps clean. Ruborist / pm tests untouched and still pass.
…visit) A/B-verified independent gain on top of worker-pool. Restores the PR #2818 originally-shipped behaviour after we briefly reverted it in `11107948` to measure the cost in isolation. Round-6 (no per-event indicatif update): p1_resolve 2.50s ±0.10 Round-7 (upstream indicatif restored): p1_resolve 2.73s ±0.07 Δ +0.23s (+9%) Both runs same workload (ant-design / npmjs / GH Actions ubuntu), all other code identical, σ 0.07-0.10 — clean independent signal. `PROGRESS_BAR.inc()` / `.inc_length()` each take indicatif's internal `Mutex<ProgressBarState>`. Worker-pool preload at concurrency=128 fires 4571 dispatches + 4571 completions = ~9000 lock acquisitions in a 3-4 s phase, contending with the steady_tick draw thread. The result is the main task's fill-and-drain loop gets serialised on this lock — standalone manifest-bench (no indicatif) sustains avg_conc=95 at cap=128; ruborist with indicatif updates was stuck at 56. The spinner still ticks via steady_tick so the user sees the phase is alive; the final `PreloadComplete` summary prints success/fail counts.
…/queued)
Halves indicatif lock-acquisition count by dropping per-event updates on
PreloadStart / PreloadQueued / PreloadFetching while keeping per-package
PreloadProgress ("resolved {name}") for user-visible feedback.
Round-6/7/8 A/B/A established the full cost: ~0.23s on p1_resolve when
both fetching + progress fire (4571 + 4571 = ~9000 lock-acquisitions on
the main task, contending with steady_tick draw thread).
This commit removes the 4571 fetching events but keeps the 4571 progress
events — same UX (still see per-package activity) at half the lock cost.
Predicted regression vs full-drop (round-6/8): 0.10-0.15s, measurable in
round-9 bench.
Phase-boundary events:
- PreloadStart fires once per phase
- PreloadQueued fires ~once per BFS layer (~10 times for ant-design)
- Both have low frequency but their `inc_length` calls were used to set
the bar's max. Dropping them means the bar runs counter-mode without a
percentage — acceptable for a spinner-style indicator at this scale.
Cleanest fix to the indicatif Mutex contention identified in PR #2818 round-6/7/8 A/B/A. Detect stderr's TTY status once at startup, cache in `IS_TTY: Lazy<bool>`, short-circuit the entire ProgressReceiver event path on non-TTY. Why a TTY gate works where dropping events doesn't: - indicatif's `inc()` / `set_message()` always acquire a write-lock on `Mutex<ProgressState>` and mutate `pos` / `est.buf` / `message`, even when the draw target is hidden or the underlying stream is not a TTY. Only the *rendering* short-circuits — state mutation always runs. Round-9 (drop fetching, keep progress) still showed +0.10- 0.15s vs full-drop, confirming the lock + ETA buffer push + format! alloc are the real costs, not the byte flush. - CI (GitHub Actions, piped output, `2>&1 | tee`) now gets zero indicatif touches: no `inc`, no `set_message`, no `format!()` allocation, no `String::clone` into `set_message`. Local dev (TTY) gets the full upstream UX with per-package progress. Restored the upstream PreloadStart / PreloadQueued / PreloadFetching / PreloadProgress event handlers (PROGRESS_BAR.inc_length / log_progress("fetching ...") / inc(1) + log_progress("resolved ...")). The `if !*IS_TTY { return; }` at the entry of `on_event` hoists the gate above the `match`, so per-event check overhead is also zero. Also gated `start_progress_bar` and `log_progress` defensively for external callers. Predicted bench (round-10 on CI Linux): p1_resolve back to round-6/8 range (2.50-2.58s) — same as if we had dropped all events, but local dev experience preserved.
3 of 6 tasks
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
… CI)
A/B-verified perf fix isolated as its own PR. Detect stderr's TTY
status once at startup (`IS_TTY: Lazy<bool>`) and short-circuit the
entire `ProgressReceiver::on_event` path on non-TTY environments.
## Why this is needed
indicatif's `inc()` / `set_message()` always acquire the internal
`Mutex<ProgressState>` write-lock and mutate `pos` / `est.buf` /
`message`, regardless of TTY status — only the *rendering* short-
circuits, not the state mutation. On the worker-pool resolve hot path
that's 9000+ lock acquisitions per phase contending across multiple
workers, costing measurable wall time on CI.
## A/B/A/A' bench evidence (CI Linux, ant-design / npmjs)
| Round | indicatif config | Lock count | p1_resolve | σ |
|---|---|---|---|---|
| A | drop entirely | 0 | 2.50s | 0.10 |
| B | full restore | 13713 | 2.73s | 0.07 |
| A' | drop again (verify) | 0 | 2.58s | 0.15 |
| A'' | only "resolved" event | 9142 | 2.65s | 0.05 |
| this PR | TTY gate at startup | 0 in CI | 2.58s | 0.07 |
A→B Δ = +0.23s (+9%); B→A Δ = −0.15s. Three independent runs across
different GH Actions runners and Cloudflare egress conditions.
Amortized cost is approximately linear in lock count
(0.23 × 9142/13713 ≈ 0.15 ≈ predicted; observed 0.15).
## Why this approach
- **Drop-only solutions** (A round) lose the local-dev progress UX.
- **Partial drops** (A'' round, only-resolved) still pay 9142 locks.
- **TTY gate** is the only solution that:
- Pays zero overhead on CI / piped (`*IS_TTY = false` → match never enters)
- Preserves full per-package "resolved {name}" UX on local dev (TTY)
- Is a 1-file, 3-call-site change
## Counter-intuitive finding
The first instinct ("CI has no TTY, indicatif must already short-
circuit internally") is wrong: indicatif separates *rendering*
(skipped on non-TTY / hidden draw target) from *state mutation*
(always runs, always locks). Round A'' confirmed this empirically —
9142 locks with no rendering still cost +0.15s on p1_resolve.
General rule: gate hot-path library calls at the call-site; never
assume the library will short-circuit internally.
## Test plan
- [x] `cargo build --release -p utoo-pm` clean
- [x] `cargo clippy -p utoo-pm --all-targets -- -D warnings --no-deps` clean
- [x] CI bench-phases-linux on PR #2818 round-10 (`f122478b`): p1_resolve
2.58s ±0.07 — matches "indicatif fully dropped" baseline (round-6/8)
- [ ] Local dev TTY: `cargo run --profile release-local --bin utoo -- install`
should show full per-package progress
- [ ] Non-TTY: `… 2>/tmp/stderr.log` should produce zero indicatif ANSI
sequences in the log
Borrowed from PR #2874 (`e17dbc8c`) — surgical optimisation that stacks cleanly on top of the worker-pool architecture. `resolve_via_full_manifest` now checks the version-manifest cache by the resolved concrete version BEFORE the `get_core_version` reparse. Whenever multiple specs (`react@^18`, `react@^18.1`, `react@18.x`) resolve to the same `18.2.0`, only the first call pays the parse cost — sibling calls reuse the cached `Arc<CoreVersionManifest>`. The dual-key cache write at the call-site (line 374-378, populated under both `(name, spec)` and `(name, resolved_version)`) was already in place; this commit just makes the second key into a natural short-circuit point. Side-effect: `resolve_via_full_manifest` return type tightens from `(String, CoreVersionManifest)` to `(String, Arc<CoreVersionManifest>)`, saving an `Arc::new` wrap at the call-site and enabling zero-copy share when the cache hit fires. Test plan: `cargo clippy --all-targets -- -D warnings` clean, ruborist 162 tests pass.
Borrowed two alloc-cleanup commits from PR #2874 (`0d52d568` + `a89a59ea`). Both are orthogonal to the worker-pool architecture and stack cleanly: plain alloc reduction on the resolve hot path, no behavioural change. ## (1) Drop redundant `manifest.versions.clone()` (`0d52d568`) `resolve_target_version` takes `&[String]`, so 4 call sites that prefixed it with `let version_list: Vec<String> = manifest.versions.clone();` were paying for a pointless deep copy of the entire version list per resolve (50-200 entries on popular packages). Pass `&manifest.versions` / `&full_manifest.versions` directly. Saves ~80 String allocs × ~600 packages = ~48k allocs per ant-design install. Sites: - crates/ruborist/src/traits/registry.rs (default `fetch_version_manifest`, default `resolve_package` non-semver branch) - crates/ruborist/src/resolver/registry.rs (`resolve_from_manifest`) - crates/pm/src/cmd/view.rs (utoo view command) ## (2) Move version_list / dist_tags into VersionsInfo (`a89a59ea`) Previously the 200 path of `resolve_full_manifest` deep-cloned `manifest.versions` (Vec<String>) and `manifest.dist_tags` (HashMap) per package fetch to fill the `versions_info` cache slot, while the same fields also moved into `Arc<FullManifest>` for the `full_manifests` slot. Use `std::mem::take` to MOVE both fields out of `manifest` directly into the new `VersionsInfo`. The cached `Arc<FullManifest>` keeps `raw` + scalar metadata; the canonical `version_list` / `dist_tags` now live in the `versions_info` cache slot only. `resolve_via_full_manifest`'s Full branch reads them via `cache.get_versions(name)` (same code path that filled the slot, atomic w.r.t. the inflight gate). `get_core_version(&resolved)` still works on the cached `Arc<FullManifest>` because it parses from `raw` (preserved) — not from the moved-out `versions` field. Saves another ~80 String + 1 HashMap clone × ~600 packages per install, matching the second deep-copy site eliminated by PR #2874. ## Combined effect Worker-pool baseline already parallelises the per-package CPU cost across N workers, so absolute wall-time savings on p1_resolve will likely be in the same noise band as the short-circuit (round-11). The architectural benefit — a single canonical owner for version_list / dist_tags — is worthwhile regardless of bench delta. Test plan: - cargo clippy --all-targets -- -D warnings clean - cargo test -p utoo-ruborist: 162 passed
4 of 5 tasks
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
… CI) (#2876) A/B-verified perf fix isolated as its own PR. Detect stderr's TTY status once at startup (`IS_TTY: Lazy<bool>`) and short-circuit the entire `ProgressReceiver::on_event` path on non-TTY environments. ## Why this is needed indicatif's `inc()` / `set_message()` always acquire the internal `Mutex<ProgressState>` write-lock and mutate `pos` / `est.buf` / `message`, regardless of TTY status — only the *rendering* short- circuits, not the state mutation. On the worker-pool resolve hot path that's 9000+ lock acquisitions per phase contending across multiple workers, costing measurable wall time on CI. ## A/B/A/A' bench evidence (CI Linux, ant-design / npmjs) | Round | indicatif config | Lock count | p1_resolve | σ | |---|---|---|---|---| | A | drop entirely | 0 | 2.50s | 0.10 | | B | full restore | 13713 | 2.73s | 0.07 | | A' | drop again (verify) | 0 | 2.58s | 0.15 | | A'' | only "resolved" event | 9142 | 2.65s | 0.05 | | this PR | TTY gate at startup | 0 in CI | 2.58s | 0.07 | A→B Δ = +0.23s (+9%); B→A Δ = −0.15s. Three independent runs across different GH Actions runners and Cloudflare egress conditions. Amortized cost is approximately linear in lock count (0.23 × 9142/13713 ≈ 0.15 ≈ predicted; observed 0.15). ## Why this approach - **Drop-only solutions** (A round) lose the local-dev progress UX. - **Partial drops** (A'' round, only-resolved) still pay 9142 locks. - **TTY gate** is the only solution that: - Pays zero overhead on CI / piped (`*IS_TTY = false` → match never enters) - Preserves full per-package "resolved {name}" UX on local dev (TTY) - Is a 1-file, 3-call-site change ## Counter-intuitive finding The first instinct ("CI has no TTY, indicatif must already short- circuit internally") is wrong: indicatif separates *rendering* (skipped on non-TTY / hidden draw target) from *state mutation* (always runs, always locks). Round A'' confirmed this empirically — 9142 locks with no rendering still cost +0.15s on p1_resolve. General rule: gate hot-path library calls at the call-site; never assume the library will short-circuit internally. ## Test plan - [x] `cargo build --release -p utoo-pm` clean - [x] `cargo clippy -p utoo-pm --all-targets -- -D warnings --no-deps` clean - [x] CI bench-phases-linux on PR #2818 round-10 (`f122478b`): p1_resolve 2.58s ±0.07 — matches "indicatif fully dropped" baseline (round-6/8) - [ ] Local dev TTY: `cargo run --profile release-local --bin utoo -- install` should show full per-package progress - [ ] Non-TTY: `… 2>/tmp/stderr.log` should produce zero indicatif ANSI sequences in the log
`dd5d9583 perf(pm): gate ProgressReceiver on IsTerminal (...) (#2876)` landed on `next` while the umbrella PR was iterating. Conflict in `pm/util/logger.rs` was purely doc-comment wording — both branches shipped identical IS_TTY gating logic. Take next's wording (the canonical merged-PR text). The umbrella branch's `b2498e91 perf(pm): drop redundant Vec<String>/HashMap clones on resolve hot path` is now part of the diff vs `next`; the architectural follow-up (PR #2878 VersionsRef view + drop double-fill) is separately tracked.
5 of 6 tasks
…fill Upstream `24d2e441 perf(pm): introduce VersionsRef<'a> view for resolve_target_version (#2878)` landed (covers both 63042d7 + 0f85360 that previously diverged on this umbrella branch). Conflict resolution: - Took next's `traits/registry.rs`, `service/registry.rs`, `resolver/registry.rs`, `pm/cmd/view.rs` versions — they have the canonical VersionsRef API. - **Re-applied** `MaybeSend` / `MaybeSync` trait shim (worker-pool needs `+ Send` on `tokio::spawn`'d futures, but trait can't carry that bound on wasm). - **Re-applied** sibling-spec short-circuit in `UnifiedRegistry::resolve_via_full_manifest` (umbrella-only feature borrowed from PR #2874): cache lookup before `get_core_version` reparse; return type tightens to `(String, Arc<CoreVersionManifest>)` to share the cached Arc. - **Re-applied** `MaybeSend/MaybeSync` bounds on `fetch_full_manifest`, `fetch_version_manifest`, `resolve_package` trait methods. - **Re-applied** `#[derive(Clone)]` on `MockRegistryClient` and `MockPackage` (worker-pool requires `R: Clone`). - Discarded the umbrella's `mem::take` versions/dist_tags (`b2498e91`) — superseded by next's drop-in-memory-double-fill which preserves `Arc<FullManifest>` integrity. cargo clippy --all-targets -- -D warnings clean. 245 pm + 162 ruborist tests pass (+ flaky `test_view_twice_no_304_error` unrelated to merge).
Sweep umbrella-only `Instant::now()` / `let t_*` / `start.elapsed()`
plumbing that survived the round-6→round-14 saga but isn't needed in
the final shape. None of these affected program behaviour — they were
diagnostic scaffolding for individual rounds that should not ship.
- `pm/helper/lock.rs::save_package_lock` — drop `t_total` /
`t_serialize` / `t_write` / `t_rename` and the
`tracing::info!("Save package-lock.json: total=...ms ...")` summary
line they fed.
- `pm/service/install.rs` — drop the
`println!(" download X · extract Y · clone Z · M MB fetched")`
trailing line. The user-facing summary above it
(`print_install_counts`) covers what users care about; the per-phase
cumulative-time print was diagnostic-only.
- `pm/util/downloader.rs` — drop `DOWNLOAD_MICROS` / `EXTRACT_MICROS`
/ `DOWNLOAD_BYTES` statics and the per-tarball `t0`/`t1` timing,
plus the now-orphan `phase_totals()` / `PhaseTotals` API.
- `pm/util/cloner.rs` — drop `CLONE_MICROS` static, `clone_micros()`
fn, and the per-clone `t0` timing. `clone_count()` is kept (consumed
by `print_install_counts`).
- `ruborist/resolver/builder.rs::run_preload_phase` — drop the
umbrella-only `tracing::info!("Preload phase: {:.2?} (... success ...)")`.
Restored next's existing `let start = ...; tracing::debug!(...)`
pair that was harmless debug-level instrumentation already upstream.
- `ruborist/resolver/preload.rs` — drop per-package `Instant::now()` +
`elapsed_ms` plumbing (was unused in any external API). Tightens
`Completion<E>` tuple back to `(String, Result<…, …>)` and shrinks
`PreloadStats` to just `success_count` / `failed_count` /
`total_processed`.
cargo clippy --all-targets clean. 245 pm + 162 ruborist + 10 doctests
pass.
Three files had umbrella-only changes that were either obsolete
documentation or cosmetic — converging to next's exact text drops
them from the diff entirely without changing behaviour.
- `pm/helper/lock.rs::save_package_lock`: restore upstream's
inline comment + tighter blank-line layout. (We had reformatted
it during the round-12 mem::take cleanup, but the blank-line
rearrangement isn't necessary now that mem::take is gone.)
- `ruborist/service/{http.rs,manifest.rs}` + `resolver/http.rs`:
rename `pick_client` back to upstream's `get_client`. Pure
naming churn from an early umbrella round; no functional
difference.
- `ruborist/service/manifest.rs::fetch_full_manifest` /
`fetch_version_manifest`: drop the long umbrella-only "Inline
parse on the async worker — simd_json is fast..." documentation
blocks. The architectural conclusion is now implicit (worker-pool
+ inline parse is the design); the extended explanation belonged
in the round-7 commit message, not in production code. Restore
upstream's compact `// Save raw bytes before simd_json mutates
the parse buffer.` for fetch_full_manifest.
cargo clippy --all-targets clean.
245 pm + 162 ruborist + 10 doctests pass (+ flaky network test).
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
Empirical evidence from 4-sample bench on perf/stack-2818-modules (runs 25178030660 / 25180538290 / 25181300495 / 25182053587): | s | bun p1 | utoo p1 | utoo σ | ratio | |---|--------|---------|--------|-------| | 1 | 2.99 (slow) | 3.34 | 0.08 | 1.12 (lucky) | | 2 | 2.39 (fast) | 7.45 | 6.02 | unusable | | 3 | 2.77 | 4.34 | 0.52 | 1.57 | | 4 | 2.60 (fast) | 4.49 | 0.29 | 1.73 | The lucky 1.12 in s1 was a Cloudflare slot where the throttle didn't trigger. Median across clean samples is ~1.57. Pattern: utoo bursts 128 simultaneous connections (worker-pool design issuing all workers at once) while bun ramps up gradually. On today's throttle-heavy slots, the burst trips Cloudflare per-IP rate limiting more often than not. PR #2818's testing chose 128 over 96 by ~6% in stable conditions. Today's bench environment is throttle-heavy enough that 128's aggression hurts more than it helps. Lower the default to 96 to reduce burst aggression and tighten cross-slot variance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
…2818) Cherry-picked from PR #2818 commit 01f25a8 to address the Cloudflare per-IP throttle issue identified in PR #2872 bench iteration 4-7. pcap diff against bun in PR #2818 revealed a DNS round-robin bug. bun distributes 256 preload connections cleanly as 64 per IP across 4 Cloudflare edges. utoo dumped 66 of 128 connections (51%) onto a single IP (104.16.5.34), leaving 11 other IPs with only 5-6 connections each — effectively serialising a quarter of the phase on one overloaded server-side instance. Root cause: `getaddrinfo` for `registry.npmjs.org` returns 10 IPv6 + 12 IPv4 = 22 addresses, v6 first. Origin/next had no rotation at all. PR #2818's parent had a flat `offset % 22` rotation which on GH Actions runners (no IPv6 routing) made offsets 0..10 (= 11 of every 22) all fall through to the *same* first IPv4 entry once Happy Eyeballs gave up on v6. Per-family rotation keeps IPv4 and IPv6 ranges rotated independently and preserves the system resolver's v6-first ordering. Every new connection's first-reachable IPv4 cycles through all v4 addresses, giving ~11 conns per IP instead of 66 on one. This addresses the directly-observed 4-sample evidence on perf/stack-2818-modules where utoo's p1 ratio at cap=128 hit 1.73 on bun-fast slots (utoo 4.49s, bun 2.60s) — symptomatic of utoo's connections piling up on a single throttled Cloudflare IP while bun fanned out across 4 IPs. Adapted to current state: - origin/next had no rotation; PR #2818's parent had flat rotation - This commit adds `rr_counter: AtomicUsize` to `CachingResolver`, the per-family `rotate_addrs` helper, and wires it into both the cache-hit and cache-miss branches of `Resolve::resolve` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
…ne blocking) reqwest's default ClientBuilder negotiates HTTP/2 with the server, which means all parallel manifest requests get multiplexed over a single TCP connection. When ANY one response is slow (e.g. Cloudflare throttle on one of the round-robin'd Cloudflare edge IPs), the H2 stream stalls and ALL subsequent requests on that connection block waiting — head-of-line blocking at the HTTP layer. With force-H1, each `tokio::spawn`-ed worker takes its own TCP stream out of the connection pool. A slow response on one connection only holds up that single response; the other 95 workers continue draining their queues unaffected. Why this matters NOW (post DNS per-family rotation): The 4-sample bench evidence on perf/stack-2818-modules at cap=96 with DNS rotation showed utoo's p1 wall floating at 4.5-7.5s when bun got 2.4-2.9s. Hypothesis confirmed: the bottleneck wasn't IP distribution (rotation didn't help measurably), it was that all our requests serialised through one H2 connection regardless of how many IPs DNS returned. PR #2818 made this same change as part of its full bundle (ratio 1.14) but we'd skipped it earlier under the "within-noise" tag from older testing. Today's bench data shows it's load-bearing — without H1, no amount of cap tuning or DNS rotation moves the needle past ratio 1.42. Pool size 256 = 2x DEFAULT_CONCURRENCY (96), enough headroom that the idle pool never evicts a connection mid-phase even if the request rate briefly exceeds the cap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added a commit
that referenced
this pull request
Apr 30, 2026
Two PR #2818 final-state adjustments missing from PR #2872: ## max_blocking_threads(worker_threads) tokio's default blocking-pool size is 512. With burst-dispatch of ~128 spawn_blocking calls (JSON parse for manifest fetches), the runtime grows the pool unbounded which causes thread-creation thrash on cold starts. PR #2818's testing showed bimodal walls (2.7s fast vs 6.9s thrash) on this; capping the pool to worker_threads count keeps the pool warm and eliminates the thrash peak. ## cap 96 → 128 cap=96 was chosen empirically in commit 610ebb6 when H2 head-of-line blocking was the dominant bottleneck — gentler burst of 96 connections tripped the per-IP Cloudflare throttle less than 128 did. With `.http1_only()` (commit a28045b) requests now use independent TCP streams, so per-IP throttle distributes across the DNS-rotated pool. 128 is the sweet spot historically (avg_conc=84 in PR #2818's sweep), and the extra 33% concurrency saturates more of the available Cloudflare connection budget without triggering the per-IP throttle that was the original concern. Combined with prior commits (worker-pool + DNS rotation + H1), these two adjustments complete the PR #2818 perf bundle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks done
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.19s | 0.69s | 9.95s | 9.49s | 575M | 299.0K |
| utoo-next | 10.31s | 0.92s | 10.58s | 12.67s | 1.17G | 154.1K |
| utoo-npm | 10.69s | 0.74s | 11.03s | 12.93s | 1.28G | 167.9K |
| utoo | 10.02s | 1.27s | 10.73s | 11.70s | 1.32G | 165.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 18.3K | 16.3K | 1.16G | 6M | 1.81G | 1.70G | 1M |
| utoo-next | 184.7K | 146.3K | 1.13G | 5M | 1.68G | 1.68G | 2M |
| utoo-npm | 181.6K | 166.2K | 1.14G | 5M | 1.68G | 1.68G | 2M |
| utoo | 114.8K | 58.9K | 1.14G | 6M | 1.68G | 1.68G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.58s | 0.09s | 3.57s | 1.08s | 471M | 180.5K |
| utoo-next | 6.16s | 0.26s | 4.71s | 1.72s | 421M | 73.3K |
| utoo-npm | 6.75s | 0.33s | 5.00s | 1.77s | 428M | 74.4K |
| utoo | 2.69s | 0.19s | 5.22s | 1.66s | 563M | 85.2K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 13.3K | 2.8K | 201M | 3M | 104M | - | 1M |
| utoo-next | 84.2K | 3.6K | 197M | 2M | 7M | 3M | 2M |
| utoo-npm | 70.9K | 1.9K | 208M | 2M | 9M | 5M | 2M |
| utoo | 26.3K | 16.7K | 200M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 7.24s | 1.00s | 6.15s | 9.36s | 577M | 197.1K |
| utoo-next | 12.05s | 5.38s | 5.37s | 11.42s | 785M | 114.6K |
| utoo-npm | 9.52s | 0.99s | 5.52s | 11.20s | 825M | 113.5K |
| utoo | 7.91s | 0.77s | 5.32s | 10.26s | 859M | 107.7K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 6.6K | 7.5K | 993M | 4M | 1.73G | 1.73G | 1M |
| utoo-next | 160.6K | 85.1K | 965M | 4M | 1.67G | 1.67G | 2M |
| utoo-npm | 147.8K | 107.5K | 965M | 4M | 1.67G | 1.67G | 2M |
| utoo | 100.0K | 57.1K | 965M | 4M | 1.67G | 1.67G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.34s | 0.05s | 0.18s | 2.39s | 136M | 31.9K |
| utoo-next | 2.29s | 0.10s | 0.49s | 3.78s | 82M | 19.3K |
| utoo-npm | 2.32s | 0.10s | 0.56s | 3.80s | 82M | 18.9K |
| utoo | 2.30s | 0.35s | 0.43s | 3.35s | 60M | 13.2K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 240 | 31 | 7M | 25K | 1.88G | 1.72G | 1M |
| utoo-next | 49.5K | 20.9K | 22K | 11K | 1.67G | 1.67G | 2M |
| utoo-npm | 47.1K | 20.6K | 23K | 11K | 1.67G | 1.67G | 2M |
| utoo | 17.6K | 8.9K | 28K | 18K | 1.68G | 1.67G | 2M |
npmmirror.com: no output captured.
elrrrrrrr
added a commit
that referenced
this pull request
May 8, 2026
Two changes after the GHA bench on the previous commit (PR #2916, run 25559625024) showed the concurrency=256 hypothesis was wrong on GHA's environment. Revert concurrency 256 → 64 --------------------------- The new fetch-timing instrumentation shipped in the previous commit caught the surprise: GHA's pcap-vs-local profile is the *opposite* of what local Mac measurements suggested. metric local Mac GHA Linux avg_request 399ms 70ms ← network MUCH faster on GHA avg_body 50ms 20ms avg_parse 730µs 266ms ← parse 365× SLOWER on GHA Mechanism: `parse_json_off_runtime` dispatches to `rayon::spawn`, and rayon's pool size is `num_cpus` (= 2 on GHA ubuntu-latest). Bumping concurrency 64 → 256 queued 256 manifest parses behind 2 rayon workers — head-of-line blocking. avg_parse jumped from ~10ms to 266ms wall, dragging p1 utoo wall from 3.10s up to 3.33s. Restore manifest-bench ---------------------- Brought back `crates/manifest-bench` (originally landed in the post-#2818 driver hunt, dropped in af714eb once #2818 graduated). It's a single-binary HTTP-only fetch tool that strips out the ruborist pipeline (no BFS, no dedup, no parse, no project cache, no lockfile write) — fires `GET <registry>/<name>` in parallel and reports the same diag shape as the new `p1-breakdown` lines. Goal: separate the network ceiling from the resolver pipeline so the next round of p1 experiments (parse offload, partial parse, dedicated parse pool, etc.) can be evaluated against a stable "pure network" baseline. Knobs (unchanged from the original drop): --concurrency N sweep without rebuilding utoo --reps N run same workload back-to-back --single-version use /<name>/latest (smaller bodies) --user-agent X UA-fingerprint experiments --http1-only H2 vs H1 toggle --accept X override Accept header Same TLS stack as ruborist (rustls + aws-lc-rs, native roots). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added a commit
that referenced
this pull request
May 8, 2026
…inux build-linux now also builds + uploads `manifest-bench` when a phases bench is going to run (label or dispatch). bench-phases-linux downloads the binary and runs it after the regular phase-isolated benchmark. Sweep mirrors the original (#2818-era) wire-in: concurrency: 32 / 64 / 96 / 128 / 192 / 256 (HTTP/1.1, full manifest) protocol: H1 vs H2-negotiate (cap=128) endpoint: full vs `/<name>/latest` (cap=128, smaller bodies) UA: default vs `Bun/1.2.21` (cap=128) Output goes to /tmp/pm-bench-output/manifest-bench-npmjs.log and ships in the existing pm-bench-logs-linux artifact — no PR comment surface (the headline phases bench comment stays the same). Why now: the new ruborist `p1-breakdown` instrumentation showed sum_parse on GHA can dominate when concurrency is bumped (256: sum_parse 728s vs sum_request 193s). To attribute the bun-vs-utoo gap on p1_resolve we need a "pure HTTP" baseline that strips out ruborist's parse / BFS / dedup / lockfile path. manifest-bench is that baseline: same TLS stack as ruborist (rustls + aws-lc-rs, native roots), no resolver pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Stale — no updates since May 1; superseded by #2905 (par_chunks(64)) which captured the install-write win. Archived. |
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
Started as "intra-package sequential tarball writes" (single-line perf tweak) and evolved into a multi-week, data-driven preload-resolver overhaul plus a complete bench infrastructure. This PR is now too large to review safely and is being split — see "PR split plan" at the bottom.
This description captures the full exploration journey so subsequent split-PRs can reference it as context.
End-to-end results (ant-design / npmjs.org / GitHub Actions ubuntu)
utoo now matches or beats bun on 3 of 4 phases. The remaining p1 gap (+0.4s) is fully accounted for and at the architectural floor.
Journey timeline — p1_resolve preload wall
Key architectural moves
1. Worker-pool preload (commit
ed7b551e) — the core winProblem:
FuturesUnorderedpolled all preload futures from a single main task. Eachresolve_packagecall had 5+ awaits (cache check +OnceMap::get_or_init+RetryIf+request.send+bytes+ parse). Every yield round-tripped through main, saturating it. Even after killing every Mutex/clone hot-path, avg_conc held at 55-60 while standalone manifest-bench (same reqwest stack, no resolver) sustained 92.Fix: N long-lived
tokio::spawnworkers pulling work fromArc<SegQueue<Dep>>withDashSetdedup. Workers run on tokio's global executor independently; main task only drains anmpsc::unbounded_channelfor receiver events + on_manifest callback. Termination viadispatched/completed: AtomicUsize+Notify.Trait surface:
RegistryClientfutures gained+ Sendbounds,MockRegistryClientderivesClone.preload_manifeststakesArc<R>instead of&R. BoundR: Clone + Send + Sync + 'static, R::Error: Sendpropagated up the API chain.Result: avg_conc 55 → 84 (CI), wall 3.10s → 2.15s (-31%).
2. HTTP stack ceiling — empirically verified (
crates/manifest-bench)Built a standalone HTTP-only fetch tool that strips out everything ruborist does on top of network: BFS, dedup, parse, project cache, lockfile. Dispatches identical workload through identical reqwest+rustls+tokio stack.
Key data point (CI ant-design npmjs.org cap=128, controlled for same Cloudflare conditions):
ruborist now matches the HTTP stack ceiling. Further preload speedup requires a non-reqwest stack (HTTP/3, custom hyper Connector, etc.). This empirically refuted ~10 candidate optimizations as not the bottleneck.
3. Allocator hot-path cleanup
Each
resolve_packagecall allocated severalStrings on the resolver hot path. Cumulatively ~10k allocs per ant-design preload, all on workers competing for shared allocator. Eliminated via:normalize_specreturnsCow<'_, str>instead of(String, String)(commit12d34dd4)Arc<FullManifest>inMemoryCache— clone is atomic bump not deep HashMap clone (commit337d4f26)versions.keys.clone()removed from cache-hit path — pass borrow directly (commitbb256ecd)format!()dedup key kept (testing removal broke transitive prefetch)4. Inline parse (drop
spawn_blocking)Worker-pool surfaced parse blocking-pool queue saturation:
queue p95=200ms sum=70-89sover 2730 manifests. Cap=4 on CI was funneling all parses through a 4-slot queue.Fix: inline
simd_jsonparse on the worker (commitf3f616d8). 1-5ms CPU per manifest is acceptable on async worker; eliminates dispatch + queue overhead.5. Workspace discovery parallelization
find_workspaces_from_pkgwas reading 200+ workspacepackage.jsonfiles sequentially in aforloop. Replaced withFuturesUnordered(commitbf149957). Saved150ms (200×1ms serial → ~10ms parallel).6. tarball extraction + cloner — install path
extractor.rs:rayon::spawnper package +par_chunks(32)intra-package writes. Verified essential by testing removal — sequential write regressed p3 +3.67s, σ exploded 0.04→2.85.cloner.rs: kept ontokio::task::spawn_blocking. Verified essential by testing rayon migration — that regressed p3 +2.65s due to oversubscription with extractor's rayon pool. The currenttokio::blocking + cap=worker_threadsis the local optimum for hardlink (single short syscall, no fan-out benefit).Failed experiments (kept here so future attempts don't repeat)
All reverted. Each tested with CI bench data.
25814552reverted byc610a58202ef05624e125908/ae2a6088/5a897e4a1a16d25ereverted byf379f7a9de5c83edreverted by90e421a5max_blocking_threads = N*4132ef36ereverted by9a071f29c7c847d6reverted by2f1092c39229e160reverted bye38329c8Cloudflare per-source-IP throttle — measured
Standalone manifest-bench cap sweep (cap=32/64/96/128/192/256):
Conclusion: npmjs's Cloudflare frontend throttles on a per-source-IP basis (CI runner egress). Cap=128 is the sweet spot for our setup. Going wider triggers per-req inflation that cancels the parallelism gain.
This refuted the original "raise cap aggressively" intuition and 6 cap-sweep experiments.
Diagnostic infrastructure (kept, valuable for future work)
5e3c12d2): per-preloadwall / busy / sum / avg_conc / p50/p95/max / cpu_tail6e0e60e5):spawn_blockingqueue p50/p95 + exec p50/p95b92aa81f): standalone HTTP-only A/B against ruboristf6846d6a): every CI run produces standalone control numbers88a4b056): warm 304 path restored without per-package syscall stormTotal wall account (p1_resolve, latest CI)
Remaining optimization space (out of this PR's scope)
rustls-native-certslazy load,reqwest::Clientdeferred build)FullManifestraw bytes + index instead ofOwnedValuetree (1.46GB → ~700MB)None are small enough to fit this PR.
PR split plan
Per discussion, this PR is being decomposed into 4-5 focused PRs:
This PR will close once 1-4 land. Track in subsequent PRs.
Test plan
cargo fmt+cargo clippy --all-targets -- -D warnings --no-depsclean across cratescargo test -p utoo-pm245 passedcargo test -p utoo-ruborist164 + 10 doctests passedpm-bench-phasesCI run >50 times, results documented above🤖 Generated with Claude Code