perf(gc): batch untracked package directory traversals#17245
Conversation
rustbot
added
A-testing-cargo-itself
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Are there intermediate refactors you could do for this that would make the more relevant to digest parts clearer? See https://epage.github.io/dev/pr-style/#c-split for more context |
| let mut builder = OverrideBuilder::new(root); | ||
| let Some((first, rest)) = paths.split_first() else { | ||
| return Ok(Vec::new()); | ||
| }; | ||
|
|
||
| for pattern in patterns { | ||
| builder.add(pattern)?; | ||
| } | ||
| let overrides = builder.build()?; |
There was a problem hiding this comment.
nit: please group the creation of builder with the building of it, rather than splitting it around getting first/rest
There was a problem hiding this comment.
Sure, I'll see if I can address all of these right now.
| // Cargo's hasher isn't reachable from cargo-util. | ||
| // The `stat` dominates the walk, so the hasher speed shouldn't make much difference here. | ||
| #[allow(clippy::disallowed_types)] | ||
| let path_to_index: std::collections::HashMap<&Path, usize> = | ||
| paths.iter().copied().zip(0..).collect(); |
There was a problem hiding this comment.
Should we address that by adding a dependency?
|
Note: did a cursory look only |
The existing benchmarks cover updating an already-populated database. Syncing one with the files on disk is the expensive part, and is what a cache left behind by a cargo that didn't track it has to go through. Sizing those files is a separate step, only taken when a size limit is given, so measure it both ways.
`du` builds a thread pool of its own. For a small directory, that costs far more than the walk itself. Sizing many directories by calling `du` in a loop pays that cost every time. `du_each` walks any number of directories with a single pool. It keeps a running total per directory so each still gets its own size. `du` becomes a thin wrapper over it.
This code originally spent far more time on starting threads than traversal. On an 8-core machine, sizing 500 directories of a few files each spawned ~4000 threads to visit ~3000 files. This change traverses them all with a single pool instead, keeping a total per directory so each still gets its own size. The same case now spawns 8 threads and cuts context switches from ~6400 to ~80. The global_tracker_sync benchmark, syncing 500 such directories with --max-download-size, drops from 1.15s to 9.5ms - a 120x speedup.
ed7f675 to
3ca4e5b
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
4 participants
What Does This PR Try to Resolve?
Addresses part of #13058
Without this change, the code would size every package directory, one
duat a time.Each
duspins up its own thread pool.On an 8-core machine, sizing 500 directories of a few files each spawned ~4000 threads to visit ~3000 files.
With this change, we size them all with one shared pool.
How to Test and Review This PR?
with_size(sizes every dir)age_only(no sizing, control)