perf(pm): split downloader cache primitives#3032
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 refactors the package downloader utility by modularizing cache lookup and extraction logic into dedicated helper functions such as resolve_seeded_cache_path, registry_cache_lookup, and extract_to_cache. The review feedback suggests improving error robustness by propagating IO errors in registry_cache_lookup rather than masking them with a default value, and reducing code duplication in extract_to_cache by utilizing the existing lookup helper.
| if crate::fs::try_exists(&cache_path.join("_resolved")) | ||
| .await | ||
| .unwrap_or(false) | ||
| { |
There was a problem hiding this comment.
The use of unwrap_or(false) on try_exists masks potential IO errors (e.g., permission issues). Since this function returns a Result, it should propagate the error using ? to allow callers to handle it appropriately.
if crate::fs::try_exists(&cache_path.join("_resolved")).await? {| let cache_path = registry_cache_path(name, version); | ||
|
|
||
| if crate::fs::try_exists(&cache_path.join("_resolved")) | ||
| .await | ||
| .unwrap_or(false) | ||
| { | ||
| REUSE_COUNT.fetch_add(1, Ordering::Relaxed); | ||
| return Ok(cache_path); | ||
| } |
There was a problem hiding this comment.
This block duplicates the logic already implemented in registry_cache_lookup. Reusing the helper function improves maintainability and reduces redundant filesystem checks, which aligns with the performance goals of this PR.
| let cache_path = registry_cache_path(name, version); | |
| if crate::fs::try_exists(&cache_path.join("_resolved")) | |
| .await | |
| .unwrap_or(false) | |
| { | |
| REUSE_COUNT.fetch_add(1, Ordering::Relaxed); | |
| return Ok(cache_path); | |
| } | |
| if let Some(cache_path) = registry_cache_lookup(name, version).await? { | |
| return Ok(cache_path); | |
| } | |
| let cache_path = registry_cache_path(name, version); |
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 12.72s | 4.17s | 10.41s | 10.21s | 576M | 308.2K |
| utoo-next | 9.94s | 0.60s | 10.85s | 12.88s | 977M | 120.4K |
| utoo-npm | 10.36s | 1.00s | 11.08s | 13.21s | 1.02G | 123.7K |
| utoo | 19.53s | 11.12s | 10.93s | 13.26s | 982M | 121.7K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 21.0K | 16.9K | 1.19G | 7M | 1.86G | 1.75G | 1M |
| utoo-next | 145.6K | 106.3K | 1.16G | 6M | 1.71G | 1.70G | 2M |
| utoo-npm | 155.4K | 120.0K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo | 170.2K | 101.0K | 1.16G | 6M | 1.71G | 1.70G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.71s | 0.04s | 3.88s | 1.12s | 490M | 189.4K |
| utoo-next | 3.45s | 0.19s | 5.50s | 1.87s | 615M | 95.2K |
| utoo-npm | 4.84s | 0.77s | 5.79s | 2.25s | 597M | 82.1K |
| utoo | 3.93s | 0.41s | 5.47s | 1.83s | 605M | 77.8K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 14.4K | 3.0K | 202M | 3M | 107M | - | 1M |
| utoo-next | 56.9K | 70.4K | 200M | 3M | 7M | 3M | 2M |
| utoo-npm | 86.1K | 89.5K | 200M | 3M | 7M | 3M | 2M |
| utoo | 56.8K | 69.3K | 200M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.68s | 0.03s | 6.40s | 10.28s | 555M | 200.8K |
| utoo-next | 8.02s | 0.87s | 5.08s | 11.21s | 477M | 59.0K |
| utoo-npm | 8.31s | 1.09s | 5.16s | 11.24s | 466M | 57.1K |
| utoo | 10.16s | 3.52s | 5.22s | 11.54s | 486M | 64.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 7.3K | 8.8K | 1019M | 4M | 1.76G | 1.76G | 1M |
| utoo-next | 118.3K | 59.4K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo-npm | 117.6K | 58.6K | 990M | 3M | 1.70G | 1.70G | 2M |
| utoo | 147.9K | 56.2K | 990M | 4M | 1.70G | 1.70G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.24s | 0.04s | 0.21s | 2.34s | 134M | 33.4K |
| utoo-next | 2.29s | 0.15s | 0.48s | 3.80s | 79M | 18.5K |
| utoo-npm | 2.16s | 0.05s | 0.51s | 3.74s | 79M | 18.0K |
| utoo | 2.26s | 0.09s | 0.48s | 3.73s | 78M | 18.3K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 273 | 21 | 5M | 34K | 1.91G | 1.75G | 1M |
| utoo-next | 42.2K | 19.0K | 16K | 27K | 1.70G | 1.70G | 2M |
| utoo-npm | 40.9K | 19.4K | 16K | 11K | 1.70G | 1.70G | 2M |
| utoo | 39.8K | 18.7K | 15K | 9K | 1.71G | 1.70G | 2M |
npmmirror.com: no output captured.
1 participant
Summary
First review-sized split from #3029 / #2948 for the installer stack.
Separates downloader cache primitives from the old
download_to_cachecompatibility path:resolve_seeded_cache_pathfor git/file/http slots already seeded by resolution;registry_cache_pathandregistry_cache_lookup;extract_to_cachefor already-downloaded tarball bytes;download_and_extract_to_cacheas the primitive future scheduler jobs will call;download_to_cacheOnceMap + semaphore path intact and routes it through the new helpers.This PR prepares the scheduler split without moving dedupe/concurrency ownership yet.
Size
1 file changed, 91 insertions(+), 38 deletions(-)Validation
cargo fmtcargo check -p utoo-pmcargo test -p utoo-pm util::downloader(2 passed)cargo clippy --all-targets -- -D warnings --no-depspack-napistill warns locally becausenext.jsis a symlink in this worktree; clippy exits successfully.