Overhaul how the rustc benchmark works.

Currently the `rustc` benchmark is handled like a normal benchmark in
some ways, and handled very differently in others. This results in
various non-obvious special cases.

This commit improves things.
- Adds a `--bench-rustc` option to `bench_local` and `bench_next`, which
  makes the rustc benchmark opt-in.
- Omits the rustc benchmark from `--include`/`--exclude` handling.
- Calls it the "special rustc benchmark", in contrast to the "normal
  benchmarks".
- Handles it directly in `bench()`, avoiding a bunch of awkward
  special-casing code.
- Adds an `eprintln!` to make it more obvious when it starts running.
- Cleans up some unnecessary string conversions in `bench()`.

Author: nnethercote

.github/workflows/ci.yml

@@ -64,8 +64,8 @@ jobs:
     strategy:
       matrix:
         BENCH_INCLUDE_EXCLUDE_OPTS: [
-          "--exclude webrender-wrench,style-servo,cargo,rustc",
-          "--include webrender-wrench,style-servo,cargo --exclude rustc",
+          "--exclude webrender-wrench,style-servo,cargo",
+          "--include webrender-wrench,style-servo,cargo",
         ]
         PROFILE_KINDS: [
           "Check,Doc,Debug",

collector/README.md

@@ -107,6 +107,12 @@ The following options alter the behaviour of the `bench_local` subcommand.
   comma-separated list of benchmark names. When this option is specified, a
   benchmark is included in the run only if its name matches one or more of the
   given names.
+- `--bench-rustc`: there is a special `rustc` benchmark that involves
+  downloading a recent Rust compiler and measuring the time taken to compile
+  it. This benchmark works very differently to all the other benchmarks. For
+  example, `--runs` and `--builds` don't affect it, and the given `ID` is used
+  as the `rust-lang/rust` ref (falling back to `HEAD` if the `ID` is not a
+  valid ref). It is for advanced and CI use only. This option enables it.
 - `--runs $RUNS`: the run kinds to be benchmarked. The possible choices are one
   or more (comma-separated) of `Full`, `IncrFull`, `IncrUnchanged`,
   `IncrPatched`, and `All`. The default is `All`. Note that `IncrFull` is

collector/collect.sh

@@ -26,6 +26,6 @@ while : ; do
         rm todo-artifacts
         touch todo-artifacts
 
-        target/release/collector bench_next $SITE_URL --self-profile --db $DATABASE;
+        target/release/collector bench_next $SITE_URL --self-profile --bench-rustc --db $DATABASE;
         echo finished run at `date`;
 done

collector/src/execute.rs

@@ -571,10 +571,6 @@ pub trait Processor {
     fn finished_first_collection(&mut self, _: ProfileKind) -> bool {
         false
     }
-
-    fn measure_rustc(&mut self, _: Compiler<'_>) -> anyhow::Result<()> {
-        Ok(())
-    }
 }
 
 pub struct BenchProcessor<'a> {
@@ -722,6 +718,16 @@ impl<'a> BenchProcessor<'a> {
         self.rt
             .block_on(async move { while let Some(()) = buf.next().await {} });
     }
+
+    pub fn measure_rustc(&mut self, compiler: Compiler<'_>) -> anyhow::Result<()> {
+        rustc::measure(
+            self.rt,
+            self.conn,
+            compiler,
+            self.artifact,
+            self.artifact_row_id,
+        )
+    }
 }
 
 struct Upload(std::process::Child, tempfile::NamedTempFile);
@@ -897,16 +903,6 @@ impl<'a> Processor for BenchProcessor<'a> {
             }
         }
     }
-
-    fn measure_rustc(&mut self, compiler: Compiler<'_>) -> anyhow::Result<()> {
-        rustc::measure(
-            self.rt,
-            self.conn,
-            compiler,
-            self.artifact,
-            self.artifact_row_id,
-        )
-    }
 }
 
 pub struct ProfileProcessor<'a> {
@@ -1231,15 +1227,6 @@ impl<'a> Processor for ProfileProcessor<'a> {
 
 impl Benchmark {
     pub fn new(name: String, path: PathBuf) -> anyhow::Result<Self> {
-        if name == "rustc" {
-            return Ok(Benchmark {
-                name: BenchmarkName(name),
-                path,
-                patches: vec![],
-                config: BenchmarkConfig::default(),
-            });
-        }
-
         let mut patches = vec![];
         for entry in fs::read_dir(&path)? {
             let entry = entry?;
@@ -1359,10 +1346,6 @@ impl Benchmark {
     ) -> anyhow::Result<()> {
         let iterations = iterations.unwrap_or(self.config.runs);
 
-        if self.name.0 == "rustc" {
-            return processor.measure_rustc(compiler).context("measure rustc");
-        }
-
         if self.config.disabled || profile_kinds.is_empty() {
             eprintln!("Skipping {}: disabled", self.name);
             bail!("disabled benchmark");

collector/src/execute/rustc.rs

@@ -15,13 +15,16 @@ use std::{collections::HashMap, process::Command};
 use std::{path::Path, time::Duration};
 use tokio::runtime::Runtime;
 
+/// Measure the special rustc benchmark.
 pub fn measure(
     rt: &mut Runtime,
     conn: &mut dyn database::Connection,
     compiler: Compiler<'_>,
     artifact: &database::ArtifactId,
     aid: database::ArtifactIdNumber,
 ) -> anyhow::Result<()> {
+    eprintln!("Running rustc");
+
     checkout(&artifact).context("checking out rust-lang/rust")?;
 
     record(rt, conn, compiler, artifact, aid)?;

collector/src/main.rs

@@ -183,9 +183,9 @@ where
     Ok(v)
 }
 
-fn n_benchmarks_remaining(n: usize) -> String {
+fn n_normal_benchmarks_remaining(n: usize) -> String {
     let suffix = if n == 1 { "" } else { "s" };
-    format!("{} benchmark{} remaining", n, suffix)
+    format!("{} normal benchmark{} remaining", n, suffix)
 }
 
 struct BenchmarkErrors(usize);
@@ -213,6 +213,7 @@ fn bench(
     artifact_id: &ArtifactId,
     profile_kinds: &[ProfileKind],
     scenario_kinds: &[ScenarioKind],
+    bench_rustc: bool,
     compiler: Compiler<'_>,
     benchmarks: &[Benchmark],
     iterations: Option<usize>,
@@ -231,10 +232,13 @@ fn bench(
         }
     }
 
-    let steps = benchmarks
+    let mut steps = benchmarks
         .iter()
         .map(|b| b.name.to_string())
         .collect::<Vec<_>>();
+    if bench_rustc {
+        steps.push("rustc".to_string());
+    }
 
     // Make sure there is no observable time when the artifact ID is available
     // but the in-progress steps are not.
@@ -249,57 +253,87 @@ fn bench(
 
     let start = Instant::now();
     let mut skipped = false;
-    for (nth_benchmark, benchmark) in benchmarks.iter().enumerate() {
-        let is_fresh =
-            rt.block_on(conn.collector_start_step(artifact_row_id, &benchmark.name.to_string()));
+
+    let mut measure_and_record = |benchmark_name: &execute::BenchmarkName,
+                                  benchmark_supports_stable: bool,
+                                  print_intro: &dyn Fn(),
+                                  measure: &dyn Fn(
+        &mut execute::BenchProcessor,
+    ) -> Result<(), anyhow::Error>| {
+        let is_fresh = rt.block_on(conn.collector_start_step(artifact_row_id, &benchmark_name.0));
         if !is_fresh {
             skipped = true;
-            eprintln!("skipping {} -- already benchmarked", benchmark.name);
-            continue;
+            eprintln!("skipping {} -- already benchmarked", benchmark_name);
+            return;
         }
         let mut tx = rt.block_on(conn.transaction());
         rt.block_on(
             tx.conn()
-                .record_benchmark(benchmark.name.0.as_str(), Some(benchmark.supports_stable())),
-        );
-        eprintln!(
-            "{}",
-            n_benchmarks_remaining(benchmarks.len() - nth_benchmark)
+                .record_benchmark(&benchmark_name.0, Some(benchmark_supports_stable)),
         );
+        print_intro();
 
         let mut processor = execute::BenchProcessor::new(
             rt,
             tx.conn(),
-            &benchmark.name,
+            benchmark_name,
             &artifact_id,
             artifact_row_id,
             is_self_profile,
         );
-        let result = benchmark.measure(
-            &mut processor,
-            profile_kinds,
-            scenario_kinds,
-            compiler,
-            iterations,
-        );
+        let result = measure(&mut processor);
         if let Err(s) = result {
             eprintln!(
                 "collector error: Failed to benchmark '{}', recorded: {:#}",
-                benchmark.name, s
+                benchmark_name, s
             );
             errors.incr();
             rt.block_on(tx.conn().record_error(
                 artifact_row_id,
-                benchmark.name.0.as_str(),
+                &benchmark_name.0,
                 &format!("{:?}", s),
             ));
         };
         rt.block_on(
             tx.conn()
-                .collector_end_step(artifact_row_id, &benchmark.name.to_string()),
+                .collector_end_step(artifact_row_id, &benchmark_name.0),
         );
         rt.block_on(tx.commit()).expect("committed");
+    };
+
+    // Normal benchmarks.
+    for (nth_benchmark, benchmark) in benchmarks.iter().enumerate() {
+        measure_and_record(
+            &benchmark.name,
+            benchmark.supports_stable(),
+            &|| {
+                eprintln!(
+                    "{}",
+                    n_normal_benchmarks_remaining(benchmarks.len() - nth_benchmark)
+                )
+            },
+            &|processor| {
+                benchmark.measure(
+                    processor,
+                    profile_kinds,
+                    scenario_kinds,
+                    compiler,
+                    iterations,
+                )
+            },
+        )
     }
+
+    // The special rustc benchmark, if requested.
+    if bench_rustc {
+        measure_and_record(
+            &execute::BenchmarkName("rustc".to_string()),
+            /* supports_stable */ false,
+            &|| eprintln!("Special benchmark commencing (due to `--bench-rustc`)"),
+            &|processor| processor.measure_rustc(compiler).context("measure rustc"),
+        );
+    }
+
     let end = start.elapsed();
 
     eprintln!(
@@ -373,7 +407,6 @@ fn get_benchmarks(
 
         paths.push((path, name));
     }
-    paths.push((PathBuf::from("rustc"), String::from("rustc")));
 
     let mut includes = include.map(|list| list.split(',').collect::<HashSet<_>>());
     let mut excludes = exclude.map(|list| list.split(',').collect::<HashSet<_>>());
@@ -683,7 +716,7 @@ fn profile(
         check_measureme_installed().unwrap();
     }
     for (i, benchmark) in benchmarks.iter().enumerate() {
-        eprintln!("{}", n_benchmarks_remaining(benchmarks.len() - i));
+        eprintln!("{}", n_normal_benchmarks_remaining(benchmarks.len() - i));
         let mut processor = execute::ProfileProcessor::new(profiler, out_dir, id);
         let result = benchmark.measure(
             &mut processor,
@@ -742,6 +775,8 @@ fn main_result() -> anyhow::Result<i32> {
             (@arg INCLUDE: --include     +takes_value
              "Include only benchmarks that are listed in\n\
              this comma-separated list of patterns")
+            (@arg BENCH_RUSTC: --("bench-rustc")
+             "Run the special `rustc` benchmark")
             (@arg RUNS:    --runs    +takes_value
              "One or more (comma-separated) of: 'Full',\n\
              'IncrFull', 'IncrUnchanged', 'IncrPatched', 'All'")
@@ -752,13 +787,15 @@ fn main_result() -> anyhow::Result<i32> {
         )
 
         (@subcommand bench_next =>
-            (about: "Benchmarks the next commit for perf.rust-lang.org")
+            (about: "Benchmarks the next commit for perf.rust-lang.org, including the special `rustc` benchmark")
 
             // Mandatory arguments
             (@arg SITE_URL: +required +takes_value "Site URL")
 
             // Options
             (@arg DB:           --db  +takes_value "Database output file")
+            (@arg BENCH_RUSTC: --("bench-rustc")
+             "Run the special `rustc` benchmark")
             (@arg SELF_PROFILE: --("self-profile") "Collect self-profile data")
         )
 
@@ -868,6 +905,7 @@ fn main_result() -> anyhow::Result<i32> {
             let db = sub_m.value_of("DB").unwrap_or(default_db);
             let exclude = sub_m.value_of("EXCLUDE");
             let include = sub_m.value_of("INCLUDE");
+            let bench_rustc = sub_m.is_present("BENCH_RUSTC");
             let scenario_kinds = scenario_kinds_from_arg(sub_m.value_of("RUNS"))?;
             let iterations = iterations_from_arg(sub_m.value_of("ITERATIONS"))?;
             let rustdoc = sub_m.value_of("RUSTDOC");
@@ -886,6 +924,7 @@ fn main_result() -> anyhow::Result<i32> {
                 &ArtifactId::Tag(id.to_string()),
                 &profile_kinds,
                 &scenario_kinds,
+                bench_rustc,
                 Compiler {
                     rustc: &rustc,
                     rustdoc: rustdoc.as_deref(),
@@ -907,6 +946,7 @@ fn main_result() -> anyhow::Result<i32> {
 
             // Options
             let db = sub_m.value_of("DB").unwrap_or(default_db);
+            let bench_rustc = sub_m.is_present("BENCH_RUSTC");
             let is_self_profile = sub_m.is_present("SELF_PROFILE");
 
             println!("processing commits");
@@ -941,6 +981,7 @@ fn main_result() -> anyhow::Result<i32> {
                 &ArtifactId::Commit(commit),
                 &ProfileKind::all(),
                 &ScenarioKind::all(),
+                bench_rustc,
                 Compiler::from_sysroot(&sysroot),
                 &benchmarks,
                 next.runs.map(|v| v as usize),
@@ -1011,6 +1052,7 @@ fn main_result() -> anyhow::Result<i32> {
                 &ArtifactId::Tag(toolchain.to_string()),
                 &proile_kinds,
                 &scenario_kinds,
+                /* bench_rustc */ false,
                 Compiler {
                     rustc: Path::new(rustc.trim()),
                     rustdoc: Some(Path::new(rustdoc.trim())),