perf(pm): prioritize demand install downloads#2997
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 introduces a mechanism to prioritize pending downloads when a new waiter is added to an existing download key. This is achieved through a new prioritize_download method that moves the relevant package to the front of the download queue. A unit test was also added to verify this behavior. Feedback was provided regarding the performance of the prioritize_download function, specifically noting that calling package.key() inside a linear search results in unnecessary string allocations; a more efficient comparison method was suggested.
| fn prioritize_download(&mut self, key: &str) { | ||
| // A preloaded tarball may become the current BFS frontier's blocker | ||
| // later. Promote only pending work; active downloads keep running. | ||
| let Some(index) = self | ||
| .download_queue | ||
| .iter() | ||
| .position(|package| package.key() == key) | ||
| else { | ||
| return; | ||
| }; | ||
| if let Some(package) = self.download_queue.remove(index) { | ||
| self.download_queue.push_front(package); | ||
| } | ||
| } |
There was a problem hiding this comment.
The prioritize_download function performs a linear search over the download_queue. In the current implementation, package.key() is called for every element in the queue until a match is found. Since key() allocates a new String via format!, this results in a large number of temporary allocations in the scheduler's hot loop, especially for large dependency trees where the queue might contain thousands of items. You can avoid these allocations by comparing the name and version fields directly against the provided key string.
fn prioritize_download(&mut self, key: &str) {
// A preloaded tarball may become the current BFS frontier's blocker
// later. Promote only pending work; active downloads keep running.
let Some(index) = self.download_queue.iter().position(|package| {
package.name.len() + package.version.len() + 1 == key.len()
&& key.as_bytes().get(package.name.len()) == Some(&b'@')
&& key.starts_with(&package.name)
&& key.ends_with(&package.version)
}) else {
return;
};
if index > 0 {
if let Some(package) = self.download_queue.remove(index) {
self.download_queue.push_front(package);
}
}
}
elrrrrrrr
added
benchmark
|
Closing this draft as a negative experiment: two GHA runs did not improve p3, and the rerun regressed p3 clearly. |
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 8.95s | 0.06s | 10.27s | 10.41s | 668M | 308.7K |
| utoo-next | 8.54s | 0.16s | 10.49s | 12.85s | 999M | 126.7K |
| utoo-npm | 9.66s | 1.76s | 10.81s | 13.08s | 1018M | 130.5K |
| utoo | 8.10s | 0.34s | 11.78s | 11.45s | 1017M | 145.1K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 16.6K | 18.6K | 1.19G | 7M | 1.86G | 1.75G | 1M |
| utoo-next | 137.6K | 108.5K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo-npm | 148.9K | 113.9K | 1.16G | 6M | 1.71G | 1.70G | 2M |
| utoo | 73.5K | 57.2K | 1.16G | 7M | 1.71G | 1.70G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 2.32s | 0.05s | 3.87s | 1.12s | 492M | 183.9K |
| utoo-next | 3.18s | 0.07s | 5.43s | 2.15s | 614M | 80.4K |
| utoo-npm | 3.24s | 0.02s | 5.50s | 2.18s | 611M | 80.7K |
| utoo | 2.50s | 0.09s | 5.99s | 1.74s | 642M | 120.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 11.7K | 3.8K | 203M | 3M | 108M | - | 1M |
| utoo-next | 77.3K | 94.8K | 200M | 3M | 7M | 3M | 2M |
| utoo-npm | 77.5K | 97.8K | 200M | 3M | 7M | 3M | 2M |
| utoo | 16.4K | 20.1K | 202M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.80s | 0.13s | 6.27s | 10.21s | 618M | 206.6K |
| utoo-next | 8.38s | 2.79s | 5.01s | 11.33s | 527M | 65.3K |
| utoo-npm | 6.47s | 0.48s | 5.07s | 11.06s | 534M | 63.3K |
| utoo | 5.44s | 0.22s | 5.49s | 9.63s | 482M | 56.2K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 5.2K | 7.9K | 1019M | 4M | 1.76G | 1.76G | 1M |
| utoo-next | 121.8K | 53.0K | 990M | 3M | 1.70G | 1.70G | 2M |
| utoo-npm | 104.8K | 52.6K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo | 52.1K | 37.9K | 990M | 3M | 1.70G | 1.70G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.10s | 0.02s | 0.19s | 2.37s | 134M | 33.3K |
| utoo-next | 2.25s | 0.07s | 0.47s | 3.82s | 78M | 18.3K |
| utoo-npm | 2.08s | 0.06s | 0.46s | 3.76s | 78M | 18.7K |
| utoo | 2.03s | 0.07s | 0.34s | 3.31s | 50M | 11.0K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 260 | 21 | 5M | 15K | 1.91G | 1.73G | 1M |
| utoo-next | 41.9K | 19.4K | 2K | 11K | 1.70G | 1.70G | 2M |
| utoo-npm | 41.3K | 18.9K | 1K | 6K | 1.70G | 1.70G | 2M |
| utoo | 7.5K | 4.5K | 1K | 4K | 1.71G | 1.70G | 2M |
npmmirror.com: no output captured.
1 participant
Summary
Experiment for p3 cold install download priority.
ensure_cloneneeds a package that is still only pending from preload, move that download to the front of the pending queueRationale
prefetch_lock_downloadscan enqueue the whole lockfile before depth-by-depth install starts. A later BFS demand currently attaches a waiter but keeps the tarball at its original preload position. This experiment keeps preload throughput while letting the current BFS frontier unblock earlier.Verification
GHA Benchmark
Conclusion: negative / not worth integrating. Two GHA runs did not improve p3; the rerun regressed p3 clearly.
Baseline is #2989 latest Linux/npmjs (
26102983933).26107409974)26108512218)The hypothesis was plausible, but GHA data says pending download reprioritization adds instability without improving the p3 cold path.