Address review comments

Author: Kobzol

collector/benchlib/src/benchmark.rs

@@ -5,8 +5,8 @@ use crate::measure::benchmark_function;
 use crate::process::raise_process_priority;
 use std::collections::HashMap;
 
-/// Create a new benchmark group. Use the closure argument to define benchmarks.
-pub fn benchmark_group<F: FnOnce(&mut BenchmarkGroup)>(define_func: F) {
+/// Create a new benchmark group. Use the closure argument to define individual benchmarks.
+pub fn run_benchmark_group<F: FnOnce(&mut BenchmarkGroup)>(define_func: F) {
     env_logger::init();
 
     let mut group = BenchmarkGroup::new();
@@ -19,11 +19,9 @@ struct BenchmarkWrapper {
     func: Box<dyn Fn() -> anyhow::Result<BenchmarkStats>>,
 }
 
-type BenchmarkMap = HashMap<&'static str, BenchmarkWrapper>;
-
 #[derive(Default)]
 pub struct BenchmarkGroup {
-    benchmarks: BenchmarkMap,
+    benchmarks: HashMap<&'static str, BenchmarkWrapper>,
 }
 
 impl BenchmarkGroup {
@@ -32,7 +30,7 @@ impl BenchmarkGroup {
     }
 
     /// Registers a single benchmark.
-    /// `func` should return a closure that will be benchmarked.
+    /// `constructor` should return a closure that will be benchmarked.
     pub fn register<F: Fn() -> Bench + Clone + 'static, R, Bench: FnOnce() -> R + 'static>(
         &mut self,
         name: &'static str,
@@ -55,49 +53,52 @@ impl BenchmarkGroup {
 
         let args = parse_cli()?;
         match args {
-            Args::Benchmark(args) => {
-                run_benchmark(args, self.benchmarks)?;
+            Args::Run(args) => {
+                self.run_benchmarks(args)?;
             }
-            Args::ListBenchmarks => list_benchmarks(self.benchmarks)?,
+            Args::List => self.list_benchmarks()?,
         }
 
         Ok(())
     }
-}
-
-fn list_benchmarks(benchmarks: BenchmarkMap) -> anyhow::Result<()> {
-    let benchmark_list: Vec<&str> = benchmarks.into_keys().collect();
-    serde_json::to_writer(std::io::stdout(), &benchmark_list)?;
 
-    Ok(())
-}
-
-fn run_benchmark(args: BenchmarkArgs, benchmarks: BenchmarkMap) -> anyhow::Result<()> {
-    let mut items: Vec<(&'static str, BenchmarkWrapper)> = benchmarks
-        .into_iter()
-        .filter(|(name, _)| passes_filter(name, args.exclude.as_deref(), args.include.as_deref()))
-        .collect();
-    items.sort_unstable_by_key(|item| item.0);
-
-    let mut stdout = std::io::stdout().lock();
-
-    for (name, def) in items {
-        let mut stats: Vec<BenchmarkStats> = Vec::with_capacity(args.iterations as usize);
-        for i in 0..args.iterations {
-            let benchmark_stats = (def.func)()?;
-            log::info!("Benchmark (run {i}) `{name}` completed: {benchmark_stats:?}");
-            stats.push(benchmark_stats);
+    fn run_benchmarks(self, args: BenchmarkArgs) -> anyhow::Result<()> {
+        let mut items: Vec<(&'static str, BenchmarkWrapper)> = self
+            .benchmarks
+            .into_iter()
+            .filter(|(name, _)| {
+                passes_filter(name, args.exclude.as_deref(), args.include.as_deref())
+            })
+            .collect();
+        items.sort_unstable_by_key(|item| item.0);
+
+        let mut stdout = std::io::stdout().lock();
+
+        for (name, def) in items {
+            let mut stats: Vec<BenchmarkStats> = Vec::with_capacity(args.iterations as usize);
+            for i in 0..args.iterations {
+                let benchmark_stats = (def.func)()?;
+                log::info!("Benchmark (run {i}) `{name}` completed: {benchmark_stats:?}");
+                stats.push(benchmark_stats);
+            }
+            output_message(
+                &mut stdout,
+                BenchmarkMessage::Result(BenchmarkResult {
+                    name: name.to_string(),
+                    stats,
+                }),
+            )?;
         }
-        output_message(
-            &mut stdout,
-            BenchmarkMessage::Result(BenchmarkResult {
-                name: name.to_string(),
-                stats,
-            }),
-        )?;
+
+        Ok(())
     }
 
-    Ok(())
+    fn list_benchmarks(self) -> anyhow::Result<()> {
+        let benchmark_list: Vec<&str> = self.benchmarks.into_keys().collect();
+        serde_json::to_writer(std::io::stdout(), &benchmark_list)?;
+
+        Ok(())
+    }
 }
 
 /// Adds a single benchmark to the benchmark group.

collector/benchlib/src/cli.rs

@@ -3,9 +3,9 @@ use clap::{FromArgMatches, IntoApp};
 #[derive(clap::Parser, Debug)]
 pub enum Args {
     /// Benchmark all benchmarks in this benchmark group and print the results as JSON.
-    Benchmark(BenchmarkArgs),
+    Run(BenchmarkArgs),
     /// List benchmarks that are defined in the current group as a JSON array.
-    ListBenchmarks,
+    List,
 }
 
 #[derive(clap::Parser, Debug)]

collector/benchlib/src/comm/messages.rs

@@ -8,7 +8,7 @@ pub enum BenchmarkMessage {
     Result(BenchmarkResult),
 }
 
-/// Stats gathered by several executions of a single benchmark.
+/// Stats gathered by several iterations of a single benchmark.
 #[derive(Debug, serde::Serialize, serde::Deserialize)]
 pub struct BenchmarkResult {
     pub name: String,

collector/runtime-benchmarks/bufreader/src/main.rs

@@ -2,7 +2,7 @@ use std::io::{BufRead, BufReader, Write};
 
 use snap::{read::FrameDecoder, write::FrameEncoder};
 
-use benchlib::benchmark::{benchmark_group, black_box};
+use benchlib::benchmark::{black_box, run_benchmark_group};
 use benchlib::define_benchmark;
 
 const BYTES: usize = 64 * 1024 * 1024;
@@ -11,7 +11,7 @@ fn main() {
     // Inspired by https://github.com/rust-lang/rust/issues/102727
     // The pattern we want is a BufReader which wraps a Read impl where one Read::read call will
     // never fill the whole BufReader buffer.
-    benchmark_group(|group| {
+    run_benchmark_group(|group| {
         define_benchmark!(group, bufreader_snappy, {
             let data = vec![0u8; BYTES];
             move || {

collector/runtime-benchmarks/hashmap/src/main.rs

@@ -1,9 +1,9 @@
 use benchlib;
-use benchlib::benchmark::benchmark_group;
+use benchlib::benchmark::run_benchmark_group;
 use benchlib::define_benchmark;
 
 fn main() {
-    benchmark_group(|group| {
+    run_benchmark_group(|group| {
         // Measures how long does it take to insert 10 thousand numbers into a `hashbrown` hashmap.
         define_benchmark!(group, hashmap_insert_10k, {
             let mut map = hashbrown::HashMap::with_capacity_and_hasher(

collector/runtime-benchmarks/nbody/src/main.rs

@@ -1,13 +1,13 @@
 //! Calculates the N-body simulation.
 //! Code taken from https://github.com/prestontw/rust-nbody
 
-use benchlib::benchmark::benchmark_group;
+use benchlib::benchmark::run_benchmark_group;
 use benchlib::define_benchmark;
 
 mod nbody;
 
 fn main() {
-    benchmark_group(|group| {
+    run_benchmark_group(|group| {
         define_benchmark!(group, nbody_10k, {
             let mut nbody_10k = nbody::init(10000);
             || {

collector/src/runtime/benchmark.rs

@@ -6,7 +6,8 @@ use core::result::Result::Ok;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
-/// A binary that defines several benchmarks using the `benchmark_group` function from `benchlib`.
+/// A binary that defines several benchmarks using the `run_benchmark_group` function from
+/// `benchlib`.
 #[derive(Debug)]
 pub struct BenchmarkGroup {
     pub binary: PathBuf,
@@ -29,6 +30,7 @@ impl BenchmarkSuite {
     pub fn total_benchmark_count(&self) -> u64 {
         self.benchmark_names().count() as u64
     }
+
     pub fn filtered_benchmark_count(&self, filter: &BenchmarkFilter) -> u64 {
         self.benchmark_names()
             .filter(|benchmark| {
@@ -96,9 +98,9 @@ pub fn discover_benchmarks(cargo_stdout: &[u8]) -> anyhow::Result<BenchmarkSuite
     Ok(BenchmarkSuite { groups })
 }
 
-/// Uses the `list-benchmarks` command from `benchlib` to find the benchmark names from the given
+/// Uses a command from `benchlib` to find the benchmark names from the given
 /// benchmark binary.
 fn gather_benchmarks(binary: &Path) -> anyhow::Result<Vec<String>> {
-    let output = Command::new(binary).arg("list-benchmarks").output()?;
+    let output = Command::new(binary).arg("list").output()?;
     Ok(serde_json::from_slice(&output.stdout)?)
 }

collector/src/runtime/mod.rs

@@ -71,7 +71,7 @@ fn execute_runtime_benchmark_binary(
     command.arg(std::env::consts::ARCH);
     command.arg("-R");
     command.arg(binary);
-    command.arg("benchmark");
+    command.arg("run");
 
     if let Some(ref exclude) = filter.exclude {
         command.args(&["--exclude", exclude]);