perf(clean): Optimize (legacy) clean with multiple -p specifiers

[osiewicz]

Co-authored-by: dino [email protected]

What does this PR try to resolve?

This commit optimizes implementation of cargo clean -p by reducing the amount of directory walks that take place.
We now batch calls to rm_rf_prefix_list, thus potentially avoiding multiple walks over a single subdirectory. In practice this helps us significantly reduce the runtime for clearing large workspaces (as implemented in #16263); for Zed, cargo clean --workspace went down from 73 seconds to 3 seconds.

We have 216 workspace members.

How to test and review this PR?

We've tested it by hand, running it against regex, ruff and zed codebases.

This PR is still marked as draft, as I don't love the code. I would also understand if y'all were against merging this, given that new build directory layout is in flight.

[epage]

Have you tried out the performance of cargo clean --workspace with -Zbuild-dir-new-layout? Unsure what the timeline will be for stabilizing that so I can understand if you want to optimize this in the mean time.

[epage]

Are we really benefiting from all of this filesystem globbing?

Should we just walk all of the content upfront, get it into a Vec, and then do a retain call on that, pattern matching against the files for the different packages, and then delete what is left?

[osiewicz]

Have you tried out the performance of cargo clean --workspace with -Zbuild-dir-new-layout?

No, I did not try -Zbuild-dir-new-layout. I'll do that and report back.

Are we really benefiting from all of this filesystem globbing?

I don't think we benefit much from globbing, particularly because most of it's use cases are about a simple prefix/suffix matching. I'm in the process of coalescing file walking further. I'll push that up to this branch before marking this PR as ready to review.

Should we just walk all of the content upfront, get it into a Vec, and then do a retain call on that, pattern matching against the files for the different packages, and then delete what is left?

If we could make the pattern matching part straightforward (to read/maintain) then sure, that'd be best. I'm clinging onto the existing setup for no good reason; the end goal for me is to have a faster cargo clean -p --workspace.

[osiewicz]

I am very happy to report that -Zbuild-dir-new-layout would make cargo clean --workspace take about a second on Zed codebase. :)

This PR is currently at 2.6s. I think we could reach a similar ballpark with the current build dir layout.. But obviously, this code will be removed at some point either way.

[rustbot]

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[osiewicz]

I've rebased the existing implementation on top of #16304 - while we could push the perf further, I think this is good enough (after all, it gets us from 70s down to 3s) for our use case.

[osiewicz]

r? epage

[epage]

I don't think we benefit much from globbing, particularly because most of it's use cases are about a simple prefix/suffix matching. I'm in the process of coalescing file walking further. I'll push that up to this branch before marking this PR as ready to review.

In my mind, we would do file walking by

let mut cache: HashMap<PathBuf, Vec<OsString>> = Default::default();
let mut to_remove: Vec<PathBuf> = Default::default();
for ... {

    //...
    let file_names = cache.entry(dir).or_insert_with(|| std::fs::read_dir(dir));
    file_names.retain(|name| {
         let remove name.starts_with(prefix) && name.ends_with(suffix);
         if remove {
             to_remove.push(dir.join(name));
         }
         !remove
    });
}

for path in to_remove {
    self.rm_rf(path)?;
}

(psuedo code, no preference on the exact details including whether cache tracks file names or paths, likely it should be wrapped in a struct for easier use)

Benefits

• Could then apply to the new layout as well. While the new layout benefits is already fast, we've been fixing bugs where we've been doing more globbing (including one still pending, fix(layout): Put examples in their unit dir in new layout #16335).
• More flexibility for deciding what gets removed rather than having to fit within any specific rule system of prefix+suffix, globs, etc.
• Guaranteed that each directory is only read once

[epage]

Why do we do the rsplit_once check earlier but not here?

[osiewicz]

Earlier call sites were using rm_rf_package_glob_containing_dash, whereas this one used to call rm_rf_prefix_list. We could probably use strip_prefix(crate_name) here followed by a dispatch on the following character.

[epage]

Eh, lets leave it for now. In a follow up, we should probably extract a starts_with_pkg_name(name, sep) so we consistently rsplit_once.

[epage]

Thanks!

View changes since this review