Introduce the "stable" benchmark category.

nnethercote · nnethercote · commit 4009f1e8bf3e · 2022-03-16T10:57:15.000+11:00
(Also, temporarily, the "primary-and-stable" category. All the current
"primary-and-stable" benchmarks will become "stable" in the near
future.)

These categories are used in `perf-config.json` and the code that
chooses which benchmarks to execute. They allow the `supports_stable`
field to be removed.

However, when it comes to storing the category in the database, to avoid
database format changes, `Category` gets split into `supports_stable` (a
bool) and `category` (either "primary" or "secondary").

Some notable details:
- `Category` is moved to a different file, because it's no longer used
  by the DB code.
- Some methods are moved from `BenchmarkConfig` to `Category`.
- `Category` now derives `clap::ArgEnum`, which improves the `download`
  command's help message by listing the possible values.
diff --git a/collector/benchmarks/README.md b/collector/benchmarks/README.md
@@ -7,7 +7,8 @@ The suite changes over time. Sometimes the code for a benchmark is updated, in
 which case a small suffix will be added (starting with "-2", then "-3", and so
 on.)
 
-There are two categories of benchmarks, **Primary** and **Secondary**.
+There are three categories of benchmarks, **Primary**, **Secondary**, and
+**Stable**.
 
 ## Primary
 
@@ -120,3 +121,21 @@ compiler in interesting ways.
 - **wg-grammar**: A parser generator.
   [Stresses](https://github.com/rust-lang/rust/issues/58178) the borrow
   checker's implementation of NLL.
+
+**Stable**
+
+These are benchmarks used in the
+[dashboard](https://perf.rust-lang.org/dashboard.html). They provide the
+longest continuous data set for compiler performance. As a result, they are
+quite old (e.g. 2017 or earlier), and not necessarily reflective of typical
+Rust code being written today.
+
+- **encoding**: See above.
+- **futures**: See above.
+- **html5ever**: See above.
+- **inflate**: See above.
+- **regex**: See above.
+- **piston-image**: See above.
+- **style-servo**: See above.
+- **syn**: See above.
+- **tokio-webpush-simple**: See above.
diff --git a/collector/benchmarks/encoding/perf-config.json b/collector/benchmarks/encoding/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": true,
     "touch_file": "src/lib.rs",
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/futures/perf-config.json b/collector/benchmarks/futures/perf-config.json
@@ -1,4 +1,3 @@
 {
-    "supports_stable": true,
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/html5ever/perf-config.json b/collector/benchmarks/html5ever/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": true,
     "touch_file": "src/lib.rs",
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/inflate/perf-config.json b/collector/benchmarks/inflate/perf-config.json
@@ -1,4 +1,3 @@
 {
-    "supports_stable": true,
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/match-stress-exhaustive_patterns/perf-config.json b/collector/benchmarks/match-stress-exhaustive_patterns/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": false,
     "touch_file": "src/lib.rs",
     "category": "secondary"
 }
diff --git a/collector/benchmarks/piston-image/perf-config.json b/collector/benchmarks/piston-image/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": true,
     "runs": 1,
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/regex/perf-config.json b/collector/benchmarks/regex/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": true,
     "touch_file": "src/lib.rs",
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/style-servo/perf-config.json b/collector/benchmarks/style-servo/perf-config.json
@@ -3,7 +3,6 @@
     "cargo_rustc_opts": "--cap-lints=warn",
     "cargo_toml": "components/style/Cargo.toml",
     "runs": 1,
-    "supports_stable": true,
     "touch_file": "components/style/lib.rs",
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/syn/perf-config.json b/collector/benchmarks/syn/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": true,
     "touch_file": "src/lib.rs",
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/benchmarks/tokio-webpush-simple/perf-config.json b/collector/benchmarks/tokio-webpush-simple/perf-config.json
@@ -1,5 +1,4 @@
 {
-    "supports_stable": true,
     "touch_file": "src/main.rs",
-    "category": "primary"
+    "category": "primary-and-stable"
 }
diff --git a/collector/src/execute.rs b/collector/src/execute.rs
@@ -4,9 +4,10 @@ use crate::{Compiler, Profile, Scenario};
 use anyhow::{bail, Context};
 use collector::command_output;
 use collector::etw_parser;
-use database::{Category, PatchName, QueryLabel};
+use database::{PatchName, QueryLabel};
 use futures::stream::FuturesUnordered;
 use futures::stream::StreamExt;
+use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 use std::env;
 use std::fmt;
@@ -106,6 +107,57 @@ fn touch_all(path: &Path) -> anyhow::Result<()> {
     Ok(())
 }
 
+#[derive(Debug, Clone, Copy, Serialize, Deserialize, clap::ArgEnum)]
+#[serde(rename_all = "kebab-case")]
+pub enum Category {
+    Primary,
+    Secondary,
+    // FIXME: this should disappear after the 2022 benchmark overhaul is
+    // complete.
+    PrimaryAndStable,
+    Stable,
+}
+
+impl Category {
+    pub fn has_stable(&self) -> bool {
+        match self {
+            Category::Primary | Category::Secondary => false,
+            Category::PrimaryAndStable | Category::Stable => true,
+        }
+    }
+
+    pub fn is_primary_or_secondary(&self) -> bool {
+        match self {
+            Category::Primary | Category::Secondary | Category::PrimaryAndStable => true,
+            Category::Stable => false,
+        }
+    }
+
+    // Within the DB, `Category` is represented in two fields:
+    // - a `supports_stable` bool,
+    // - a `category` which is either "primary" or "secondary".
+    pub fn db_representation(&self) -> (bool, String) {
+        match self {
+            Category::Primary => (false, "primary".to_string()),
+            Category::Secondary => (false, "secondary".to_string()),
+            // These two have the same DB representation, even though they are
+            // treated differently when choosing which benchmarks to run.
+            Category::PrimaryAndStable | Category::Stable => (true, "primary".to_string()),
+        }
+    }
+}
+
+impl fmt::Display for Category {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Category::Primary => f.write_str("primary"),
+            Category::Secondary => f.write_str("secondary"),
+            Category::PrimaryAndStable => f.write_str("primary-and-stable"),
+            Category::Stable => f.write_str("stable"),
+        }
+    }
+}
+
 fn default_runs() -> usize {
     3
 }
@@ -121,8 +173,6 @@ struct BenchmarkConfig {
     disabled: bool,
     #[serde(default = "default_runs")]
     runs: usize,
-    #[serde(default)]
-    supports_stable: bool,
 
     /// The file that should be touched to ensure cargo re-checks the leaf crate
     /// we're interested in. Likely, something similar to `src/lib.rs`. The
@@ -1236,12 +1286,8 @@ impl Benchmark {
         })
     }
 
-    pub fn supports_stable(&self) -> bool {
-        self.config.supports_stable
-    }
-
-    pub fn category(&self) -> &Category {
-        &self.config.category
+    pub fn category(&self) -> Category {
+        self.config.category
     }
 
     #[cfg(windows)]
diff --git a/collector/src/main.rs b/collector/src/main.rs
@@ -2,7 +2,7 @@
 
 use anyhow::{bail, Context};
 use clap::Parser;
-use database::{ArtifactId, Category, Commit};
+use database::{ArtifactId, Commit};
 use log::debug;
 use std::collections::HashSet;
 use std::fs;
@@ -18,7 +18,7 @@ use tokio::runtime::Runtime;
 mod execute;
 mod sysroot;
 
-use execute::{BenchProcessor, Benchmark, BenchmarkName, ProfileProcessor, Profiler};
+use execute::{BenchProcessor, Benchmark, BenchmarkName, Category, ProfileProcessor, Profiler};
 use sysroot::Sysroot;
 
 #[derive(Debug, Copy, Clone)]
@@ -190,8 +190,7 @@ fn bench(
 
     let mut measure_and_record =
         |benchmark_name: &BenchmarkName,
-         benchmark_supports_stable: bool,
-         benchmark_category: Category,
+         category: Category,
          print_intro: &dyn Fn(),
          measure: &dyn Fn(&mut BenchProcessor) -> anyhow::Result<()>| {
             let is_fresh =
@@ -202,10 +201,11 @@ fn bench(
                 return;
             }
             let mut tx = rt.block_on(conn.transaction());
+            let (supports_stable, category) = category.db_representation();
             rt.block_on(tx.conn().record_benchmark(
                 &benchmark_name.0,
-                Some(benchmark_supports_stable),
-                benchmark_category,
+                Some(supports_stable),
+                category,
             ));
             print_intro();
 
@@ -241,8 +241,7 @@ fn bench(
     for (nth_benchmark, benchmark) in benchmarks.iter().enumerate() {
         measure_and_record(
             &benchmark.name,
-            benchmark.supports_stable(),
-            benchmark.category().clone(),
+            benchmark.category(),
             &|| {
                 eprintln!(
                     "{}",
@@ -257,7 +256,6 @@ fn bench(
     if bench_rustc {
         measure_and_record(
             &BenchmarkName("rustc".to_string()),
-            /* supports_stable */ false,
             Category::Primary,
             &|| eprintln!("Special benchmark commencing (due to `--bench-rustc`)"),
             &|processor| processor.measure_rustc(compiler).context("measure rustc"),
@@ -860,12 +858,12 @@ struct DownloadCommand {
     #[clap(long, global = true)]
     name: Option<String>,
 
-    /// Overwrite the benchmark directory if it already exists.
+    /// Overwrite the benchmark directory if it already exists
     #[clap(long, short('f'), global = true)]
     force: bool,
 
-    /// What category does the benchmark belong to (primary or secondary).
-    #[clap(long, short('c'), global = true, default_value = "secondary")]
+    /// What category does the benchmark belong to
+    #[clap(long, short('c'), arg_enum, global = true, default_value = "secondary")]
     category: Category,
 
     #[clap(subcommand)]
@@ -924,11 +922,12 @@ fn main_result() -> anyhow::Result<i32> {
                 "",
             )?;
 
-            let benchmarks = get_benchmarks(
+            let mut benchmarks = get_benchmarks(
                 &benchmark_dir,
                 local.include.as_deref(),
                 local.exclude.as_deref(),
             )?;
+            benchmarks.retain(|b| b.category().is_primary_or_secondary());
 
             let res = bench(
                 &mut rt,
@@ -978,11 +977,12 @@ fn main_result() -> anyhow::Result<i32> {
             let sysroot = Sysroot::install(commit.sha.to_string(), &target_triple)
                 .with_context(|| format!("failed to install sysroot for {:?}", commit))?;
 
-            let benchmarks = get_benchmarks(
+            let mut benchmarks = get_benchmarks(
                 &benchmark_dir,
                 next.include.as_deref(),
                 next.exclude.as_deref(),
             )?;
+            benchmarks.retain(|b| b.category().is_primary_or_secondary());
 
             let res = bench(
                 &mut rt,
@@ -1044,7 +1044,7 @@ fn main_result() -> anyhow::Result<i32> {
 
             // Exclude benchmarks that don't work with a stable compiler.
             let mut benchmarks = get_benchmarks(&benchmark_dir, None, None)?;
-            benchmarks.retain(|b| b.supports_stable());
+            benchmarks.retain(|b| b.category().has_stable());
 
             let res = bench(
                 &mut rt,
@@ -1077,11 +1077,13 @@ fn main_result() -> anyhow::Result<i32> {
             let profiles = Profile::expand_all(&local.profiles);
             let scenarios = Scenario::expand_all(&local.scenarios);
 
-            let benchmarks = get_benchmarks(
+            let mut benchmarks = get_benchmarks(
                 &benchmark_dir,
                 local.include.as_deref(),
                 local.exclude.as_deref(),
             )?;
+            benchmarks.retain(|b| b.category().is_primary_or_secondary());
+
             let mut errors = BenchmarkErrors::new();
 
             let mut get_toolchain_and_profile =
diff --git a/database/src/lib.rs b/database/src/lib.rs
@@ -767,38 +767,5 @@ impl fmt::Display for CollectionId {
 #[derive(Debug, Clone, Serialize)]
 pub struct BenchmarkData {
     pub name: String,
-    pub category: Category,
-}
-
-#[derive(Debug, Clone, Serialize, Deserialize)]
-#[serde(rename_all = "lowercase")]
-pub enum Category {
-    Primary,
-    Secondary,
-}
-
-impl Default for Category {
-    fn default() -> Self {
-        Self::Secondary
-    }
-}
-
-impl fmt::Display for Category {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Category::Primary => f.write_str("primary"),
-            Category::Secondary => f.write_str("secondary"),
-        }
-    }
-}
-impl std::str::FromStr for Category {
-    type Err = anyhow::Error;
-
-    fn from_str(input: &str) -> Result<Self, Self::Err> {
-        match input {
-            "primary" => Ok(Category::Primary),
-            "secondary" => Ok(Category::Secondary),
-            _ => Err(anyhow::anyhow!("Invalid category {input}")),
-        }
-    }
+    pub category: String,
 }
diff --git a/database/src/pool.rs b/database/src/pool.rs
@@ -1,4 +1,4 @@
-use crate::{ArtifactId, ArtifactIdNumber, BenchmarkData, Category};
+use crate::{ArtifactId, ArtifactIdNumber, BenchmarkData};
 use crate::{CollectionId, Index, Profile, QueryDatum, QueuedCommit, Scenario, Step};
 use chrono::{DateTime, Utc};
 use hashbrown::HashMap;
@@ -27,12 +27,7 @@ pub trait Connection: Send + Sync {
     async fn artifact_id(&self, artifact: &ArtifactId) -> ArtifactIdNumber;
     /// None means that the caller doesn't know; it should be left alone if
     /// known or set to false if unknown.
-    async fn record_benchmark(
-        &self,
-        krate: &str,
-        supports_stable: Option<bool>,
-        category: Category,
-    );
+    async fn record_benchmark(&self, krate: &str, supports_stable: Option<bool>, category: String);
     async fn record_statistic(
         &self,
         collection: CollectionId,
diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs
diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs
diff --git a/docs/glossary.md b/docs/glossary.md

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`{`
`2`		`- "supports_stable": true,`
`3`	`2`	`"touch_file": "src/lib.rs",`
`4`		`- "category": "primary"`
	`3`	`+ "category": "primary-and-stable"`
`5`	`4`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,3 @@`
`1`	`1`	`{`
`2`		`- "supports_stable": true,`
`3`		`- "category": "primary"`
	`2`	`+ "category": "primary-and-stable"`
`4`	`3`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`{`
`2`		`- "supports_stable": false,`
`3`	`2`	`"touch_file": "src/lib.rs",`
`4`	`3`	`"category": "secondary"`
`5`	`4`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,4 @@`
`1`	`1`	`{`
`2`		`- "supports_stable": true,`
`3`	`2`	`"runs": 1,`
`4`		`- "category": "primary"`
	`3`	`+ "category": "primary-and-stable"`
`5`	`4`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,6 @@`
`3`	`3`	`"cargo_rustc_opts": "--cap-lints=warn",`
`4`	`4`	`"cargo_toml": "components/style/Cargo.toml",`
`5`	`5`	`"runs": 1,`
`6`		`- "supports_stable": true,`
`7`	`6`	`"touch_file": "components/style/lib.rs",`
`8`		`- "category": "primary"`
	`7`	`+ "category": "primary-and-stable"`
`9`	`8`	`}`