perf(pm): route installs through the install scheduler#3076
Open
elrrrrrrr wants to merge 1 commit into
Open
Conversation
PR 2 of 2. Wire the staged primitives into the new InstallScheduler and retire
the legacy pipeline:
- service/install_scheduler: actor owning download→extract→clone with
backpressure, dedup, parent/child clone blocking, and install counts.
- install.rs: route through the scheduler; drop the baseline/delta stats hack.
- Remove service/pipeline/{mod,receiver,worker}, clone_package_once,
CLONE_CACHE/DOWNLOAD_CACHE OnceMap, download_to_cache/resolve_cache_path.
- downloader slimmed to pure network; package_cache owns the _resolved contract.
- Statistics owned by SchedulerState — no util-layer global atomics.
- Remove the module-level allow(dead_code) from PR1 (primitives now consumed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
elrrrrrrr
added
A-Pkg Manager
![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 replaces the previous pipeline installer architecture with a new InstallScheduler to manage concurrent package downloading, extracting, and cloning. It removes global atomic counters and caches from the utility layers, delegating state management and counting directly to the scheduler. Feedback on the changes suggests removing the panic recovery logic (std::panic::catch_unwind) in the scheduler's clone worker to allow panics to propagate naturally as unrecoverable bugs.
Comment on lines
+483
to
+493
| let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | ||
| clone_package_sync(&PackageClone { | ||
| name: &job.spec.package.name, | ||
| version: &job.spec.package.version, | ||
| tarball_url: &job.spec.package.tarball_url, | ||
| cache: &job.cache_path, | ||
| target: &job.spec.target, | ||
| }) | ||
| .map_err(|e| format!("{e:#}")) | ||
| })) | ||
| .unwrap_or_else(|_| Err("install clone worker panicked".to_string())); |
Contributor
There was a problem hiding this comment.
Implementing recovery logic for panics via std::panic::catch_unwind violates the general rule that panics should be treated as unrecoverable bugs. Instead of catching the panic and converting it to an Err, let the panic propagate naturally so it can be detected and fixed as an unrecoverable bug.
let result = clone_package_sync(&PackageClone {
name: &job.spec.package.name,
version: &job.spec.package.version,
tarball_url: &job.spec.package.tarball_url,
cache: &job.cache_path,
target: &job.spec.target,
})
.map_err(|e| format!("{e:#}"));References
- Do not implement recovery logic for panics. Panics should be treated as unrecoverable bugs that need to be fixed, not as transient, recoverable errors.
This was referenced May 25, 2026
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.78s | 0.22s | 10.56s | 10.28s | 598M | 288.3K |
| utoo-next | 8.44s | 0.14s | 10.60s | 12.21s | 1001M | 128.1K |
| utoo-npm | 8.80s | 0.30s | 10.73s | 12.48s | 1011M | 125.8K |
| utoo | 8.09s | 0.19s | 10.34s | 12.07s | 987M | 113.2K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 22.4K | 18.9K | 1.22G | 7M | 1.93G | 1.81G | 1M |
| utoo-next | 120.9K | 91.5K | 1.20G | 6M | 1.78G | 1.77G | 2M |
| utoo-npm | 142.5K | 105.7K | 1.20G | 5M | 1.78G | 1.76G | 2M |
| utoo | 131.8K | 79.3K | 1.20G | 6M | 1.78G | 1.77G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.94s | 0.10s | 3.84s | 1.17s | 475M | 170.0K |
| utoo-next | 3.38s | 0.05s | 5.16s | 2.02s | 619M | 86.8K |
| utoo-npm | 3.52s | 0.05s | 5.38s | 2.45s | 621M | 80.9K |
| utoo | 3.35s | 0.02s | 5.23s | 2.07s | 616M | 80.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 16.8K | 3.0K | 205M | 3M | 109M | - | 1M |
| utoo-next | 60.4K | 77.2K | 203M | 3M | 7M | 3M | 2M |
| utoo-npm | 89.7K | 93.3K | 203M | 3M | 7M | 3M | 2M |
| utoo | 61.0K | 77.1K | 203M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.93s | 0.32s | 6.61s | 9.99s | 556M | 202.8K |
| utoo-next | 6.55s | 0.27s | 5.27s | 10.62s | 472M | 62.5K |
| utoo-npm | 7.01s | 0.91s | 5.34s | 10.68s | 470M | 60.0K |
| utoo | 6.28s | 0.08s | 5.07s | 10.55s | 467M | 57.8K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 10.3K | 9.6K | 1.02G | 5M | 1.82G | 1.82G | 1M |
| utoo-next | 109.8K | 54.0K | 1017M | 3M | 1.76G | 1.76G | 2M |
| utoo-npm | 111.4K | 52.0K | 1017M | 3M | 1.76G | 1.76G | 2M |
| utoo | 96.5K | 56.9K | 1017M | 3M | 1.76G | 1.76G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.40s | 0.08s | 0.20s | 2.47s | 136M | 32.9K |
| utoo-next | 2.48s | 0.29s | 0.51s | 3.81s | 81M | 18.6K |
| utoo-npm | 2.21s | 0.09s | 0.50s | 3.80s | 81M | 18.7K |
| utoo | 1.94s | 0.01s | 0.39s | 3.46s | 53M | 11.7K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 359 | 21 | 5M | 48K | 1.98G | 1.81G | 1M |
| utoo-next | 40.3K | 18.0K | 329K | 10K | 1.76G | 1.76G | 2M |
| utoo-npm | 40.9K | 19.4K | 330K | 12K | 1.76G | 1.76G | 2M |
| utoo | 21.6K | 13.7K | 329K | 17K | 1.77G | 1.76G | 2M |
npmmirror.com: no output captured.
2 tasks
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
1 participant
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.
PR 2 of 2 (stacked on #3075). Wire the staged primitives into the new
InstallSchedulerand retire the legacy pipeline engine.service/install_scheduler: actor owning download→extract→clone with backpressure, dedup, parent/child clone blocking, and install counts.install.rs: route through the scheduler; drop the baseline/delta stats hack.service/pipeline/*,clone_package_once,CLONE_CACHE/DOWNLOAD_CACHEOnceMap,download_to_cache/resolve_cache_path.downloaderslimmed to pure network;package_cacheowns the_resolvedcontract.SchedulerState— no util-layer global atomics.#.PR1 (#3075) + this = the full installer scheduler rework. Validated for no perf regression on the combined tree (pm-bench-phases p0/p1/p3/p4 wall ≤ baseline).
🤖 Generated with Claude Code