Rollup merge of #108772 - jyn514:faster-tidy, r=the8472

Speed up tidy quite a lot

I highly recommend reviewing this commit-by-commit. Based on #106440 for convenience.

## Timings

These were collected by running `x test tidy -v` to copy paste the command, then using [`samply record`](https://github.com/mstange/samply).

before (8 threads)
![image](https://user-images.githubusercontent.com/23638587/222965319-352ad2c8-367c-4d74-960a-e4bb161a6aff.png)

after (8 threads) ![image](https://user-images.githubusercontent.com/23638587/222965323-fa846f4e-727a-4bf8-8e3b-1b7b40505cc3.png)

before (64 threads) ![image](https://user-images.githubusercontent.com/23638587/222965302-dc88020c-19e9-49d9-a87d-cad054d717f3.png)
after (64 threads) ![image](https://user-images.githubusercontent.com/23638587/222965335-e73d7622-59de-41d2-9cc4-1bd67042a349.png)

The last commit makes tidy use more threads, so comparing "before (8 threads)" to "after (64 threads)" is IMO the most realistic comparison. Locally, that brings the time for me to run tidy down from 4 to .9 seconds, i.e. the majority of the time for `x test tidy` is now spend running `fmt --check`.

r? `@the8472`
This commit is contained in:
Matthias Krüger
2023-03-18 12:04:21 +01:00
committed by GitHub
13 changed files with 227 additions and 189 deletions
+1
View File
@@ -42,6 +42,7 @@ no_llvm_build
/llvm/
/mingw-build/
build/
!/compiler/rustc_mir_build/src/build/
/build-rust-analyzer/
/dist/
/unicode-downloads
@@ -73,19 +73,34 @@ pub(crate) fn as_rvalue(
}
ExprKind::Binary { op, lhs, rhs } => {
let lhs = unpack!(
block =
this.as_operand(block, scope, &this.thir[lhs], LocalInfo::Boring, NeedsTemporary::Maybe)
block = this.as_operand(
block,
scope,
&this.thir[lhs],
LocalInfo::Boring,
NeedsTemporary::Maybe
)
);
let rhs = unpack!(
block =
this.as_operand(block, scope, &this.thir[rhs], LocalInfo::Boring, NeedsTemporary::No)
block = this.as_operand(
block,
scope,
&this.thir[rhs],
LocalInfo::Boring,
NeedsTemporary::No
)
);
this.build_binary_op(block, op, expr_span, expr.ty, lhs, rhs)
}
ExprKind::Unary { op, arg } => {
let arg = unpack!(
block =
this.as_operand(block, scope, &this.thir[arg], LocalInfo::Boring, NeedsTemporary::No)
block = this.as_operand(
block,
scope,
&this.thir[arg],
LocalInfo::Boring,
NeedsTemporary::No
)
);
// Check for -MIN on signed integers
if this.check_overflow && op == UnOp::Neg && expr.ty.is_signed() {
@@ -272,8 +287,13 @@ pub(crate) fn as_rvalue(
}
ExprKind::Pointer { cast, source } => {
let source = unpack!(
block =
this.as_operand(block, scope, &this.thir[source], LocalInfo::Boring, NeedsTemporary::No)
block = this.as_operand(
block,
scope,
&this.thir[source],
LocalInfo::Boring,
NeedsTemporary::No
)
);
block.and(Rvalue::Cast(CastKind::Pointer(cast), source, expr.ty))
}
@@ -502,8 +522,10 @@ pub(crate) fn as_rvalue(
Category::of(&expr.kind),
Some(Category::Rvalue(RvalueFunc::AsRvalue) | Category::Constant)
));
let operand =
unpack!(block = this.as_operand(block, scope, expr, LocalInfo::Boring, NeedsTemporary::No));
let operand = unpack!(
block =
this.as_operand(block, scope, expr, LocalInfo::Boring, NeedsTemporary::No)
);
block.and(Rvalue::Use(operand))
}
}
@@ -662,8 +684,9 @@ fn build_zero_repeat(
// Repeating a const does nothing
} else {
// For a non-const, we may need to generate an appropriate `Drop`
let value_operand =
unpack!(block = this.as_operand(block, scope, value, LocalInfo::Boring, NeedsTemporary::No));
let value_operand = unpack!(
block = this.as_operand(block, scope, value, LocalInfo::Boring, NeedsTemporary::No)
);
if let Operand::Move(to_drop) = value_operand {
let success = this.cfg.start_new_block();
this.cfg.terminate(
@@ -2252,7 +2252,9 @@ fn declare_binding(
user_ty: None,
source_info,
internal: false,
local_info: ClearCrossCrate::Set(Box::new(LocalInfo::User(BindingForm::RefForGuard))),
local_info: ClearCrossCrate::Set(Box::new(LocalInfo::User(
BindingForm::RefForGuard,
))),
});
self.var_debug_info.push(VarDebugInfo {
name,
+8 -11
View File
@@ -876,21 +876,18 @@ fn args_and_body(
} => {
self.local_decls[local].mutability = mutability;
self.local_decls[local].source_info.scope = self.source_scope;
**self.local_decls[local].local_info.as_mut().assert_crate_local() = if let Some(kind) = param.self_kind {
LocalInfo::User(
BindingForm::ImplicitSelf(kind),
)
} else {
let binding_mode = ty::BindingMode::BindByValue(mutability);
LocalInfo::User(BindingForm::Var(
VarBindingForm {
**self.local_decls[local].local_info.as_mut().assert_crate_local() =
if let Some(kind) = param.self_kind {
LocalInfo::User(BindingForm::ImplicitSelf(kind))
} else {
let binding_mode = ty::BindingMode::BindByValue(mutability);
LocalInfo::User(BindingForm::Var(VarBindingForm {
binding_mode,
opt_ty_info: param.ty_span,
opt_match_place: Some((None, span)),
pat_span: span,
},
))
};
}))
};
self.var_indices.insert(var, LocalsForNode::One(local));
}
_ => {
+5 -1
View File
@@ -1118,7 +1118,11 @@ fn run(self, builder: &Builder<'_>) {
cmd.arg(&builder.src);
cmd.arg(&builder.initial_cargo);
cmd.arg(&builder.out);
cmd.arg(builder.jobs().to_string());
// Tidy is heavily IO constrained. Still respect `-j`, but use a higher limit if `jobs` hasn't been configured.
let jobs = builder.config.jobs.unwrap_or_else(|| {
8 * std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get) as u32
});
cmd.arg(jobs.to_string());
if builder.is_verbose() {
cmd.arg("--verbose");
}
+1 -1
View File
@@ -103,7 +103,7 @@ pub fn check(path: &Path, bad: &mut bool) {
// FIXME: we don't need to look at all binaries, only files that have been modified in this branch
// (e.g. using `git ls-files`).
walk_no_read(path, |path| filter_dirs(path) || path.ends_with("src/etc"), &mut |entry| {
walk_no_read(&[path], |path| filter_dirs(path) || path.ends_with("src/etc"), &mut |entry| {
let file = entry.path();
let extension = file.extension();
let scripts = ["py", "sh", "ps1"];
+3 -9
View File
@@ -1,21 +1,15 @@
//! Tidy check to prevent creation of unnecessary debug artifacts while running tests.
use crate::walk::{filter_dirs, walk};
use crate::walk::{filter_dirs, filter_not_rust, walk};
use std::path::Path;
const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in test";
pub fn check(test_dir: &Path, bad: &mut bool) {
walk(test_dir, filter_dirs, &mut |entry, contents| {
let filename = entry.path();
let is_rust = filename.extension().map_or(false, |ext| ext == "rs");
if !is_rust {
return;
}
walk(test_dir, |path| filter_dirs(path) || filter_not_rust(path), &mut |entry, contents| {
for (i, line) in contents.lines().enumerate() {
if line.contains("borrowck_graphviz_postflow") {
tidy_error!(bad, "{}:{}: {}", filename.display(), i + 1, GRAPHVIZ_POSTFLOW_MSG);
tidy_error!(bad, "{}:{}: {}", entry.path().display(), i + 1, GRAPHVIZ_POSTFLOW_MSG);
}
}
});
+8 -9
View File
@@ -9,8 +9,9 @@
//! * All unstable lang features have tests to ensure they are actually unstable.
//! * Language features in a group are sorted by feature name.
use crate::walk::{filter_dirs, walk, walk_many};
use crate::walk::{filter_dirs, filter_not_rust, walk, walk_many};
use std::collections::hash_map::{Entry, HashMap};
use std::ffi::OsStr;
use std::fmt;
use std::fs;
use std::num::NonZeroU32;
@@ -101,17 +102,15 @@ pub fn check(
&tests_path.join("rustdoc-ui"),
&tests_path.join("rustdoc"),
],
filter_dirs,
|path| {
filter_dirs(path)
|| filter_not_rust(path)
|| path.file_name() == Some(OsStr::new("features.rs"))
|| path.file_name() == Some(OsStr::new("diagnostic_list.rs"))
},
&mut |entry, contents| {
let file = entry.path();
let filename = file.file_name().unwrap().to_string_lossy();
if !filename.ends_with(".rs")
|| filename == "features.rs"
|| filename == "diagnostic_list.rs"
{
return;
}
let filen_underscore = filename.replace('-', "_").replace(".rs", "");
let filename_is_gate_test = test_filen_gate(&filen_underscore, &mut features);
+16 -5
View File
@@ -13,7 +13,7 @@
use std::process;
use std::str::FromStr;
use std::sync::atomic::{AtomicBool, Ordering};
use std::thread::{scope, ScopedJoinHandle};
use std::thread::{self, scope, ScopedJoinHandle};
fn main() {
let root_path: PathBuf = env::args_os().nth(1).expect("need path to root of repo").into();
@@ -55,16 +55,28 @@ fn main() {
VecDeque::with_capacity(concurrency.get());
macro_rules! check {
($p:ident $(, $args:expr)* ) => {
($p:ident) => {
check!(@ $p, name=format!("{}", stringify!($p)));
};
($p:ident, $path:expr $(, $args:expr)* ) => {
let shortened = $path.strip_prefix(&root_path).unwrap();
let name = if shortened == std::path::Path::new("") {
format!("{} (.)", stringify!($p))
} else {
format!("{} ({})", stringify!($p), shortened.display())
};
check!(@ $p, name=name, $path $(,$args)*);
};
(@ $p:ident, name=$name:expr $(, $args:expr)* ) => {
drain_handles(&mut handles);
let handle = s.spawn(|| {
let handle = thread::Builder::new().name($name).spawn_scoped(s, || {
let mut flag = false;
$p::check($($args, )* &mut flag);
if (flag) {
bad.store(true, Ordering::Relaxed);
}
});
}).unwrap();
handles.push_back(handle);
}
}
@@ -108,7 +120,6 @@ macro_rules! check {
check!(edition, &library_path);
check!(alphabetical, &src_path);
check!(alphabetical, &tests_path);
check!(alphabetical, &compiler_path);
check!(alphabetical, &library_path);
+19 -12
View File
@@ -19,7 +19,7 @@
use crate::walk::{filter_dirs, walk};
use regex::{Regex, RegexSet};
use std::path::Path;
use std::{ffi::OsStr, path::Path};
/// Error code markdown is restricted to 80 columns because they can be
/// displayed on the console with --example.
@@ -228,21 +228,33 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool {
pub fn check(path: &Path, bad: &mut bool) {
fn skip(path: &Path) -> bool {
filter_dirs(path) || skip_markdown_path(path)
if path.file_name().map_or(false, |name| name.to_string_lossy().starts_with(".#")) {
// vim or emacs temporary file
return true;
}
if filter_dirs(path) || skip_markdown_path(path) {
return true;
}
let extensions = ["rs", "py", "js", "sh", "c", "cpp", "h", "md", "css", "ftl", "goml"];
if extensions.iter().all(|e| path.extension() != Some(OsStr::new(e))) {
return true;
}
// We only check CSS files in rustdoc.
path.extension().map_or(false, |e| e == "css") && !is_in(path, "src", "librustdoc")
}
let problematic_consts_strings: Vec<String> = (PROBLEMATIC_CONSTS.iter().map(u32::to_string))
.chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:x}", v)))
.chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:X}", v)))
.collect();
let problematic_regex = RegexSet::new(problematic_consts_strings.as_slice()).unwrap();
walk(path, skip, &mut |entry, contents| {
let file = entry.path();
let filename = file.file_name().unwrap().to_string_lossy();
let extensions =
[".rs", ".py", ".js", ".sh", ".c", ".cpp", ".h", ".md", ".css", ".ftl", ".goml"];
if extensions.iter().all(|e| !filename.ends_with(e)) || filename.starts_with(".#") {
return;
}
let is_style_file = filename.ends_with(".css");
let under_rustfmt = filename.ends_with(".rs") &&
@@ -253,11 +265,6 @@ fn skip(path: &Path) -> bool {
a.ends_with("src/doc/book")
});
if is_style_file && !is_in(file, "src", "librustdoc") {
// We only check CSS files in rustdoc.
return;
}
if contents.is_empty() {
tidy_error!(bad, "{}: empty file", file.display());
}
+49 -51
View File
@@ -4,6 +4,8 @@
use std::collections::BTreeMap;
use std::path::Path;
use crate::walk::filter_not_rust;
const COMMENT: &str = "//";
const LLVM_COMPONENTS_HEADER: &str = "needs-llvm-components:";
const COMPILE_FLAGS_HEADER: &str = "compile-flags:";
@@ -35,61 +37,57 @@ struct RevisionInfo<'a> {
}
pub fn check(path: &Path, bad: &mut bool) {
crate::walk::walk(
path,
|path| path.extension().map(|p| p == "rs") == Some(false),
&mut |entry, content| {
let file = entry.path().display();
let mut header_map = BTreeMap::new();
iter_header(content, &mut |cfg, directive| {
if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) {
let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
let comp_vec = info.llvm_components.get_or_insert(Vec::new());
for component in value.split(' ') {
let component = component.trim();
if !component.is_empty() {
comp_vec.push(component);
}
}
} else if directive.starts_with(COMPILE_FLAGS_HEADER) {
let compile_flags = &directive[COMPILE_FLAGS_HEADER.len()..];
if let Some((_, v)) = compile_flags.split_once("--target") {
if let Some((arch, _)) =
v.trim_start_matches(|c| c == ' ' || c == '=').split_once("-")
{
let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
info.target_arch.replace(arch);
} else {
eprintln!("{file}: seems to have a malformed --target value");
*bad = true;
}
crate::walk::walk(path, filter_not_rust, &mut |entry, content| {
let file = entry.path().display();
let mut header_map = BTreeMap::new();
iter_header(content, &mut |cfg, directive| {
if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) {
let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
let comp_vec = info.llvm_components.get_or_insert(Vec::new());
for component in value.split(' ') {
let component = component.trim();
if !component.is_empty() {
comp_vec.push(component);
}
}
});
for (rev, RevisionInfo { target_arch, llvm_components }) in &header_map {
let rev = rev.unwrap_or("[unspecified]");
match (target_arch, llvm_components) {
(None, None) => {}
(Some(_), None) => {
eprintln!(
"{}: revision {} should specify `{}` as it has `--target` set",
file, rev, LLVM_COMPONENTS_HEADER
);
} else if directive.starts_with(COMPILE_FLAGS_HEADER) {
let compile_flags = &directive[COMPILE_FLAGS_HEADER.len()..];
if let Some((_, v)) = compile_flags.split_once("--target") {
if let Some((arch, _)) =
v.trim_start_matches(|c| c == ' ' || c == '=').split_once("-")
{
let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
info.target_arch.replace(arch);
} else {
eprintln!("{file}: seems to have a malformed --target value");
*bad = true;
}
(None, Some(_)) => {
eprintln!(
"{}: revision {} should not specify `{}` as it doesn't need `--target`",
file, rev, LLVM_COMPONENTS_HEADER
);
*bad = true;
}
(Some(_), Some(_)) => {
// FIXME: check specified components against the target architectures we
// gathered.
}
}
}
},
);
});
for (rev, RevisionInfo { target_arch, llvm_components }) in &header_map {
let rev = rev.unwrap_or("[unspecified]");
match (target_arch, llvm_components) {
(None, None) => {}
(Some(_), None) => {
eprintln!(
"{}: revision {} should specify `{}` as it has `--target` set",
file, rev, LLVM_COMPONENTS_HEADER
);
*bad = true;
}
(None, Some(_)) => {
eprintln!(
"{}: revision {} should not specify `{}` as it doesn't need `--target`",
file, rev, LLVM_COMPONENTS_HEADER
);
*bad = true;
}
(Some(_), Some(_)) => {
// FIXME: check specified components against the target architectures we
// gathered.
}
}
}
});
}
+59 -63
View File
@@ -3,87 +3,83 @@
//! - there are no stray `.stderr` files
use ignore::Walk;
use ignore::WalkBuilder;
use std::collections::HashMap;
use std::fs;
use std::path::Path;
use std::path::{Path, PathBuf};
const ENTRY_LIMIT: usize = 1000;
// FIXME: The following limits should be reduced eventually.
const ROOT_ENTRY_LIMIT: usize = 940;
const ISSUES_ENTRY_LIMIT: usize = 1978;
fn check_entries(path: &Path, bad: &mut bool) {
for dir in Walk::new(&path.join("ui")) {
fn check_entries(tests_path: &Path, bad: &mut bool) {
let mut directories: HashMap<PathBuf, usize> = HashMap::new();
for dir in Walk::new(&tests_path.join("ui")) {
if let Ok(entry) = dir {
if entry.file_type().map(|ft| ft.is_dir()).unwrap_or(false) {
let dir_path = entry.path();
// Use special values for these dirs.
let is_root = path.join("ui") == dir_path;
let is_issues_dir = path.join("ui/issues") == dir_path;
let limit = if is_root {
ROOT_ENTRY_LIMIT
} else if is_issues_dir {
ISSUES_ENTRY_LIMIT
} else {
ENTRY_LIMIT
};
let parent = entry.path().parent().unwrap().to_path_buf();
*directories.entry(parent).or_default() += 1;
}
}
let count = WalkBuilder::new(&dir_path)
.max_depth(Some(1))
.build()
.into_iter()
.collect::<Vec<_>>()
.len()
- 1; // remove the dir itself
for (dir_path, count) in directories {
// Use special values for these dirs.
let is_root = tests_path.join("ui") == dir_path;
let is_issues_dir = tests_path.join("ui/issues") == dir_path;
let limit = if is_root {
ROOT_ENTRY_LIMIT
} else if is_issues_dir {
ISSUES_ENTRY_LIMIT
} else {
ENTRY_LIMIT
};
if count > limit {
tidy_error!(
bad,
"following path contains more than {} entries, \
you should move the test to some relevant subdirectory (current: {}): {}",
limit,
count,
dir_path.display()
);
}
}
if count > limit {
tidy_error!(
bad,
"following path contains more than {} entries, \
you should move the test to some relevant subdirectory (current: {}): {}",
limit,
count,
dir_path.display()
);
}
}
}
pub fn check(path: &Path, bad: &mut bool) {
check_entries(&path, bad);
for path in &[&path.join("ui"), &path.join("ui-fulldeps")] {
crate::walk::walk_no_read(path, |_| false, &mut |entry| {
let file_path = entry.path();
if let Some(ext) = file_path.extension() {
if ext == "stderr" || ext == "stdout" {
// Test output filenames have one of the formats:
// ```
// $testname.stderr
// $testname.$mode.stderr
// $testname.$revision.stderr
// $testname.$revision.$mode.stderr
// ```
//
// For now, just make sure that there is a corresponding
// `$testname.rs` file.
//
// NB: We do not use file_stem() as some file names have multiple `.`s and we
// must strip all of them.
let testname =
file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;
if !file_path.with_file_name(testname).with_extension("rs").exists() {
tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path);
}
let (ui, ui_fulldeps) = (path.join("ui"), path.join("ui-fulldeps"));
let paths = [ui.as_path(), ui_fulldeps.as_path()];
crate::walk::walk_no_read(&paths, |_| false, &mut |entry| {
let file_path = entry.path();
if let Some(ext) = file_path.extension() {
if ext == "stderr" || ext == "stdout" {
// Test output filenames have one of the formats:
// ```
// $testname.stderr
// $testname.$mode.stderr
// $testname.$revision.stderr
// $testname.$revision.$mode.stderr
// ```
//
// For now, just make sure that there is a corresponding
// `$testname.rs` file.
//
// NB: We do not use file_stem() as some file names have multiple `.`s and we
// must strip all of them.
let testname =
file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;
if !file_path.with_file_name(testname).with_extension("rs").exists() {
tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path);
}
if let Ok(metadata) = fs::metadata(file_path) {
if metadata.len() == 0 {
tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path);
}
if let Ok(metadata) = fs::metadata(file_path) {
if metadata.len() == 0 {
tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path);
}
}
}
});
}
}
});
}
+20 -14
View File
@@ -1,6 +1,6 @@
use ignore::DirEntry;
use std::{fs::File, io::Read, path::Path};
use std::{ffi::OsStr, fs::File, io::Read, path::Path};
/// The default directory filter.
pub fn filter_dirs(path: &Path) -> bool {
@@ -33,23 +33,26 @@ pub fn filter_dirs(path: &Path) -> bool {
skip.iter().any(|p| path.ends_with(p))
}
/// Filter for only files that end in `.rs`.
pub fn filter_not_rust(path: &Path) -> bool {
path.extension() != Some(OsStr::new("rs")) && !path.is_dir()
}
pub fn walk(
path: &Path,
skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool,
f: &mut dyn FnMut(&DirEntry, &str),
) {
walk_many(&[path], skip, f);
}
pub fn walk_many(
paths: &[&Path],
skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool,
f: &mut dyn FnMut(&DirEntry, &str),
) {
for path in paths {
walk(path, skip.clone(), f);
}
}
pub fn walk(
path: &Path,
skip: impl Send + Sync + 'static + Fn(&Path) -> bool,
f: &mut dyn FnMut(&DirEntry, &str),
) {
let mut contents = Vec::new();
walk_no_read(path, skip, &mut |entry| {
walk_no_read(paths, skip, &mut |entry| {
contents.clear();
let mut file = t!(File::open(entry.path()), entry.path());
t!(file.read_to_end(&mut contents), entry.path());
@@ -62,11 +65,14 @@ pub fn walk(
}
pub(crate) fn walk_no_read(
path: &Path,
paths: &[&Path],
skip: impl Send + Sync + 'static + Fn(&Path) -> bool,
f: &mut dyn FnMut(&DirEntry),
) {
let mut walker = ignore::WalkBuilder::new(path);
let mut walker = ignore::WalkBuilder::new(paths[0]);
for path in &paths[1..] {
walker.add(path);
}
let walker = walker.filter_entry(move |e| !skip(e.path()));
for entry in walker.build() {
if let Ok(entry) = entry {