Auto merge of #82347 - the8472:parallelize-tidy, r=Mark-Simulacrum

Parallelize tidy

Split off from #81833

While that PR brings wall time of `x.py test tidy` down to 0m2.847s adding this one on top should bring it down to 0m1.673s.

r? `@Mark-Simulacrum`

Previous concerns can be found at #81833 (comment) and #81833 (comment)

Author: bors

Cargo.lock

@@ -5308,6 +5308,7 @@ name = "tidy"
 version = "0.1.0"
 dependencies = [
  "cargo_metadata 0.11.1",
+ "crossbeam-utils 0.8.0",
  "lazy_static",
  "regex",
  "walkdir",

src/bootstrap/test.rs

@@ -854,6 +854,7 @@ impl Step for Tidy {
         cmd.arg(&builder.src);
         cmd.arg(&builder.initial_cargo);
         cmd.arg(&builder.out);
+        cmd.arg(builder.jobs().to_string());
         if builder.is_verbose() {
             cmd.arg("--verbose");
         }

src/tools/tidy/Cargo.toml

@@ -10,6 +10,7 @@ cargo_metadata = "0.11"
 regex = "1"
 lazy_static = "1"
 walkdir = "2"
+crossbeam-utils = "0.8.0"
 
 [[bin]]
 name = "rust-tidy"

src/tools/tidy/src/bins.rs

@@ -5,91 +5,126 @@
 //! huge amount of bloat to the Git history, so it's good to just ensure we
 //! don't do that again.
 
-use std::path::Path;
+pub use os_impl::*;
 
 // All files are executable on Windows, so just check on Unix.
 #[cfg(windows)]
-pub fn check(_path: &Path, _output: &Path, _bad: &mut bool) {}
+mod os_impl {
+    use std::path::Path;
+
+    pub fn check_filesystem_support(_sources: &[&Path], _output: &Path) -> bool {
+        return false;
+    }
+
+    pub fn check(_path: &Path, _bad: &mut bool) {}
+}
 
 #[cfg(unix)]
-pub fn check(path: &Path, output: &Path, bad: &mut bool) {
+mod os_impl {
     use std::fs;
     use std::os::unix::prelude::*;
+    use std::path::Path;
     use std::process::{Command, Stdio};
 
+    enum FilesystemSupport {
+        Supported,
+        Unsupported,
+        ReadOnlyFs,
+    }
+
+    use FilesystemSupport::*;
+
     fn is_executable(path: &Path) -> std::io::Result<bool> {
         Ok(path.metadata()?.mode() & 0o111 != 0)
     }
 
-    // We want to avoid false positives on filesystems that do not support the
-    // executable bit. This occurs on some versions of Window's linux subsystem,
-    // for example.
-    //
-    // We try to create the temporary file first in the src directory, which is
-    // the preferred location as it's most likely to be on the same filesystem,
-    // and then in the output (`build`) directory if that fails. Sometimes we
-    // see the source directory mounted as read-only which means we can't
-    // readily create a file there to test.
-    //
-    // See #36706 and #74753 for context.
-    let mut temp_path = path.join("tidy-test-file");
-    match fs::File::create(&temp_path).or_else(|_| {
-        temp_path = output.join("tidy-test-file");
-        fs::File::create(&temp_path)
-    }) {
-        Ok(file) => {
-            let exec = is_executable(&temp_path).unwrap_or(false);
-            std::mem::drop(file);
-            std::fs::remove_file(&temp_path).expect("Deleted temp file");
-            if exec {
-                // If the file is executable, then we assume that this
-                // filesystem does not track executability, so skip this check.
-                return;
-            }
+    pub fn check_filesystem_support(sources: &[&Path], output: &Path) -> bool {
+        // We want to avoid false positives on filesystems that do not support the
+        // executable bit. This occurs on some versions of Window's linux subsystem,
+        // for example.
+        //
+        // We try to create the temporary file first in the src directory, which is
+        // the preferred location as it's most likely to be on the same filesystem,
+        // and then in the output (`build`) directory if that fails. Sometimes we
+        // see the source directory mounted as read-only which means we can't
+        // readily create a file there to test.
+        //
+        // See #36706 and #74753 for context.
+
+        fn check_dir(dir: &Path) -> FilesystemSupport {
+            let path = dir.join("tidy-test-file");
+            match fs::File::create(&path) {
+                Ok(file) => {
+                    let exec = is_executable(&path).unwrap_or(false);
+                    std::mem::drop(file);
+                    std::fs::remove_file(&path).expect("Deleted temp file");
+                    // If the file is executable, then we assume that this
+                    // filesystem does not track executability, so skip this check.
+                    return if exec { Unsupported } else { Supported };
+                }
+                Err(e) => {
+                    // If the directory is read-only or we otherwise don't have rights,
+                    // just don't run this check.
+                    //
+                    // 30 is the "Read-only filesystem" code at least in one CI
+                    //    environment.
+                    if e.raw_os_error() == Some(30) {
+                        eprintln!("tidy: Skipping binary file check, read-only filesystem");
+                        return ReadOnlyFs;
+                    }
+
+                    panic!("unable to create temporary file `{:?}`: {:?}", path, e);
+                }
+            };
         }
-        Err(e) => {
-            // If the directory is read-only or we otherwise don't have rights,
-            // just don't run this check.
-            //
-            // 30 is the "Read-only filesystem" code at least in one CI
-            //    environment.
-            if e.raw_os_error() == Some(30) {
-                eprintln!("tidy: Skipping binary file check, read-only filesystem");
-                return;
-            }
 
-            panic!("unable to create temporary file `{:?}`: {:?}", temp_path, e);
+        for &source_dir in sources {
+            match check_dir(source_dir) {
+                Unsupported => return false,
+                ReadOnlyFs => {
+                    return match check_dir(output) {
+                        Supported => true,
+                        _ => false,
+                    };
+                }
+                _ => {}
+            }
         }
+
+        return true;
     }
 
-    super::walk_no_read(
-        path,
-        &mut |path| super::filter_dirs(path) || path.ends_with("src/etc"),
-        &mut |entry| {
-            let file = entry.path();
-            let filename = file.file_name().unwrap().to_string_lossy();
-            let extensions = [".py", ".sh"];
-            if extensions.iter().any(|e| filename.ends_with(e)) {
-                return;
-            }
+    #[cfg(unix)]
+    pub fn check(path: &Path, bad: &mut bool) {
+        crate::walk_no_read(
+            path,
+            &mut |path| crate::filter_dirs(path) || path.ends_with("src/etc"),
+            &mut |entry| {
+                let file = entry.path();
+                let filename = file.file_name().unwrap().to_string_lossy();
+                let extensions = [".py", ".sh"];
+                if extensions.iter().any(|e| filename.ends_with(e)) {
+                    return;
+                }
 
-            if t!(is_executable(&file), file) {
-                let rel_path = file.strip_prefix(path).unwrap();
-                let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/");
-                let output = Command::new("git")
-                    .arg("ls-files")
-                    .arg(&git_friendly_path)
-                    .current_dir(path)
-                    .stderr(Stdio::null())
-                    .output()
-                    .unwrap_or_else(|e| {
-                        panic!("could not run git ls-files: {}", e);
-                    });
-                let path_bytes = rel_path.as_os_str().as_bytes();
-                if output.status.success() && output.stdout.starts_with(path_bytes) {
-                    tidy_error!(bad, "binary checked into source: {}", file.display());
+                if t!(is_executable(&file), file) {
+                    let rel_path = file.strip_prefix(path).unwrap();
+                    let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/");
+                    let output = Command::new("git")
+                        .arg("ls-files")
+                        .arg(&git_friendly_path)
+                        .current_dir(path)
+                        .stderr(Stdio::null())
+                        .output()
+                        .unwrap_or_else(|e| {
+                            panic!("could not run git ls-files: {}", e);
+                        });
+                    let path_bytes = rel_path.as_os_str().as_bytes();
+                    if output.status.success() && output.stdout.starts_with(path_bytes) {
+                        tidy_error!(bad, "binary checked into source: {}", file.display());
+                    }
                 }
-            }
-        },
-    )
+            },
+        )
+    }
 }

src/tools/tidy/src/main.rs

@@ -6,61 +6,108 @@
 
 use tidy::*;
 
+use crossbeam_utils::thread::{scope, ScopedJoinHandle};
+use std::collections::VecDeque;
 use std::env;
+use std::num::NonZeroUsize;
 use std::path::PathBuf;
 use std::process;
+use std::str::FromStr;
+use std::sync::atomic::{AtomicBool, Ordering};
 
 fn main() {
     let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into();
     let cargo: PathBuf = env::args_os().nth(2).expect("need path to cargo").into();
     let output_directory: PathBuf =
         env::args_os().nth(3).expect("need path to output directory").into();
+    let concurrency: NonZeroUsize =
+        FromStr::from_str(&env::args().nth(4).expect("need concurrency"))
+            .expect("concurrency must be a number");
 
     let src_path = root_path.join("src");
     let library_path = root_path.join("library");
     let compiler_path = root_path.join("compiler");
 
     let args: Vec<String> = env::args().skip(1).collect();
 
-    let mut bad = false;
     let verbose = args.iter().any(|s| *s == "--verbose");
 
-    // Checks over tests.
-    debug_artifacts::check(&src_path, &mut bad);
-    ui_tests::check(&src_path, &mut bad);
-
-    // Checks that only make sense for the compiler.
-    errors::check(&compiler_path, &mut bad);
-    error_codes_check::check(&src_path, &mut bad);
-
-    // Checks that only make sense for the std libs.
-    pal::check(&library_path, &mut bad);
-
-    // Checks that need to be done for both the compiler and std libraries.
-    unit_tests::check(&src_path, &mut bad);
-    unit_tests::check(&compiler_path, &mut bad);
-    unit_tests::check(&library_path, &mut bad);
-
-    bins::check(&src_path, &output_directory, &mut bad);
-    bins::check(&compiler_path, &output_directory, &mut bad);
-    bins::check(&library_path, &output_directory, &mut bad);
-
-    style::check(&src_path, &mut bad);
-    style::check(&compiler_path, &mut bad);
-    style::check(&library_path, &mut bad);
-
-    edition::check(&src_path, &mut bad);
-    edition::check(&compiler_path, &mut bad);
-    edition::check(&library_path, &mut bad);
-
-    let collected = features::check(&src_path, &compiler_path, &library_path, &mut bad, verbose);
-    unstable_book::check(&src_path, collected, &mut bad);
-
-    // Checks that are done on the cargo workspace.
-    deps::check(&root_path, &cargo, &mut bad);
-    extdeps::check(&root_path, &mut bad);
-
-    if bad {
+    let bad = std::sync::Arc::new(AtomicBool::new(false));
+
+    scope(|s| {
+        let mut handles: VecDeque<ScopedJoinHandle<'_, ()>> =
+            VecDeque::with_capacity(concurrency.get());
+
+        macro_rules! check {
+            ($p:ident $(, $args:expr)* ) => {
+                while handles.len() >= concurrency.get() {
+                    handles.pop_front().unwrap().join().unwrap();
+                }
+
+                let handle = s.spawn(|_| {
+                    let mut flag = false;
+                    $p::check($($args),* , &mut flag);
+                    if (flag) {
+                        bad.store(true, Ordering::Relaxed);
+                    }
+                });
+                handles.push_back(handle);
+            }
+        }
+
+        // Checks that are done on the cargo workspace.
+        check!(deps, &root_path, &cargo);
+        check!(extdeps, &root_path);
+
+        // Checks over tests.
+        check!(debug_artifacts, &src_path);
+        check!(ui_tests, &src_path);
+
+        // Checks that only make sense for the compiler.
+        check!(errors, &compiler_path);
+        check!(error_codes_check, &src_path);
+
+        // Checks that only make sense for the std libs.
+        check!(pal, &library_path);
+
+        // Checks that need to be done for both the compiler and std libraries.
+        check!(unit_tests, &src_path);
+        check!(unit_tests, &compiler_path);
+        check!(unit_tests, &library_path);
+
+        if bins::check_filesystem_support(
+            &[&src_path, &compiler_path, &library_path],
+            &output_directory,
+        ) {
+            check!(bins, &src_path);
+            check!(bins, &compiler_path);
+            check!(bins, &library_path);
+        }
+
+        check!(style, &src_path);
+        check!(style, &compiler_path);
+        check!(style, &library_path);
+
+        check!(edition, &src_path);
+        check!(edition, &compiler_path);
+        check!(edition, &library_path);
+
+        let collected = {
+            while handles.len() >= concurrency.get() {
+                handles.pop_front().unwrap().join().unwrap();
+            }
+            let mut flag = false;
+            let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose);
+            if flag {
+                bad.store(true, Ordering::Relaxed);
+            }
+            r
+        };
+        check!(unstable_book, &src_path, collected);
+    })
+    .unwrap();
+
+    if bad.load(Ordering::Relaxed) {
         eprintln!("some tidy checks failed");
         process::exit(1);
     }