From f6464a294300749e696a39046b633243f8a2baff Mon Sep 17 00:00:00 2001 From: William Chargin Date: Mon, 14 Sep 2020 20:28:02 -0700 Subject: [PATCH] Support matrices of independent parameter lists This patch permits the `-L`/`--parameter-list` argument to appear multiple times. Each additional parameter increases the dimensionality of the parameter matrix. One benchmark will be run for each combination of parameters (i.e., the benchmark space is the Cartesian product of the parameter spaces). For now, `--parameter-list` and `--parameter-scan` are still mutually exclusive. If desired, a follow-up change could similarly permit multiple occurrences of `--parameter-scan` and also permit use of both listed and scanned parameters together. (After all, `--parameter-scan` can be thought of as syntactic sugar for a parameter list.) This implementation is a little profligate with memory usage for `BenchmarkResult`s: each result contains a separate copy of all parameter names (as `String`s). This could be done away with by threading lifetimes through the `BenchmarkResult`s, or by ref-counting the names. Because `BenchmarkResult`s now have maps with arbitrary key sets, we can no longer use `serde` to serialize them to CSV. But a CSV writer is easy to write by hand, so we just do that. Fixes #253. Test Plan: Some unit tests included. As an integration test, try: ``` cargo run --release -- --runs 10 \ -L foo foo1,foo2 -L bar bar9,bar8 \ 'echo {foo} {bar}' \ 'printf "%s\n" {foo} {bar}' \ --export-csv /tmp/out ``` with all the export formats. wchargin-branch: param-list-matrix wchargin-source: 11051ab222c9de0fbc6ac6a080fbca48e05996c2 --- src/hyperfine/app.rs | 6 +- src/hyperfine/benchmark.rs | 25 +++---- src/hyperfine/export/asciidoc.rs | 29 +++++--- src/hyperfine/export/csv.rs | 88 ++++++++++++++++++++++-- src/hyperfine/export/markdown.rs | 20 +++--- src/hyperfine/internal.rs | 3 +- src/hyperfine/parameter_range.rs | 3 +- src/hyperfine/types.rs | 77 +++++++++++++++------ src/main.rs | 113 +++++++++++++++++++++++++++---- 9 files changed, 286 insertions(+), 78 deletions(-) diff --git a/src/hyperfine/app.rs b/src/hyperfine/app.rs index c8eec78..6421ab2 100644 --- a/src/hyperfine/app.rs +++ b/src/hyperfine/app.rs @@ -3,9 +3,10 @@ use atty::Stream; use clap::{crate_version, App, AppSettings, Arg, ArgMatches}; use std::ffi::OsString; -pub fn get_arg_matches(args: T) -> ArgMatches<'static> +pub fn get_arg_matches(args: I) -> ArgMatches<'static> where - T: Iterator, + I: IntoIterator, + T: Into + Clone, { let app = build_app(); app.get_matches_from(args) @@ -134,6 +135,7 @@ fn build_app() -> App<'static, 'static> { .long("parameter-list") .short("L") .takes_value(true) + .multiple(true) .allow_hyphen_values(true) .value_names(&["VAR", "VALUES"]) .conflicts_with_all(&["parameter-scan", "parameter-step-size"]) diff --git a/src/hyperfine/benchmark.rs b/src/hyperfine/benchmark.rs index c118600..f2de69d 100644 --- a/src/hyperfine/benchmark.rs +++ b/src/hyperfine/benchmark.rs @@ -223,12 +223,7 @@ pub fn run_benchmark( } else { &values[num] }; - match cmd.get_parameter() { - Some((param, value)) => { - Command::new_parametrized(preparation_command, param, value.clone()) - } - None => Command::new(preparation_command), - } + Command::new_parametrized(preparation_command, cmd.get_parameters().clone()) }); // Warmup phase @@ -414,16 +409,9 @@ pub fn run_benchmark( } // Run cleanup command - let cleanup_cmd = - options - .cleanup_command - .as_ref() - .map(|cleanup_command| match cmd.get_parameter() { - Some((param, value)) => { - Command::new_parametrized(cleanup_command, param, value.clone()) - } - None => Command::new(cleanup_command), - }); + let cleanup_cmd = options.cleanup_command.as_ref().map(|cleanup_command| { + Command::new_parametrized(cleanup_command, cmd.get_parameters().clone()) + }); run_cleanup_command(&options.shell, &cleanup_cmd, options.show_output)?; Ok(BenchmarkResult::new( @@ -436,6 +424,9 @@ pub fn run_benchmark( t_min, t_max, times_real, - cmd.get_parameter().as_ref().map(|p| p.1.to_string()), + cmd.get_parameters() + .iter() + .map(|(name, value)| ((*name).to_string(), value.to_string())) + .collect(), )) } diff --git a/src/hyperfine/export/asciidoc.rs b/src/hyperfine/export/asciidoc.rs index 99f7ea8..cae6075 100644 --- a/src/hyperfine/export/asciidoc.rs +++ b/src/hyperfine/export/asciidoc.rs @@ -83,6 +83,7 @@ fn test_asciidoc_header() { /// Ensure each table row is generated properly #[test] fn test_asciidoc_table_row() { + use std::collections::BTreeMap; let result = BenchmarkResult::new( String::from("sleep 1"), // command 0.10491992406666667, // mean @@ -98,7 +99,7 @@ fn test_asciidoc_table_row() { 0.10745223440000001, 0.10697327940000001, ], - None, // param + BTreeMap::new(), // param ); let expms = format!( @@ -134,6 +135,7 @@ fn test_asciidoc_table_row() { /// Ensure commands get properly escaped #[test] fn test_asciidoc_table_row_command_escape() { + use std::collections::BTreeMap; let result = BenchmarkResult::new( String::from("sleep 1|"), // command 0.10491992406666667, // mean @@ -149,7 +151,7 @@ fn test_asciidoc_table_row_command_escape() { 0.10745223440000001, 0.10697327940000001, ], - None, // param + BTreeMap::new(), // param ); let exps = format!( "| `sleep 1\\|`\n\ @@ -169,11 +171,12 @@ fn test_asciidoc_table_row_command_escape() { /// Integration test #[test] fn test_asciidoc() { + use std::collections::BTreeMap; let exporter = AsciidocExporter::default(); // NOTE: results are fabricated, unlike above let results = vec![ BenchmarkResult::new( - String::from("command | 1"), + String::from("FOO=1 BAR=2 command | 1"), 1.0, 2.0, 1.0, @@ -182,10 +185,15 @@ fn test_asciidoc() { 5.0, 6.0, vec![7.0, 8.0, 9.0], - None, + { + let mut params = BTreeMap::new(); + params.insert("foo".into(), "1".into()); + params.insert("bar".into(), "2".into()); + params + }, ), BenchmarkResult::new( - String::from("command | 2"), + String::from("FOO=1 BAR=7 command | 2"), 11.0, 12.0, 11.0, @@ -194,7 +202,12 @@ fn test_asciidoc() { 15.0, 16.0, vec![17.0, 18.0, 19.0], - None, + { + let mut params = BTreeMap::new(); + params.insert("foo".into(), "1".into()); + params.insert("bar".into(), "7".into()); + params + }, ), ]; // NOTE: only testing with s, s/ms is tested elsewhere @@ -203,11 +216,11 @@ fn test_asciidoc() { |===\n\ | Command | Mean [s] | Min…Max [s]\n\ \n\ - | `command \\| 1`\n\ + | `FOO=1 BAR=2 command \\| 1`\n\ | 1.000 ± 2.000\n\ | 5.000…6.000\n\ \n\ - | `command \\| 2`\n\ + | `FOO=1 BAR=7 command \\| 2`\n\ | 11.000 ± 12.000\n\ | 15.000…16.000\n\ |===\n\ diff --git a/src/hyperfine/export/csv.rs b/src/hyperfine/export/csv.rs index 1612952..6260a2a 100644 --- a/src/hyperfine/export/csv.rs +++ b/src/hyperfine/export/csv.rs @@ -3,6 +3,7 @@ use super::Exporter; use crate::hyperfine::types::BenchmarkResult; use crate::hyperfine::units::Unit; +use std::borrow::Cow; use std::io::{Error, ErrorKind, Result}; use csv::WriterBuilder; @@ -13,12 +14,35 @@ pub struct CsvExporter {} impl Exporter for CsvExporter { fn serialize(&self, results: &[BenchmarkResult], _unit: Option) -> Result> { let mut writer = WriterBuilder::new().from_writer(vec![]); - for res in results { - // The list of times cannot be exported to the CSV file - remove it: - let mut result = res.clone(); - result.times = None; - writer.serialize(result)?; + { + let mut headers: Vec = [ + // The list of times cannot be exported to the CSV file - omit it. + "command", "mean", "stddev", "median", "user", "system", "min", "max", + ] + .iter() + .map(|x| (*x).to_string()) + .collect(); + if let Some(res) = results.first() { + for param_name in res.parameters.keys() { + headers.push(format!("parameter_{}", param_name)); + } + } + writer.write_record(headers)?; + } + + for res in results { + let mut fields = Vec::new(); + fields.push(Cow::Borrowed(res.command.as_bytes())); + for f in &[ + res.mean, res.stddev, res.median, res.user, res.system, res.min, res.max, + ] { + fields.push(Cow::Owned(f.to_string().into_bytes())) + } + for v in res.parameters.values() { + fields.push(Cow::Borrowed(v.as_bytes())) + } + writer.write_record(fields)?; } writer @@ -26,3 +50,57 @@ impl Exporter for CsvExporter { .map_err(|e| Error::new(ErrorKind::Other, e)) } } + +#[test] +fn test_csv() { + use std::collections::BTreeMap; + let exporter = CsvExporter::default(); + + // NOTE: results are fabricated + let results = vec![ + BenchmarkResult::new( + String::from("FOO=one BAR=two command | 1"), + 1.0, + 2.0, + 1.0, + 3.0, + 4.0, + 5.0, + 6.0, + vec![7.0, 8.0, 9.0], + { + let mut params = BTreeMap::new(); + params.insert("foo".into(), "one".into()); + params.insert("bar".into(), "two".into()); + params + }, + ), + BenchmarkResult::new( + String::from("FOO=one BAR=seven command | 2"), + 11.0, + 12.0, + 11.0, + 13.0, + 14.0, + 15.0, + 16.5, + vec![17.0, 18.0, 19.0], + { + let mut params = BTreeMap::new(); + params.insert("foo".into(), "one".into()); + params.insert("bar".into(), "seven".into()); + params + }, + ), + ]; + let exps: String = String::from( + "command,mean,stddev,median,user,system,min,max,parameter_bar,parameter_foo\n\ + FOO=one BAR=two command | 1,1,2,1,3,4,5,6,two,one\n\ + FOO=one BAR=seven command | 2,11,12,11,13,14,15,16.5,seven,one\n\ + ", + ); + let gens = + String::from_utf8(exporter.serialize(&results, Some(Unit::Second)).unwrap()).unwrap(); + + assert_eq!(exps, gens); +} diff --git a/src/hyperfine/export/markdown.rs b/src/hyperfine/export/markdown.rs index 39ca1f8..f8432a8 100644 --- a/src/hyperfine/export/markdown.rs +++ b/src/hyperfine/export/markdown.rs @@ -82,6 +82,7 @@ fn add_table_row(dest: &mut Vec, entry: &BenchmarkResultWithRelativeSpeed, u /// the units for all entries when the time unit is not given. #[test] fn test_markdown_format_ms() { + use std::collections::BTreeMap; let exporter = MarkdownExporter::default(); let mut timing_results = vec![]; @@ -96,7 +97,7 @@ fn test_markdown_format_ms() { 0.1023, // min 0.1080, // max vec![0.1, 0.1, 0.1], // times - None, // parameter + BTreeMap::new(), // parameter )); timing_results.push(BenchmarkResult::new( @@ -109,7 +110,7 @@ fn test_markdown_format_ms() { 2.0020, // min 2.0080, // max vec![2.0, 2.0, 2.0], // times - None, // parameter + BTreeMap::new(), // parameter )); let formatted = String::from_utf8(exporter.serialize(&timing_results, None).unwrap()).unwrap(); @@ -129,6 +130,7 @@ fn test_markdown_format_ms() { /// the units for all entries when the time unit is not given. #[test] fn test_markdown_format_s() { + use std::collections::BTreeMap; let exporter = MarkdownExporter::default(); let mut timing_results = vec![]; @@ -143,7 +145,7 @@ fn test_markdown_format_s() { 2.0020, // min 2.0080, // max vec![2.0, 2.0, 2.0], // times - None, // parameter + BTreeMap::new(), // parameter )); timing_results.push(BenchmarkResult::new( @@ -156,7 +158,7 @@ fn test_markdown_format_s() { 0.1023, // min 0.1080, // max vec![0.1, 0.1, 0.1], // times - None, // parameter + BTreeMap::new(), // parameter )); let formatted = String::from_utf8(exporter.serialize(&timing_results, None).unwrap()).unwrap(); @@ -175,6 +177,7 @@ fn test_markdown_format_s() { /// The given time unit (s) is used to set the units for all entries. #[test] fn test_markdown_format_time_unit_s() { + use std::collections::BTreeMap; let exporter = MarkdownExporter::default(); let mut timing_results = vec![]; @@ -189,7 +192,7 @@ fn test_markdown_format_time_unit_s() { 0.1023, // min 0.1080, // max vec![0.1, 0.1, 0.1], // times - None, // parameter + BTreeMap::new(), // parameter )); timing_results.push(BenchmarkResult::new( @@ -202,7 +205,7 @@ fn test_markdown_format_time_unit_s() { 2.0020, // min 2.0080, // max vec![2.0, 2.0, 2.0], // times - None, // parameter + BTreeMap::new(), // parameter )); let formatted = String::from_utf8( @@ -227,6 +230,7 @@ fn test_markdown_format_time_unit_s() { /// the units for all entries. #[test] fn test_markdown_format_time_unit_ms() { + use std::collections::BTreeMap; let exporter = MarkdownExporter::default(); let mut timing_results = vec![]; @@ -241,7 +245,7 @@ fn test_markdown_format_time_unit_ms() { 2.0020, // min 2.0080, // max vec![2.0, 2.0, 2.0], // times - None, // parameter + BTreeMap::new(), // parameter )); timing_results.push(BenchmarkResult::new( @@ -254,7 +258,7 @@ fn test_markdown_format_time_unit_ms() { 0.1023, // min 0.1080, // max vec![0.1, 0.1, 0.1], // times - None, // parameter + BTreeMap::new(), // parameter )); let formatted = String::from_utf8( diff --git a/src/hyperfine/internal.rs b/src/hyperfine/internal.rs index ad1a49d..97961de 100644 --- a/src/hyperfine/internal.rs +++ b/src/hyperfine/internal.rs @@ -128,6 +128,7 @@ fn test_max() { #[test] fn test_compute_relative_speed() { use approx::assert_relative_eq; + use std::collections::BTreeMap; let create_result = |name: &str, mean| BenchmarkResult { command: name.into(), @@ -139,7 +140,7 @@ fn test_compute_relative_speed() { min: mean, max: mean, times: None, - parameter: None, + parameters: BTreeMap::new(), }; let results = vec![ diff --git a/src/hyperfine/parameter_range.rs b/src/hyperfine/parameter_range.rs index 5f73580..92d15b4 100644 --- a/src/hyperfine/parameter_range.rs +++ b/src/hyperfine/parameter_range.rs @@ -94,8 +94,7 @@ fn build_parameterized_commands<'a, T: Numeric>( for cmd in &command_strings { commands.push(Command::new_parametrized( cmd, - param_name, - ParameterValue::Numeric(value.into()), + vec![(param_name, ParameterValue::Numeric(value.into()))], )); } } diff --git a/src/hyperfine/types.rs b/src/hyperfine/types.rs index f65fa07..a60b1ad 100644 --- a/src/hyperfine/types.rs +++ b/src/hyperfine/types.rs @@ -1,6 +1,7 @@ use rust_decimal::Decimal; /// This module contains common internal types. use serde::*; +use std::collections::BTreeMap; use std::fmt; use crate::hyperfine::units::{Second, Unit}; @@ -11,7 +12,7 @@ pub const DEFAULT_SHELL: &str = "sh"; #[cfg(windows)] pub const DEFAULT_SHELL: &str = "cmd.exe"; -#[derive(Debug, Clone, Serialize, Copy, PartialEq)] +#[derive(Debug, Clone, Serialize, Copy, PartialEq, Eq)] #[serde(untagged)] pub enum NumericType { Int(i32), @@ -39,7 +40,7 @@ impl Into for Decimal { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum ParameterValue { Text(String), Numeric(NumericType), @@ -55,49 +56,79 @@ impl<'a> ToString for ParameterValue { } /// A command that should be benchmarked. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Command<'a> { /// The command that should be executed (without parameter substitution) expression: &'a str, - /// A possible parameter value. - parameter: Option<(&'a str, ParameterValue)>, + /// Zero or more parameter values. + parameters: Vec<(&'a str, ParameterValue)>, } impl<'a> Command<'a> { pub fn new(expression: &'a str) -> Command<'a> { Command { expression, - parameter: None, + parameters: Vec::new(), } } pub fn new_parametrized( expression: &'a str, - parameter: &'a str, - value: ParameterValue, + parameters: Vec<(&'a str, ParameterValue)>, ) -> Command<'a> { Command { expression, - parameter: Some((parameter, value)), + parameters, } } pub fn get_shell_command(&self) -> String { - match &self.parameter { - Some((param_name, param_value)) => self.expression.replace( - &format!("{{{param_name}}}", param_name = param_name), - ¶m_value.to_string(), - ), - None => self.expression.into(), + let mut result = String::new(); + let mut replacements = BTreeMap::::new(); + for (param_name, param_value) in &self.parameters { + replacements.insert( + format!("{{{param_name}}}", param_name = param_name), + param_value.to_string(), + ); } + let mut remaining = self.expression; + // Manually replace consecutive occurrences to avoid double-replacing: e.g., + // + // hyperfine -L foo 'a,{bar}' -L bar 'baz,quux' 'echo {foo} {bar}' + // + // should not ever run 'echo baz baz'. See `test_get_shell_command_nonoverlapping`. + 'outer: while let Some(head) = remaining.chars().next() { + for (k, v) in &replacements { + if remaining.starts_with(k.as_str()) { + result.push_str(&v); + remaining = &remaining[k.len()..]; + continue 'outer; + } + } + result.push(head); + remaining = &remaining[head.len_utf8()..]; + } + result } - pub fn get_parameter(&self) -> &Option<(&'a str, ParameterValue)> { - &self.parameter + pub fn get_parameters(&self) -> &Vec<(&'a str, ParameterValue)> { + &self.parameters } } +#[test] +fn test_get_shell_command_nonoverlapping() { + let cmd = Command::new_parametrized( + "echo {foo} {bar}", + vec![ + ("foo", ParameterValue::Text("{bar} baz".into())), + ("bar", ParameterValue::Text("quux".into())), + ], + ); + assert_eq!(cmd.get_shell_command(), "echo {bar} baz quux"); +} + impl<'a> fmt::Display for Command<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.get_shell_command()) @@ -199,6 +230,8 @@ impl Default for HyperfineOptions { } /// Set of values that will be exported. +// NOTE: `serde` is used for JSON serialization, but not for CSV serialization due to the +// `parameters` map. Update `src/hyperfine/export/csv.rs` with new fields, as appropriate. #[derive(Debug, Default, Clone, Serialize, PartialEq)] pub struct BenchmarkResult { /// The command that was run @@ -229,9 +262,9 @@ pub struct BenchmarkResult { #[serde(skip_serializing_if = "Option::is_none")] pub times: Option>, - /// Any parameter used - #[serde(skip_serializing_if = "Option::is_none")] - pub parameter: Option, + /// Any parameter values used + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + pub parameters: BTreeMap, } impl BenchmarkResult { @@ -246,7 +279,7 @@ impl BenchmarkResult { min: Second, max: Second, times: Vec, - parameter: Option, + parameters: BTreeMap, ) -> Self { BenchmarkResult { command, @@ -258,7 +291,7 @@ impl BenchmarkResult { min, max, times: Some(times), - parameter, + parameters, } } } diff --git a/src/main.rs b/src/main.rs index 3181f0a..f4c0b68 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,4 +1,5 @@ use std::cmp; +use std::collections::BTreeMap; use std::env; use std::io; @@ -207,23 +208,60 @@ fn build_commands<'a>(matches: &'a ArgMatches<'_>) -> Vec> { Ok(commands) => commands, Err(e) => error(&e.to_string()), } - } else if let Some(mut args) = matches.values_of("parameter-list") { - let param_name = args.next().unwrap(); - let param_list_str = args.next().unwrap(); - - let param_list = tokenize(param_list_str); + } else if let Some(args) = matches.values_of("parameter-list") { + let args: Vec<_> = args.collect(); + let param_names_and_values: Vec<(&str, Vec)> = args + .chunks_exact(2) + .map(|pair| { + let name = pair[0]; + let list_str = pair[1]; + (name, tokenize(list_str)) + }) + .collect(); + { + let dupes = find_dupes(param_names_and_values.iter().map(|(name, _)| *name)); + if !dupes.is_empty() { + error(&format!("duplicate parameter names: {}", &dupes.join(", "))) + } + } let command_list = command_strings.collect::>(); - let mut commands = Vec::with_capacity(param_list.len() * command_list.len()); + let dimensions: Vec = std::iter::once(command_list.len()) + .chain( + param_names_and_values + .iter() + .map(|(_, values)| values.len()), + ) + .collect(); + let param_space_size = dimensions.iter().product(); + if param_space_size == 0 { + return Vec::new(); + } - for value in param_list { - for cmd in &command_list { - commands.push(Command::new_parametrized( - cmd, - param_name, - ParameterValue::Text(value.clone()), - )); + let mut commands = Vec::with_capacity(param_space_size); + let mut index = vec![0usize; dimensions.len()]; + 'outer: loop { + let (command_index, params_indices) = index.split_first().unwrap(); + let parameters = param_names_and_values + .iter() + .zip(params_indices) + .map(|((name, values), i)| (*name, ParameterValue::Text(values[*i].clone()))) + .collect(); + commands.push(Command::new_parametrized( + command_list[*command_index], + parameters, + )); + + // Increment index, exiting loop on overflow. + for (i, n) in index.iter_mut().zip(dimensions.iter()) { + *i += 1; + if *i < *n { + continue 'outer; + } else { + *i = 0; + } } + break 'outer; } commands @@ -231,3 +269,52 @@ fn build_commands<'a>(matches: &'a ArgMatches<'_>) -> Vec> { command_strings.map(Command::new).collect() } } + +/// Finds all the strings that appear multiple times in the input iterator, returning them in +/// sorted order. If no string appears more than once, the result is an empty vector. +fn find_dupes<'a, I: IntoIterator>(i: I) -> Vec<&'a str> { + let mut counts = BTreeMap::<&'a str, usize>::new(); + for s in i { + *counts.entry(s).or_default() += 1; + } + counts + .into_iter() + .filter_map(|(k, n)| if n > 1 { Some(k) } else { None }) + .collect() +} + +#[test] +fn test_build_commands_cross_product() { + let matches = get_arg_matches(vec![ + "hyperfine", + "-L", + "foo", + "a,b", + "-L", + "bar", + "z,y", + "echo {foo} {bar}", + "printf '%s\n' {foo} {bar}", + ]); + let result = build_commands(&matches); + + // Iteration order: command list first, then parameters in listed order (here, "foo" before + // "bar", which is distinct from their sorted order), with parameter values in listed order. + let pv = |s: &str| ParameterValue::Text(s.to_string()); + let cmd = |cmd: usize, foo: &str, bar: &str| { + let expression = ["echo {foo} {bar}", "printf '%s\n' {foo} {bar}"][cmd]; + let params = vec![("foo", pv(foo)), ("bar", pv(bar))]; + Command::new_parametrized(expression, params) + }; + let expected = vec![ + cmd(0, "a", "z"), + cmd(1, "a", "z"), + cmd(0, "b", "z"), + cmd(1, "b", "z"), + cmd(0, "a", "y"), + cmd(1, "a", "y"), + cmd(0, "b", "y"), + cmd(1, "b", "y"), + ]; + assert_eq!(result, expected); +}