clean: Optimize clean with multiple -p specifiers

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 rust-lang#16263); for Zed, `cargo clean --workspace` went down from 73 seconds to 3 seconds.
We have 216 workspace members.

Co-authored-by: dino <dinojoaocosta@gmail.com>
Co-authored-by: Ed Page <eopage@gmail.com>

Author: 3 people

src/cargo/core/compiler/build_context/target_info.rs

@@ -145,6 +145,13 @@ impl FileType {
             should_replace_hyphens: true,
         }
     }
+
+    pub fn output_prefix_suffix(&self, target: &Target) -> (String, String) {
+        (
+            format!("{}{}-", self.prefix, target.crate_name()),
+            self.suffix.clone(),
+        )
+    }
 }
 
 impl TargetInfo {

src/cargo/ops/cargo_clean.rs

@@ -9,8 +9,9 @@ use crate::util::interning::InternedString;
 use crate::util::{GlobalContext, Progress, ProgressStyle};
 use anyhow::bail;
 use cargo_util::paths;
-use indexmap::IndexSet;
-use std::collections::{HashMap, HashSet};
+use indexmap::{IndexMap, IndexSet};
+
+use std::ffi::OsString;
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::rc::Rc;
@@ -250,31 +251,40 @@ fn clean_specs(
             }
         }
     } else {
-        // Try to reduce the amount of times we iterate over the same target directory by storing away
-        // the directories we've iterated over (and cleaned for a given package).
-        let mut cleaned_packages: HashMap<_, HashSet<_>> = HashMap::default();
+        let mut dirs_to_clean = DirectoriesToClean::default();
         for pkg in packages {
-            let pkg_dir = format!("{}-*", pkg.name());
+            let pkg_fingerprint_prefix = format!("{}-", pkg.name());
             clean_ctx.progress.on_cleaning_package(&pkg.name())?;
 
             // Clean fingerprints.
+            let package = &*pkg.name();
             for (_, layout) in &layouts_with_host {
-                let dir = escape_glob_path(layout.build_dir().legacy_fingerprint())?;
-                clean_ctx.rm_rf_package_glob_containing_hash(
-                    &pkg.name(),
-                    &Path::new(&dir).join(&pkg_dir),
-                )?;
+                dirs_to_clean.mark_utf(layout.build_dir().legacy_fingerprint(), |path| {
+                    if !path.starts_with(&pkg_fingerprint_prefix) {
+                        return false;
+                    }
+                    let Some((pkg_name, _)) = path.rsplit_once('-') else {
+                        return false;
+                    };
+
+                    pkg_name == package
+                });
             }
 
             for target in pkg.targets() {
                 if target.is_custom_build() {
                     // Get both the build_script_build and the output directory.
                     for (_, layout) in &layouts_with_host {
-                        let dir = escape_glob_path(layout.build_dir().build())?;
-                        clean_ctx.rm_rf_package_glob_containing_hash(
-                            &pkg.name(),
-                            &Path::new(&dir).join(&pkg_dir),
-                        )?;
+                        dirs_to_clean.mark_utf(layout.build_dir().build(), |path| {
+                            if !path.starts_with(&pkg_fingerprint_prefix) {
+                                return false;
+                            }
+                            let Some((pkg_name, _)) = path.rsplit_once('-') else {
+                                return false;
+                            };
+
+                            pkg_name == package
+                        });
                     }
                     continue;
                 }
@@ -304,15 +314,15 @@ fn clean_specs(
                             }
                             _ => (layout.build_dir().legacy_deps(), Some(artifact_dir.dest())),
                         };
-                        let mut dir_glob_str = escape_glob_path(dir)?;
-                        let dir_glob = Path::new(&dir_glob_str);
+
                         for file_type in file_types {
                             // Some files include a hash in the filename, some don't.
-                            let hashed_name = file_type.output_filename(target, Some("*"));
+                            let (prefix, suffix) = file_type.output_prefix_suffix(target);
                             let unhashed_name = file_type.output_filename(target, None);
-
-                            clean_ctx.rm_rf_glob(&dir_glob.join(&hashed_name))?;
-                            clean_ctx.rm_rf(&dir.join(&unhashed_name))?;
+                            dirs_to_clean.mark_utf(&dir, |filename| {
+                                (filename.starts_with(&prefix) && filename.ends_with(&suffix))
+                                    || unhashed_name == filename
+                            });
 
                             // Remove the uplifted copy.
                             if let Some(uplift_dir) = uplift_dir {
@@ -324,44 +334,82 @@ fn clean_specs(
                                 clean_ctx.rm_rf(&dep_info)?;
                             }
                         }
-                        let unhashed_dep_info = dir.join(format!("{}.d", crate_name));
-                        clean_ctx.rm_rf(&unhashed_dep_info)?;
+                        let unhashed_dep_info = format!("{}.d", crate_name);
+                        dirs_to_clean.mark_utf(dir, |filename| filename == unhashed_dep_info);
 
-                        if !dir_glob_str.ends_with(std::path::MAIN_SEPARATOR) {
-                            dir_glob_str.push(std::path::MAIN_SEPARATOR);
-                        }
-                        dir_glob_str.push('*');
-                        let dir_glob_str: Rc<str> = dir_glob_str.into();
-                        if cleaned_packages
-                            .entry(dir_glob_str.clone())
-                            .or_default()
-                            .insert(crate_name.clone())
-                        {
-                            let paths = [
+                        dirs_to_clean.mark_utf(dir, |path| {
+                            if path.starts_with(&path_dash) {
                                 // Remove dep-info file generated by rustc. It is not tracked in
                                 // file_types. It does not have a prefix.
-                                (path_dash, ".d"),
+                                path.ends_with(".d")
+                            } else if path.starts_with(&path_dot) {
                                 // Remove split-debuginfo files generated by rustc.
-                                (path_dot, ".o"),
-                                (path_dot, ".dwo"),
-                                (path_dot, ".dwp"),
-                            ];
-                            clean_ctx.rm_rf_prefix_list(&dir_glob_str, &paths)?;
-                        }
+                                [".o", ".dwo", ".dwp"]
+                                    .iter()
+                                    .any(|suffix| path.ends_with(suffix))
+                            } else {
+                                false
+                            }
+                        });
 
                         // TODO: what to do about build_script_build?
-                        let dir = escape_glob_path(layout.build_dir().incremental())?;
-                        let incremental = Path::new(&dir).join(format!("{}-*", crate_name));
-                        clean_ctx.rm_rf_glob(&incremental)?;
+                        let incremental_prefix = format!("{}-", crate_name);
+                        dirs_to_clean.mark_utf(layout.build_dir().incremental(), |path| {
+                            path.starts_with(&incremental_prefix)
+                        });
                     }
                 }
             }
         }
+        clean_ctx.rm_rf_all(dirs_to_clean)?;
     }
 
     Ok(())
 }
 
+#[derive(Default)]
+struct DirectoriesToClean {
+    dir_contents: IndexMap<PathBuf, IndexSet<OsString>>,
+    to_remove: IndexSet<PathBuf>,
+}
+
+impl DirectoriesToClean {
+    fn mark(&mut self, directory: &Path, mut should_remove_entry: impl FnMut(&OsString) -> bool) {
+        let entry = match self.dir_contents.entry(directory.to_owned()) {
+            indexmap::map::Entry::Occupied(occupied_entry) => occupied_entry.into_mut(),
+            indexmap::map::Entry::Vacant(vacant_entry) => {
+                let Ok(dir_entries) = std::fs::read_dir(directory.to_owned()) else {
+                    return;
+                };
+                vacant_entry.insert(
+                    dir_entries
+                        .into_iter()
+                        .flatten()
+                        .map(|entry| entry.file_name())
+                        .collect::<IndexSet<_>>(),
+                )
+            }
+        };
+
+        entry.retain(|filename| {
+            let should_remove = should_remove_entry(filename);
+            if should_remove {
+                self.to_remove.insert(directory.join(filename));
+            }
+            !should_remove
+        });
+    }
+
+    fn mark_utf(&mut self, directory: &Path, mut should_remove_entry: impl FnMut(&str) -> bool) {
+        self.mark(directory, move |path| {
+            let Some(as_utf) = path.to_str() else {
+                return false;
+            };
+            should_remove_entry(as_utf)
+        });
+    }
+}
+
 fn escape_glob_path(pattern: &Path) -> CargoResult<String> {
     let pattern = pattern
         .to_str()
@@ -384,34 +432,8 @@ impl<'gctx> CleanContext<'gctx> {
         }
     }
 
-    /// Glob remove artifacts for the provided `package`
-    ///
-    /// Make sure the artifact is for `package` and not another crate that is prefixed by
-    /// `package` by getting the original name stripped of the trailing hash and possible
-    /// extension
-    fn rm_rf_package_glob_containing_hash(
-        &mut self,
-        package: &str,
-        pattern: &Path,
-    ) -> CargoResult<()> {
-        // TODO: Display utf8 warning to user?  Or switch to globset?
-        let pattern = pattern
-            .to_str()
-            .ok_or_else(|| anyhow::anyhow!("expected utf-8 path"))?;
-        for path in glob::glob(pattern)? {
-            let path = path?;
-
-            let pkg_name = path
-                .file_name()
-                .and_then(std::ffi::OsStr::to_str)
-                .and_then(|artifact| artifact.rsplit_once('-'))
-                .ok_or_else(|| anyhow::anyhow!("expected utf-8 path"))?
-                .0;
-
-            if pkg_name != package {
-                continue;
-            }
-
+    fn rm_rf_all(&mut self, dirs: DirectoriesToClean) -> CargoResult<()> {
+        for path in dirs.to_remove {
             self.rm_rf(&path)?;
         }
         Ok(())
@@ -428,30 +450,6 @@ impl<'gctx> CleanContext<'gctx> {
         Ok(())
     }
 
-    /// Removes files matching a glob and any of the provided filename patterns (prefix/suffix pairs).
-    ///
-    /// This function iterates over files matching a glob (`pattern`) and removes those whose
-    /// filenames start and end with specific prefix/suffix pairs. It should be more efficient for
-    /// operations involving multiple prefix/suffix pairs, as it iterates over the directory
-    /// only once, unlike making multiple calls to [`Self::rm_rf_glob`].
-    fn rm_rf_prefix_list(
-        &mut self,
-        pattern: &str,
-        path_matchers: &[(&str, &str)],
-    ) -> CargoResult<()> {
-        for path in glob::glob(pattern)? {
-            let path = path?;
-            let filename = path.file_name().and_then(|name| name.to_str()).unwrap();
-            if path_matchers
-                .iter()
-                .any(|(prefix, suffix)| filename.starts_with(prefix) && filename.ends_with(suffix))
-            {
-                self.rm_rf(&path)?;
-            }
-        }
-        Ok(())
-    }
-
     pub fn rm_rf(&mut self, path: &Path) -> CargoResult<()> {
         let meta = match fs::symlink_metadata(path) {
             Ok(meta) => meta,