Merge pull request #1608 from Kobzol/runtime-compare-refactoring

Refactor `site` to make it easier to introduce runtime benchmarks

Author: Kobzol

collector/benchlib/src/benchmark.rs

@@ -110,12 +110,12 @@ impl BenchmarkGroup {
 pub fn passes_filter(name: &str, exclude: Option<&str>, include: Option<&str>) -> bool {
     match (exclude, include) {
         (Some(exclude), Some(include)) => {
-            let included = include.split(",").any(|filter| name.starts_with(filter));
-            let excluded = exclude.split(",").any(|filter| name.starts_with(filter));
+            let included = include.split(',').any(|filter| name.starts_with(filter));
+            let excluded = exclude.split(',').any(|filter| name.starts_with(filter));
             included && !excluded
         }
-        (None, Some(include)) => include.split(",").any(|filter| name.starts_with(filter)),
-        (Some(exclude), None) => !exclude.split(",").any(|filter| name.starts_with(filter)),
+        (None, Some(include)) => include.split(',').any(|filter| name.starts_with(filter)),
+        (Some(exclude), None) => !exclude.split(',').any(|filter| name.starts_with(filter)),
         (None, None) => true,
     }
 }

collector/src/benchmark/mod.rs

@@ -88,7 +88,7 @@ impl Benchmark {
             }
         }
 
-        let mut patches: Vec<_> = patches.into_iter().map(|p| Patch::new(p)).collect();
+        let mut patches: Vec<_> = patches.into_iter().map(Patch::new).collect();
         patches.sort_by_key(|p| p.index);
 
         let config_path = path.join("perf-config.json");
@@ -205,7 +205,7 @@ impl Benchmark {
         let iterations = iterations.unwrap_or(self.config.runs);
 
         let profiles: Vec<Profile> = profiles
-            .into_iter()
+            .iter()
             .copied()
             .filter(|profile| !self.config.excluded_profiles.contains(profile))
             .collect();
@@ -216,7 +216,7 @@ impl Benchmark {
         }
 
         let scenarios: Vec<Scenario> = scenarios
-            .into_iter()
+            .iter()
             .copied()
             .filter(|scenario| !self.config.excluded_scenarios.contains(scenario))
             .collect();
@@ -346,7 +346,7 @@ impl Benchmark {
                                     processor,
                                     Scenario::IncrPatched,
                                     &scenario_str,
-                                    Some(&patch),
+                                    Some(patch),
                                 )
                                 .run_rustc(true)?;
                         }

collector/src/benchmark/patch.rs

@@ -30,7 +30,7 @@ impl Patch {
         assert!(path.is_file());
         let (index, name) = {
             let file_name = path.file_name().unwrap().to_string_lossy();
-            let mut parts = file_name.split("-");
+            let mut parts = file_name.split('-');
             let index = parts.next().unwrap().parse().unwrap_or_else(|e| {
                 panic!(
                     "{:?} should be in the format 000-name.patch, \
@@ -61,7 +61,7 @@ impl Patch {
         log::debug!("applying {} to {:?}", self.name, dir);
 
         let mut cmd = Command::new("git");
-        cmd.current_dir(dir).args(&["apply"]).arg(&*self.path);
+        cmd.current_dir(dir).args(["apply"]).arg(&*self.path);
 
         command_output(&mut cmd)?;
 

collector/src/bin/bootstrap-rustc.rs

@@ -15,9 +15,9 @@ fn run() -> i32 {
     let status = cmd.status().expect("spawned");
 
     if status.success() {
-        return 0;
+        0
     } else {
-        return 1;
+        1
     }
 }
 

collector/src/bin/collector.rs

@@ -13,6 +13,7 @@ use collector::benchmark::{
 use collector::{runtime, utils, CollectorCtx, CollectorStepBuilder};
 use database::{ArtifactId, Commit, CommitType, Connection, Pool};
 use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator};
+use std::cmp::Ordering;
 use std::ffi::OsStr;
 use std::fs;
 use std::fs::File;
@@ -57,6 +58,7 @@ impl BenchmarkErrors {
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 fn bench(
     rt: &mut Runtime,
     mut conn: Box<dyn Connection>,
@@ -326,6 +328,7 @@ fn cg_annotate(cgout: &Path, path: &Path) -> anyhow::Result<()> {
     Ok(())
 }
 
+#[allow(clippy::too_many_arguments)]
 fn profile(
     compiler: Compiler,
     id: &str,
@@ -348,8 +351,7 @@ fn profile(
             let benchmark_id = format!("{} ({}/{})", benchmark.name, i + 1, benchmarks.len());
             eprintln!("Executing benchmark {benchmark_id}");
             let mut processor = ProfileProcessor::new(profiler, out_dir, id);
-            let result =
-                benchmark.measure(&mut processor, &profiles, &scenarios, compiler, Some(1));
+            let result = benchmark.measure(&mut processor, profiles, scenarios, compiler, Some(1));
             eprintln!("Finished benchmark {benchmark_id}");
 
             if let Err(ref s) = result {
@@ -400,7 +402,7 @@ impl TypedValueParser for ProfileArgParser {
             let profiles: Result<Vec<Profile>, _> = value
                 .to_str()
                 .unwrap()
-                .split(",")
+                .split(',')
                 .map(|item| clap::value_parser!(Profile).parse_ref(cmd, arg, OsStr::new(item)))
                 .collect();
 
@@ -410,7 +412,7 @@ impl TypedValueParser for ProfileArgParser {
 
     fn possible_values(&self) -> Option<Box<dyn Iterator<Item = PossibleValue> + '_>> {
         let values = Profile::value_variants()
-            .into_iter()
+            .iter()
             .filter_map(|item| item.to_possible_value())
             .chain([PossibleValue::new("All")]);
         Some(Box::new(values))
@@ -438,7 +440,7 @@ impl TypedValueParser for ScenarioArgParser {
             let scenarios: Result<Vec<Scenario>, _> = value
                 .to_str()
                 .unwrap()
-                .split(",")
+                .split(',')
                 .map(|item| clap::value_parser!(Scenario).parse_ref(cmd, arg, OsStr::new(item)))
                 .collect();
 
@@ -448,7 +450,7 @@ impl TypedValueParser for ScenarioArgParser {
 
     fn possible_values(&self) -> Option<Box<dyn Iterator<Item = PossibleValue> + '_>> {
         let values = Scenario::value_variants()
-            .into_iter()
+            .iter()
             .filter_map(|item| item.to_possible_value())
             .chain([PossibleValue::new("All")]);
         Some(Box::new(values))
@@ -711,7 +713,7 @@ fn main_result() -> anyhow::Result<i32> {
             let pool = Pool::open(&db.db);
 
             let suite = runtime::discover_benchmarks(&toolchain, &runtime_benchmark_dir)?;
-            let artifact_id = ArtifactId::Tag(toolchain.id.clone());
+            let artifact_id = ArtifactId::Tag(toolchain.id);
             let (conn, collector) = rt.block_on(async {
                 let mut conn = pool.connection().await;
                 let collector = CollectorStepBuilder::default()
@@ -744,7 +746,7 @@ fn main_result() -> anyhow::Result<i32> {
             let pool = database::Pool::open(&db.db);
 
             let toolchain = get_local_toolchain(
-                &profiles,
+                profiles,
                 &local.rustc,
                 opts.rustdoc.as_deref(),
                 local.cargo.as_deref(),
@@ -771,8 +773,8 @@ fn main_result() -> anyhow::Result<i32> {
             let res = bench(
                 &mut rt,
                 conn,
-                &profiles,
-                &scenarios,
+                profiles,
+                scenarios,
                 Compiler::from_toolchain(&toolchain, &target_triple),
                 &benchmarks,
                 Some(iterations),
@@ -792,7 +794,7 @@ fn main_result() -> anyhow::Result<i32> {
             println!("processing artifacts");
             let client = reqwest::blocking::Client::new();
             let response: collector::api::next_artifact::Response = client
-                .get(&format!("{}/perf/next_artifact", site_url))
+                .get(format!("{}/perf/next_artifact", site_url))
                 .send()?
                 .json()?;
             let next = if let Some(c) = response.artifact {
@@ -815,7 +817,7 @@ fn main_result() -> anyhow::Result<i32> {
                         &compile_benchmark_dir,
                     )?;
 
-                    client.post(&format!("{}/perf/onpush", site_url)).send()?;
+                    client.post(format!("{}/perf/onpush", site_url)).send()?;
                 }
                 NextArtifact::Commit {
                     commit,
@@ -853,7 +855,7 @@ fn main_result() -> anyhow::Result<i32> {
                         collector,
                     );
 
-                    client.post(&format!("{}/perf/onpush", site_url)).send()?;
+                    client.post(format!("{}/perf/onpush", site_url)).send()?;
 
                     res.fail_if_nonzero()?;
                 }
@@ -912,8 +914,8 @@ fn main_result() -> anyhow::Result<i32> {
             let mut get_toolchain_and_profile =
                 |rustc: &str, suffix: &str| -> anyhow::Result<String> {
                     let toolchain = get_local_toolchain(
-                        &profiles,
-                        &rustc,
+                        profiles,
+                        rustc,
                         opts.rustdoc.as_deref(),
                         local.cargo.as_deref(),
                         local.id.as_deref(),
@@ -926,8 +928,8 @@ fn main_result() -> anyhow::Result<i32> {
                         profiler,
                         &out_dir,
                         &benchmarks,
-                        &profiles,
-                        &scenarios,
+                        profiles,
+                        scenarios,
                         &mut errors,
                     );
                     Ok(toolchain.id)
@@ -950,20 +952,23 @@ fn main_result() -> anyhow::Result<i32> {
                         &id2,
                         &out_dir,
                         &benchmarks,
-                        &profiles,
-                        &scenarios,
+                        profiles,
+                        scenarios,
                         &mut errors,
                     );
-                    if diffs.len() > 1 {
-                        eprintln!("Diffs:");
-                        for diff in diffs {
-                            eprintln!("{}", diff.to_string_lossy());
+                    match diffs.len().cmp(&1) {
+                        Ordering::Equal => {
+                            let short = out_dir.join("cgann-diff-latest");
+                            std::fs::copy(&diffs[0], &short).expect("copy to short path");
+                            eprintln!("Original diff at: {}", diffs[0].to_string_lossy());
+                            eprintln!("Short path: {}", short.to_string_lossy());
+                        }
+                        _ => {
+                            eprintln!("Diffs:");
+                            for diff in diffs {
+                                eprintln!("{}", diff.to_string_lossy());
+                            }
                         }
-                    } else if diffs.len() == 1 {
-                        let short = out_dir.join("cgann-diff-latest");
-                        std::fs::copy(&diffs[0], &short).expect("copy to short path");
-                        eprintln!("Original diff at: {}", diffs[0].to_string_lossy());
-                        eprintln!("Short path: {}", short.to_string_lossy());
                     }
                 }
             } else {
@@ -983,8 +988,8 @@ fn main_result() -> anyhow::Result<i32> {
                 .unwrap();
             let last_sha = String::from_utf8(last_sha.stdout).expect("utf8");
             let last_sha = last_sha.split_whitespace().next().expect(&last_sha);
-            let commit = get_commit_or_fake_it(&last_sha).expect("success");
-            let mut sysroot = Sysroot::install(commit.sha.to_string(), &target_triple)?;
+            let commit = get_commit_or_fake_it(last_sha).expect("success");
+            let mut sysroot = Sysroot::install(commit.sha, &target_triple)?;
             sysroot.preserve(); // don't delete it
 
             // Print the directory containing the toolchain.
@@ -1021,7 +1026,7 @@ async fn init_compile_collector(
 ) -> (Box<dyn Connection>, CollectorCtx) {
     let mut conn = pool.connection().await;
     let collector = CollectorStepBuilder::default()
-        .record_compile_benchmarks(&benchmarks, bench_rustc)
+        .record_compile_benchmarks(benchmarks, bench_rustc)
         .start_collection(conn.as_mut(), artifact_id)
         .await;
     (conn, collector)
@@ -1035,7 +1040,7 @@ fn bench_published_artifact(
     benchmark_dir: &Path,
 ) -> anyhow::Result<()> {
     let status = Command::new("rustup")
-        .args(&["install", "--profile=minimal", &toolchain])
+        .args(["install", "--profile=minimal", &toolchain])
         .status()
         .context("rustup install")?;
     if !status.success() {
@@ -1071,7 +1076,7 @@ fn bench_published_artifact(
     let cargo = which("cargo")?;
 
     // Exclude benchmarks that don't work with a stable compiler.
-    let mut benchmarks = get_compile_benchmarks(&benchmark_dir, None, None, None)?;
+    let mut benchmarks = get_compile_benchmarks(benchmark_dir, None, None, None)?;
     benchmarks.retain(|b| b.category().is_stable());
 
     let artifact_id = ArtifactId::Tag(toolchain);
@@ -1091,7 +1096,7 @@ fn bench_published_artifact(
             rustdoc: Some(Path::new(rustdoc.trim())),
             cargo: Path::new(cargo.trim()),
             is_nightly: false,
-            triple: &target_triple,
+            triple: target_triple,
         },
         &benchmarks,
         Some(3),
@@ -1102,7 +1107,7 @@ fn bench_published_artifact(
     Ok(())
 }
 
-fn add_perf_config(directory: &PathBuf, category: Category) {
+fn add_perf_config(directory: &Path, category: Category) {
     let data = serde_json::json!({
         "category": category.to_string()
     });
@@ -1118,10 +1123,12 @@ fn add_perf_config(directory: &PathBuf, category: Category) {
 fn check_target_dir(target_dir: &Path, force: bool) -> anyhow::Result<()> {
     if target_dir.exists() {
         if force {
-            std::fs::remove_dir_all(&target_dir).expect(&format!(
-                "Cannot remove previous directory at {}",
-                target_dir.display()
-            ));
+            std::fs::remove_dir_all(target_dir).unwrap_or_else(|_| {
+                panic!(
+                    "Cannot remove previous directory at {}",
+                    target_dir.display()
+                )
+            });
         } else {
             return Err(anyhow::anyhow!(
                 "Directory {} already exists",
@@ -1138,9 +1145,9 @@ fn get_downloaded_crate_target(benchmark_dir: &Path, cmd: &DownloadCommand) -> P
         // URLs in general can end with /, which we also want to remove to make sure that the
         // last part of the URL is the repository name.
         DownloadSubcommand::Git { url } => url
-            .trim_end_matches("/")
+            .trim_end_matches('/')
             .trim_end_matches(".git")
-            .split("/")
+            .split('/')
             .last()
             .expect("Crate name could not be determined from git URL")
             .to_string(),
@@ -1168,7 +1175,7 @@ fn download_from_git(target: &Path, url: &str) -> anyhow::Result<()> {
         log::error!("Could not delete .git directory: {error:?}");
     }
 
-    utils::fs::rename(&tmpdir, &target)?;
+    utils::fs::rename(&tmpdir, target)?;
     Ok(())
 }
 
@@ -1198,7 +1205,7 @@ fn download_from_crates_io(target_dir: &Path, krate: &str, version: &str) -> any
     // under <crate-name>-<version> directory.
     let unpacked_dir = tmpdir.path().join(format!("{krate}-{version}"));
     generate_lockfile(&unpacked_dir);
-    utils::fs::rename(&unpacked_dir, &target_dir)?;
+    utils::fs::rename(&unpacked_dir, target_dir)?;
 
     Ok(())
 }
@@ -1235,7 +1242,7 @@ fn get_commit_or_fake_it(sha: &str) -> anyhow::Result<Commit> {
             log::warn!("utilizing fake commit!");
             Commit {
                 sha: sha.into(),
-                date: database::Date::ymd_hms(2000, 01, 01, 0, 0, 0),
+                date: database::Date::ymd_hms(2000, 1, 1, 0, 0, 0),
                 r#type: CommitType::Master,
             }
         }))