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?
There was a problem hiding this comment.
I ended up going with a BTreeMap, instead, which doesn't require #[allow(clippy::disallowed_types)].
|
Note: did a cursory look only |
ed7f675 to
3ca4e5b
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| /// Returns the disk usage of each of `paths`, which must all be within `root`. | ||
| // skip(paths) to keep a potentially large slice out of tracing. | ||
| #[tracing::instrument(skip(paths))] | ||
| fn du_each(root: &Path, paths: &[&Path], table_name: &str) -> CargoResult<Vec<u64>> { |
There was a problem hiding this comment.
| fn du_each(root: &Path, paths: &[&Path], table_name: &str) -> CargoResult<Vec<u64>> { | |
| fn du_each(root: &Path, relative_paths: &[&Path], table_name: &str) -> CargoResult<Vec<u64>> { |
Maybe we could try building the full path internally to make it more explicit that paths must be within the root.
Actually, I am not sure if this really helps. But I also couldn’t come up with a better idea. I’m not sure how we can express “each of paths, which must all be within root” in a better way.
@epage What do you think?
There was a problem hiding this comment.
@epage I just fixed merge conflicts.
All checks currently pass. Are there any other changes you want me to make here?
This comment has been minimized.
This comment has been minimized.
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. The same case now spawns 8 threads and cuts context switches from ~6400 to ~80. The benchmark, which synced 500 such directories, drops from 1.15s to 9.5ms - a 120x speedup.
3ca4e5b to
6980ba7
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. |
6 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)