Address review comments

Author: Kobzol

collector/benchmarks/README.md

@@ -7,20 +7,33 @@ 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.)
 
-## Real programs that are important
+There are two categories of benchmarks, **Primary** and **Secondary**.
+
+## Primary
 
 These are real programs that are important in some way, and worth tracking.
+They mostly consist of real-world crates.
 
 - **cargo**: The Rust package manager. An important program in the Rust
   ecosystem.
 - **clap-rs**: A command line argument parser. A crate used by many Rust
   programs.
 - **cranelift-codegen**: The largest crate from a code generator. Used by
   Firefox.
+- **diesel**: A type save SQL query builder. Utilizes the type system to
+  ensure a lot of invariants. Stresses anything related to resolving
+  trait bounds, by having a lot of trait impls for a large number of different
+  types.
+- **encoding**: Character encoding support. Contains some large tables.
 - **futures**: A futures implementation. Used by many Rust programs.
 - **helloworld**: A trivial program. Gives a lower bound on compile time.
+- **html5ever**: An HTML parser. Stresses macro parsing code significantly.
 - **hyper-2**: A fairly large crate. Utilizes async/await, and used by
   many Rust programs.
+- **inflate**: An old implementation of the DEFLATE algorithm. Stresses the
+  compiler in certain ways.
+- **keccak**: A cryptography algorithm. Contains a very high number of locals
+  and basic blocks.
 - **piston-image**: A modular game engine. An interesting Rust program.
 - **regex**: A regular expression parser. Used by many Rust programs.
 - **ripgrep**: A line-oriented search tool. A widely-used utility.
@@ -33,40 +46,20 @@ These are real programs that are important in some way, and worth tracking.
   ecosystem.
 - **tokio-webpush-simple**: A simple web server built with tokio. Uses futures
   a lot.
-- **webrender**: A web renderer. Used by Firefox and Servo.
-- **webrender-wrench**: WebRender's test bench. An executable pulling in large
-  dependencies.
-
-## Real programs that stress the compiler
-
-These are real programs that are known to stress the compiler in interesting
-ways.
-
-- **diesel**: A type save SQL query builder. Utilizes the type system to
-  ensure a lot of invariants. Stresses anything related to resolving
-  trait bounds, by having a lot of trait impls for a large number of different
-  types.
-- **encoding**: Character encoding support. Contains some large tables.
-- **html5ever**: An HTML parser. Stresses macro parsing code significantly.
-- **inflate**: An old implementation of the DEFLATE algorithm. Stresses the
-  compiler in certain ways.
-- **keccak**: A cryptography algorithm. Contains a very high number of locals
-  and basic blocks.
 - **ucd**: A Unicode crate. Contains large statics that
   [stress](https://github.com/rust-lang/rust/issues/53643) the borrow checker's
   implementation of NLL.
 - **unicode_normalization**: Unicode character composition and decomposition
   utilities. Uses huge `match` statements that stress the compiler in unusual
   ways.
-- **wg-grammar**: A parser generator.
-  [Stresses](https://github.com/rust-lang/rust/issues/58178) the borrow
-  checker's implementation of NLL.
+- **webrender**: A web renderer. Used by Firefox and Servo.
+- **webrender-wrench**: WebRender's test bench. An executable pulling in large
+  dependencies.
 
-## Artificial stress tests
+## Secondary
 
-These are artificial programs that stress one particular aspect of the
-compiler. Some are entirely artificial, and some are extracted from real
-programs.
+These are either artificial programs or real crates that stress one particular aspect of the
+compiler in interesting ways.
 
 - **await-call-tree**: A tree of async fns that await each other, creating a
   large type composed of many repeated `impl Future` types. Such types caused
@@ -85,6 +78,8 @@ programs.
 - **deep-vector**: A test containing a single large vector of zeroes, which
   caused [poor performance](https://github.com/rust-lang/rust/issues/20936) in
   the past.
+- **derive**: A large amount of simple structs with a `#[derive]` attribute for common built-in traits such as Copy and Debug.
+- **externs**: A large amount of extern functions has caused [slowdowns in the past](https://github.com/rust-lang/rust/pull/78448).
 - **issue-46449**: A small program that caused [poor
   performance](https://github.com/rust-lang/rust/issues/46449) in the past.
 - **issue-58319**: A small program that caused [poor
@@ -103,7 +98,7 @@ programs.
 - **projection-caching**: A small program that causes extremely, deeply nested
   types which stress the trait system's projection cache. Removing that cache
   resulted in hours long compilations for some programs using futures,
-  actix-web and other libraries with similiarly nested type combinators.
+  actix-web and other libraries with similarly nested type combinators.
 - **regression-31157**: A small program that caused a [large performance
   regression](https://github.com/rust-lang/rust/issues/31157) from the past.
 - **token-stream-stress**: Constructs a long token stream much like the `quote`
@@ -122,5 +117,6 @@ programs.
 - **wf-projection-stress-65510**: A stress test which showcases [quadratic
   behavior](https://github.com/rust-lang/rust/issues/65510) (in the number of
   associated type bounds).
-- **externs**: A large amount of extern functions has caused [slowdowns in the past](https://github.com/rust-lang/rust/pull/78448).
-- **derive**: A large amount of simple structs with a `#[derive]` attribute for common built-in traits such as Copy and Debug.
+- **wg-grammar**: A parser generator.
+  [Stresses](https://github.com/rust-lang/rust/issues/58178) the borrow
+  checker's implementation of NLL.

collector/benchmarks/inflate/perf-config.json

@@ -1,3 +1,4 @@
 {
-    "supports_stable": true
+    "supports_stable": true,
+    "category": "primary"
 }

collector/benchmarks/rust-mozjs/perf-config.json renamed to collector/benchmarks/keccak/perf-config.json

@@ -1,4 +1,3 @@
 {
-    "touch_file": "src/lib.rs",
-    "category": "primary"
+    "touch_file": "src/lib.rs"
 }

collector/benchmarks/native-tls-0.1.5/perf-config.json

@@ -1,4 +1,5 @@
 {
     "supports_stable": true,
-    "touch_file": "src/main.rs"
+    "touch_file": "src/main.rs",
+    "category": "primary"
 }

collector/benchmarks/tokio-webpush-simple/perf-config.json

@@ -1,4 +1,3 @@
 {
-    "touch_file": "src/lib.rs",
-    "category": "primary"
+    "touch_file": "src/lib.rs"
 }

collector/benchmarks/wg-grammar/perf-config.json

@@ -112,17 +112,25 @@ fn default_runs() -> usize {
 
 #[derive(Debug, Clone, serde::Deserialize)]
 #[serde(rename_all = "lowercase")]
-enum BenchmarkCategory {
+enum Category {
     Primary,
-    Secondary
+    Secondary,
 }
 
-impl Default for BenchmarkCategory {
+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"),
+        }
+    }
+}
 /// This is the internal representation of an individual benchmark's
 /// perf-config.json file.
 #[derive(Debug, Clone, serde::Deserialize)]
@@ -145,7 +153,7 @@ struct BenchmarkConfig {
     touch_file: Option<String>,
 
     #[serde(default)]
-    category: BenchmarkCategory
+    category: Category,
 }
 
 impl Default for BenchmarkConfig {
@@ -158,7 +166,7 @@ impl Default for BenchmarkConfig {
             runs: default_runs(),
             supports_stable: false,
             touch_file: None,
-            category: BenchmarkCategory::Secondary
+            category: Category::Secondary,
         }
     }
 }
@@ -1256,11 +1264,8 @@ impl Benchmark {
         self.config.supports_stable
     }
 
-    pub fn category_name(&self) -> &str {
-        match self.config.category {
-            BenchmarkCategory::Primary => "primary",
-            BenchmarkCategory::Secondary => "secondary"
-        }
+    pub fn category(&self) -> &Category {
+        &self.config.category
     }
 
     #[cfg(windows)]

collector/src/execute.rs

@@ -199,10 +199,11 @@ fn bench(
                 return;
             }
             let mut tx = rt.block_on(conn.transaction());
-            rt.block_on(
-                tx.conn()
-                    .record_benchmark(&benchmark_name.0, Some(benchmark_supports_stable), benchmark_category),
-            );
+            rt.block_on(tx.conn().record_benchmark(
+                &benchmark_name.0,
+                Some(benchmark_supports_stable),
+                benchmark_category,
+            ));
             print_intro();
 
             let mut processor = BenchProcessor::new(
@@ -238,7 +239,7 @@ fn bench(
         measure_and_record(
             &benchmark.name,
             benchmark.supports_stable(),
-            benchmark.category_name(),
+            &benchmark.category().to_string(),
             &|| {
                 eprintln!(
                     "{}",

collector/src/main.rs

@@ -767,5 +767,5 @@ impl fmt::Display for CollectionId {
 #[derive(Debug, Clone, Serialize)]
 pub struct BenchmarkData {
     name: String,
-    category: String
+    category: String,
 }

database/src/lib.rs

@@ -1,5 +1,8 @@
 use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction};
-use crate::{ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkData, CollectionId, Commit, Date, Index, Profile, QueuedCommit, Scenario};
+use crate::{
+    ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkData, CollectionId, Commit, Date, Index,
+    Profile, QueuedCommit, Scenario,
+};
 use anyhow::Context as _;
 use chrono::{DateTime, TimeZone, Utc};
 use hashbrown::HashMap;
@@ -599,18 +602,16 @@ where
         }
     }
     async fn get_benchmarks(&self) -> Vec<BenchmarkData> {
-        let rows = self.conn()
+        let rows = self
+            .conn()
             .query(&self.statements().get_benchmarks, &[])
             .await
             .unwrap();
         rows.into_iter()
             .map(|r| {
                 let name: String = r.get(0);
                 let category: String = r.get(1);
-                BenchmarkData {
-                    name,
-                    category
-                }
+                BenchmarkData { name, category }
             })
             .collect()
     }
@@ -985,7 +986,12 @@ where
             .await
             .unwrap();
     }
-    async fn record_benchmark(&self, benchmark: &str, supports_stable: Option<bool>, category: &str) {
+    async fn record_benchmark(
+        &self,
+        benchmark: &str,
+        supports_stable: Option<bool>,
+        category: &str,
+    ) {
         if let Some(r) = self
             .conn()
             .query_opt(

database/src/pool/postgres.rs

@@ -506,11 +506,17 @@ impl Connection for SqliteConnection {
 
     async fn get_benchmarks(&self) -> Vec<BenchmarkData> {
         let conn = self.raw_ref();
-        let mut query = conn.prepare_cached("select name, category from benchmark").unwrap();
-        let rows = query.query_map([], |row| Ok(BenchmarkData {
-            name: row.get(0)?,
-            category: row.get(1)?
-        })).unwrap();
+        let mut query = conn
+            .prepare_cached("select name, category from benchmark")
+            .unwrap();
+        let rows = query
+            .query_map([], |row| {
+                Ok(BenchmarkData {
+                    name: row.get(0)?,
+                    category: row.get(1)?,
+                })
+            })
+            .unwrap();
         let mut benchmarks = Vec::new();
         for row in rows {
             benchmarks.push(row.unwrap());
@@ -878,7 +884,12 @@ impl Connection for SqliteConnection {
             )
             .unwrap();
     }
-    async fn record_benchmark(&self, benchmark: &str, supports_stable: Option<bool>, category: &str) {
+    async fn record_benchmark(
+        &self,
+        benchmark: &str,
+        supports_stable: Option<bool>,
+        category: &str,
+    ) {
         if let Some(stable) = supports_stable {
             self.raw_ref()
                 .execute(

database/src/pool/sqlite.rs

@@ -9,6 +9,7 @@ The following is a glossary of domain specific terminology. Although benchmarks
 * **scenario**: The scenario under which a user is compiling their code. Currently, this is the incremental cache state and an optional change in the source since last compilation (e.g., full incremental cache and a `println!` statement is added).  
 * **metric**: a name of a quantifiable metric being measured (e.g., instruction count)
 * **artifact**: a specific version of rustc (usually a commit sha or some sort of human readable "tag" like 1.51.0)
+* **category**: a high-level group of benchmarks. Currently, there are two categories, primary (mostly real-world crates) and secondary (mostly stress tests).
 
 ## Benchmarks
 

docs/glossary.md

@@ -137,7 +137,7 @@ pub async fn handle_compare(
         new_errors,
         next,
         is_contiguous,
-        benchmark_data: benchmark_map
+        benchmark_data: benchmark_map,
     })
 }