perf(pm): flush clone unblockers early#2994
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to flush clone completions immediately for parent packages that unblock dependencies, rather than waiting for the entire batch to finish. This is implemented via a new CloneBatchJob struct and an OpDone::CloneProgress event. Review feedback suggests simplifying the CloneBatchJob struct by removing the redundant target field, as this information is already accessible through the ready field's specification.
| struct CloneBatchJob { | ||
| target: PathBuf, | ||
| ready: ReadyClone, | ||
| flush_on_complete: bool, | ||
| } |
There was a problem hiding this comment.
| .filter_map(|batch_job| { | ||
| let result = clone_package_from_cache_indexed_sync( | ||
| &job.spec.package.name, | ||
| &job.spec.package.version, | ||
| &job.spec.package.tarball_url, | ||
| &job.cache.path, | ||
| &job.spec.target, | ||
| job.cache.index.as_ref(), | ||
| &batch_job.ready.spec.package.name, | ||
| &batch_job.ready.spec.package.version, | ||
| &batch_job.ready.spec.package.tarball_url, | ||
| &batch_job.ready.cache.path, | ||
| &batch_job.ready.spec.target, | ||
| batch_job.ready.cache.index.as_ref(), | ||
| ) | ||
| .map_err(|e| format!("{e:#}")); | ||
| (target, result) | ||
| if batch_job.flush_on_complete { | ||
| let _ = done_tx.send(OpDone::CloneProgress { | ||
| target: batch_job.target, | ||
| result, | ||
| }); | ||
| None | ||
| } else { | ||
| Some((batch_job.target, result)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
By moving batch_job.ready.spec.target directly into the result messages, we avoid using the redundant target field from the batch job struct.
.filter_map(|batch_job| {
let result = clone_package_from_cache_indexed_sync(
&batch_job.ready.spec.package.name,
&batch_job.ready.spec.package.version,
&batch_job.ready.spec.package.tarball_url,
&batch_job.ready.cache.path,
&batch_job.ready.spec.target,
batch_job.ready.cache.index.as_ref(),
)
.map_err(|e| format!("{e:#}"));
let target = batch_job.ready.spec.target;
if batch_job.flush_on_complete {
let _ = done_tx.send(OpDone::CloneProgress {
target,
result,
});
None
} else {
Some((target, result))
}
})| assert_eq!(batch[0].target, parent_target); | ||
| assert!(batch[0].flush_on_complete); | ||
| assert_eq!( | ||
| state.clone_queue[0].spec.target, | ||
| batch[1].target, | ||
| PathBuf::from("/tmp/project/node_modules/leaf") | ||
| ); | ||
| assert!(!batch[1].flush_on_complete); | ||
| assert!(state.clone_active.contains(&parent_target)); | ||
| assert!(state.clone_queue.is_empty()); |
There was a problem hiding this comment.
Updating the test assertions to reflect the removal of the redundant target field in CloneBatchJob.
| assert_eq!(batch[0].target, parent_target); | |
| assert!(batch[0].flush_on_complete); | |
| assert_eq!( | |
| state.clone_queue[0].spec.target, | |
| batch[1].target, | |
| PathBuf::from("/tmp/project/node_modules/leaf") | |
| ); | |
| assert!(!batch[1].flush_on_complete); | |
| assert!(state.clone_active.contains(&parent_target)); | |
| assert!(state.clone_queue.is_empty()); | |
| assert_eq!(batch[0].ready.spec.target, parent_target); | |
| assert!(batch[0].flush_on_complete); | |
| assert_eq!( | |
| batch[1].ready.spec.target, | |
| PathBuf::from("/tmp/project/node_modules/leaf") | |
| ); | |
| assert!(!batch[1].flush_on_complete); | |
| assert!(state.clone_active.contains(&parent_target)); | |
| assert!(state.clone_queue.is_empty()); |
📊 pm-bench-phases ·
|
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 9.24s | 0.30s | 10.49s | 10.36s | 746M | 323.7K |
| utoo-next | 8.74s | 1.43s | 10.86s | 12.88s | 1001M | 125.7K |
| utoo-npm | 9.43s | 1.93s | 10.88s | 12.58s | 997M | 122.4K |
| utoo | 7.49s | 0.16s | 11.38s | 11.37s | 952M | 141.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 15.5K | 18.4K | 1.19G | 6M | 1.88G | 1.76G | 1M |
| utoo-next | 133.2K | 94.4K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo-npm | 136.6K | 92.1K | 1.16G | 5M | 1.71G | 1.70G | 2M |
| utoo | 67.6K | 47.4K | 1.16G | 6M | 1.71G | 1.70G | 2M |
p1_resolve
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 1.97s | 0.07s | 4.08s | 1.04s | 532M | 160.5K |
| utoo-next | 3.09s | 0.03s | 5.54s | 2.11s | 621M | 86.1K |
| utoo-npm | 3.07s | 0.01s | 5.60s | 2.11s | 603M | 83.3K |
| utoo | 2.45s | 0.06s | 6.14s | 1.75s | 656M | 127.6K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 8.8K | 4.4K | 202M | 3M | 107M | - | 1M |
| utoo-next | 71.6K | 90.2K | 200M | 2M | 7M | 3M | 2M |
| utoo-npm | 71.9K | 93.2K | 200M | 2M | 7M | 3M | 2M |
| utoo | 14.9K | 20.6K | 202M | 3M | 7M | 3M | 2M |
p3_cold_install
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 6.75s | 0.16s | 6.38s | 10.16s | 608M | 210.4K |
| utoo-next | 7.24s | 2.23s | 5.03s | 11.25s | 455M | 62.0K |
| utoo-npm | 5.94s | 0.28s | 5.17s | 11.14s | 508M | 59.4K |
| utoo | 5.46s | 0.26s | 4.97s | 9.66s | 512M | 57.1K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 3.8K | 7.1K | 1019M | 3M | 1.76G | 1.76G | 1M |
| utoo-next | 106.3K | 51.2K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo-npm | 102.3K | 49.2K | 989M | 3M | 1.70G | 1.70G | 2M |
| utoo | 52.6K | 34.0K | 989M | 2M | 1.70G | 1.70G | 2M |
p4_warm_link
| PM | wall | ±σ | user | sys | RSS | pgMinor |
|---|---|---|---|---|---|---|
| bun | 3.40s | 0.01s | 0.20s | 2.42s | 134M | 31.4K |
| utoo-next | 2.54s | 0.49s | 0.50s | 3.84s | 78M | 18.1K |
| utoo-npm | 2.36s | 0.10s | 0.50s | 3.83s | 79M | 18.2K |
| utoo | 2.14s | 0.04s | 0.34s | 3.31s | 50M | 11.1K |
| PM | vCtx | iCtx | netRX | netTX | cache | node_mod | lock |
|---|---|---|---|---|---|---|---|
| bun | 286 | 25 | 3K | 13K | 1.87G | 1.76G | 1M |
| utoo-next | 40.5K | 17.3K | 7K | 10K | 1.70G | 1.70G | 2M |
| utoo-npm | 41.1K | 18.4K | 6K | 7K | 1.70G | 1.70G | 2M |
| utoo | 7.5K | 4.3K | 6K | 6K | 1.71G | 1.70G | 2M |
npmmirror.com: no output captured.
Summary
This is a p3/p4 scheduler experiment on top of #2989 after #2991 integration.
#2991 proved that parent clone completions are on the nested placement critical path. It handles that by prioritizing parent unblockers and putting them in a single-job batch. This PR keeps that early parent signal but avoids dedicating a whole worker batch to it:
CloneProgressimmediately after a parent unblocker finishes;CloneProgresscompletes that target and wakes nested children, but does not release the worker slot;CloneBatch.Hypothesis
Validation
cargo fmtcargo test -p utoo-pm install_schedulercargo clippy -p utoo-pm --all-targets -- -D warnings --no-depsBench Result
GHA Linux npmjs run
26097117399:Reference #2989 integrated head (
fd38cfff):2609474755426095851789Conclusion: inconclusive / do not integrate for now. The early-flush implementation is logically reasonable, but it does not beat #2991 clearly enough to justify the extra completion state and worker-slot semantics.