perf(pm): skip install progress off tty#2982
Conversation
elrrrrrrr
added
A-Pkg Manager
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements prefetching for registry tarballs and refactors the installation scheduler to perform extraction and cloning using synchronous I/O within a Rayon worker pool. It also introduces utility functions for progress bar updates that incorporate TTY checks. Review feedback identifies an inefficiency in the directory cloning logic where a HashSet is used redundantly and suggests improving error handling for directory creation to avoid silent failures.
| let mut created_dirs = HashSet::new(); | ||
| for dir in &dirs { | ||
| if created_dirs.insert(dir.clone()) | ||
| && let Err(e) = fs::create_dir_all(dir) | ||
| && e.kind() != io::ErrorKind::AlreadyExists | ||
| { | ||
| return Err(e).with_context(|| err_msg.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of a HashSet and dir.clone() here is redundant and inefficient. The dirs vector is populated by collect_entries using a depth-first traversal, which ensures that each directory path is unique and added in a top-down order. Consequently, created_dirs.insert(dir.clone()) will always be true, and the extra allocations and hashing overhead are unnecessary. Additionally, the error handling for fs::create_dir_all is problematic: create_dir_all returns Ok(()) if the directory already exists, and only returns an error (like AlreadyExists) if a non-directory file is in the way. By checking e.kind() != io::ErrorKind::AlreadyExists, the code silently ignores cases where a file exists at the intended directory path, which will lead to confusing failures later when trying to create files inside that path.
for dir in &dirs {
if let Err(e) = fs::create_dir_all(dir) {
return Err(e).with_context(|| err_msg.clone());
}
}
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.18s | 0.28s | 10.50s | 10.52s | 721M | 335.2K |
| utoo-next | 8.68s | 0.93s | 10.85s | 12.89s | 990M | 126.7K |
| utoo-npm | 8.43s | 0.57s | 10.84s | 12.64s | 997M | 128.7K |
| utoo | 7.55s | 0.15s | 11.28s | 12.04s | 916M | 141.0K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 15.0K | 18.6K | 1.20G | 6M | 1.88G | 1.76G | 1M |
| utoo-next | 143.4K | 101.2K | 1.17G | 5M | 1.73G | 1.72G | 2M |
| utoo-npm | 127.8K | 91.9K | 1.17G | 5M | 1.73G | 1.72G | 2M |
| utoo | 75.5K | 49.6K | 1.17G | 6M | 1.73G | 1.72G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 1.95s | 0.04s | 4.06s | 1.06s | 533M | 166.7K |
| utoo-next | 3.02s | 0.04s | 5.58s | 2.10s | 615M | 77.2K |
| utoo-npm | 3.06s | 0.03s | 5.53s | 2.06s | 612M | 76.8K |
| utoo | 2.38s | 0.04s | 6.14s | 1.62s | 627M | 119.9K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 8.5K | 4.5K | 203M | 3M | 108M | - | 1M |
| utoo-next | 71.9K | 92.0K | 201M | 2M | 7M | 3M | 2M |
| utoo-npm | 71.2K | 93.0K | 201M | 2M | 7M | 3M | 2M |
| utoo | 14.4K | 19.1K | 204M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.67s | 0.13s | 6.40s | 10.09s | 625M | 206.7K |
| utoo-next | 6.90s | 1.64s | 5.03s | 11.14s | 475M | 60.8K |
| utoo-npm | 6.12s | 0.32s | 5.13s | 10.91s | 455M | 62.4K |
| utoo | 5.72s | 0.28s | 4.72s | 10.52s | 471M | 59.4K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 4.2K | 7.2K | 1.00G | 4M | 1.77G | 1.77G | 1M |
| utoo-next | 108.5K | 50.6K | 999M | 3M | 1.72G | 1.72G | 3M |
| utoo-npm | 96.3K | 46.7K | 998M | 2M | 1.72G | 1.72G | 3M |
| utoo | 62.8K | 42.0K | 998M | 2M | 1.72G | 1.72G | 3M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.50s | 0.02s | 0.18s | 2.45s | 134M | 31.6K |
| utoo-next | 2.37s | 0.16s | 0.49s | 3.86s | 80M | 18.6K |
| utoo-npm | 2.39s | 0.04s | 0.50s | 3.88s | 81M | 18.9K |
| utoo | 2.18s | 0.01s | 0.37s | 3.50s | 51M | 11.4K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 310 | 24 | 4K | 27K | 1.88G | 1.77G | 1M |
| utoo-next | 40.9K | 19.0K | 306K | 8K | 1.72G | 1.72G | 2M |
| utoo-npm | 42.0K | 19.3K | 307K | 13K | 1.72G | 1.72G | 2M |
| utoo | 13.1K | 9.3K | 306K | 7K | 1.73G | 1.72G | 2M |
npmmirror.com: no output captured.
1 participant
Summary
finish_progress_barno-op off TTY as wellHypothesis
ProgressReceiveralready avoids indicatif work off TTY, but PM install still calledPROGRESS_BAR.inc()directly for every package placement. In CI and2>&1 | tee, indicatif rendering is hidden, but each update still mutates progress state under its internal lock. Warm-link p4 has thousands of package placements, so this may show up as lock contention, vCtx/iCtx, and sys time.This AB checks whether removing hidden progress-bar state mutation lowers p3/p4 ctx without changing local TTY UX.
Validation
cargo fmtCARGO_NET_OFFLINE=true cargo clippy -p utoo-pm --all-targets -- -D warnings --no-depsBenchmark Result
GHA run 1: https://github.com/utooland/utoo/actions/runs/26031609818
GHA run 2: https://github.com/utooland/utoo/actions/runs/26032374627
npmjs phase bench:
Compared with #2975 p4
13.1-13.7K / 9.3-9.9K, this branch is consistently at the low end:13.0-13.1K / 9.2-9.3K. Wall is neutral/slower on these globally slow runners, but the ctx/sys signal reproduces and the code fixes the existing inconsistency where resolver progress is TTY-gated while install progress still mutates hidden indicatif state.Conclusion: keep as a small ctx/syscall cleanup candidate and consider folding into the main PR.