Skip to content

Commit

Permalink
Rollup merge of rust-lang#105829 - the8472:tidy-style, r=jyn514
Browse files Browse the repository at this point in the history 
Speed up tidy

This can be reviewed commit by commit since they contain separate optimizations.

```
# master
$ taskset -c 0-5 hyperfine './x test tidy'
Benchmark #1: ./x test tidy
  Time (mean ± σ):      4.857 s ±  0.064 s    [User: 12.967 s, System: 2.014 s]
  Range (min … max):    4.779 s …  4.997 s    10 runs

# PR
$ taskset -c 0-5 hyperfine './x test tidy'
Benchmark #1: ./x test tidy
  Time (mean ± σ):      3.672 s ±  0.035 s    [User: 10.524 s, System: 2.029 s]
  Range (min … max):    3.610 s …  3.725 s    10 runs
```
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Dec 17, 2022
2 parents eaf2f26 + ab7d769 commit cf08eda
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 34 deletions.
23 changes: 19 additions & 4 deletions src/bootstrap/format.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};
use std::sync::mpsc::SyncSender;

fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut() {
fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut(bool) -> bool {
let mut cmd = Command::new(&rustfmt);
// avoid the submodule config paths from coming into play,
// we only allow a single global config for the workspace for now
Expand All @@ -23,7 +23,13 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
let cmd_debug = format!("{:?}", cmd);
let mut cmd = cmd.spawn().expect("running rustfmt");
// poor man's async: return a closure that'll wait for rustfmt's completion
move || {
move |block: bool| -> bool {
if !block {
match cmd.try_wait() {
Ok(Some(_)) => {}
_ => return false,
}
}
let status = cmd.wait().unwrap();
if !status.success() {
eprintln!(
Expand All @@ -34,6 +40,7 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
);
crate::detail_exit(1);
}
true
}
}

Expand Down Expand Up @@ -146,15 +153,23 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
let child = rustfmt(&src, &rustfmt_path, paths.as_slice(), check);
children.push_back(child);

// poll completion before waiting
for i in (0..children.len()).rev() {
if children[i](false) {
children.swap_remove_back(i);
break;
}
}

if children.len() >= max_processes {
// await oldest child
children.pop_front().unwrap()();
children.pop_front().unwrap()(true);
}
}

// await remaining children
for mut child in children {
child();
child(true);
}
});

Expand Down
22 changes: 16 additions & 6 deletions src/tools/tidy/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,26 @@ fn main() {

let bad = std::sync::Arc::new(AtomicBool::new(false));

let drain_handles = |handles: &mut VecDeque<ScopedJoinHandle<'_, ()>>| {
// poll all threads for completion before awaiting the oldest one
for i in (0..handles.len()).rev() {
if handles[i].is_finished() {
handles.swap_remove_back(i).unwrap().join().unwrap();
}
}

while handles.len() >= concurrency.get() {
handles.pop_front().unwrap().join().unwrap();
}
};

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();
}
drain_handles(&mut handles);

let handle = s.spawn(|| {
let mut flag = false;
Expand Down Expand Up @@ -97,9 +108,8 @@ fn main() {
check!(alphabetical, &library_path);

let collected = {
while handles.len() >= concurrency.get() {
handles.pop_front().unwrap().join().unwrap();
}
drain_handles(&mut handles);

let mut flag = false;
let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose);
if flag {
Expand Down
59 changes: 35 additions & 24 deletions src/tools/tidy/src/style.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
//! `// ignore-tidy-CHECK-NAME`.
use crate::walk::{filter_dirs, walk};
use regex::Regex;
use regex::{Regex, RegexSet};
use std::path::Path;

/// Error code markdown is restricted to 80 columns because they can be
Expand Down Expand Up @@ -225,6 +225,7 @@ pub fn check(path: &Path, bad: &mut bool) {
.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, &mut skip, &mut |entry, contents| {
let file = entry.path();
let filename = file.file_name().unwrap().to_string_lossy();
Expand Down Expand Up @@ -281,7 +282,27 @@ pub fn check(path: &Path, bad: &mut bool) {
let mut trailing_new_lines = 0;
let mut lines = 0;
let mut last_safety_comment = false;
let is_test = file.components().any(|c| c.as_os_str() == "tests");
// scanning the whole file for multiple needles at once is more efficient than
// executing lines times needles separate searches.
let any_problematic_line = problematic_regex.is_match(contents);
for (i, line) in contents.split('\n').enumerate() {
if line.is_empty() {
if i == 0 {
leading_new_lines = true;
}
trailing_new_lines += 1;
continue;
} else {
trailing_new_lines = 0;
}

let trimmed = line.trim();

if !trimmed.starts_with("//") {
lines += 1;
}

let mut err = |msg: &str| {
tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
};
Expand All @@ -308,36 +329,38 @@ pub fn check(path: &Path, bad: &mut bool) {
suppressible_tidy_err!(err, skip_cr, "CR character");
}
if filename != "style.rs" {
if line.contains("TODO") {
if trimmed.contains("TODO") {
err("TODO is deprecated; use FIXME")
}
if line.contains("//") && line.contains(" XXX") {
if trimmed.contains("//") && trimmed.contains(" XXX") {
err("XXX is deprecated; use FIXME")
}
for s in problematic_consts_strings.iter() {
if line.contains(s) {
err("Don't use magic numbers that spell things (consider 0x12345678)");
if any_problematic_line {
for s in problematic_consts_strings.iter() {
if trimmed.contains(s) {
err("Don't use magic numbers that spell things (consider 0x12345678)");
}
}
}
}
let is_test = || file.components().any(|c| c.as_os_str() == "tests");
// for now we just check libcore
if line.contains("unsafe {") && !line.trim().starts_with("//") && !last_safety_comment {
if file.components().any(|c| c.as_os_str() == "core") && !is_test() {
if trimmed.contains("unsafe {") && !trimmed.starts_with("//") && !last_safety_comment {
if file.components().any(|c| c.as_os_str() == "core") && !is_test {
suppressible_tidy_err!(err, skip_undocumented_unsafe, "undocumented unsafe");
}
}
if line.contains("// SAFETY:") {
if trimmed.contains("// SAFETY:") {
last_safety_comment = true;
} else if line.trim().starts_with("//") || line.trim().is_empty() {
} else if trimmed.starts_with("//") || trimmed.is_empty() {
// keep previous value
} else {
last_safety_comment = false;
}
if (line.starts_with("// Copyright")
|| line.starts_with("# Copyright")
|| line.starts_with("Copyright"))
&& (line.contains("Rust Developers") || line.contains("Rust Project Developers"))
&& (trimmed.contains("Rust Developers")
|| trimmed.contains("Rust Project Developers"))
{
suppressible_tidy_err!(
err,
Expand All @@ -351,18 +374,6 @@ pub fn check(path: &Path, bad: &mut bool) {
if filename.ends_with(".cpp") && line.contains("llvm_unreachable") {
err(LLVM_UNREACHABLE_INFO);
}
if line.is_empty() {
if i == 0 {
leading_new_lines = true;
}
trailing_new_lines += 1;
} else {
trailing_new_lines = 0;
}

if !line.trim().starts_with("//") {
lines += 1;
}
}
if leading_new_lines {
let mut err = |_| {
Expand Down

0 comments on commit cf08eda

Please sign in to comment.