Record and Use Engine Flags in Summarize/Effect-Size Tools (#293)

* Allow benchmark names to be overriden

This can be useful when the engine filename is not useful
or when wanting a concise way to differentiate benchmark
runs that may use the same engine but with different flags.

* Record engine_flags with measurement data

Previously, only the engine name/path was stored for each measurement.
The flags used represents valuable information for both understanding
how a historical run was performed as well as determining how different
configurations impact performance for the same engine.

* Update summarize/effect-size to use engine flags

Previously, recordings were updated to capture
engine flag information.  This change updates the code
used for summaries and effect-size analysis to allow
for comparing engine/flag combinations against each other
which is useful for a class of comparisons.

* Fix issues raised by the tests, improve coverage

* Use single field for sightglass_data::Engine

This simplifies consuming and building measurement and other
related structures but at the cost of somewhat more complex
serialization.  This is largely due to our use of CSV as
an input/output format and rust-csv's longstanding inability
to handle flattened structures.

See BurntSushi/rust-csv#98

Author: posborne

crates/analysis/src/effect_size.rs

@@ -1,6 +1,6 @@
 use crate::keys::KeyBuilder;
 use anyhow::Result;
-use sightglass_data::{EffectSize, Measurement, Phase, Summary};
+use sightglass_data::{EffectSize, Engine, Measurement, Phase, Summary};
 use std::{collections::BTreeSet, io::Write};
 
 /// Find the effect size (and confidence interval) of between two different
@@ -25,7 +25,10 @@ pub fn calculate<'a>(
         significance_level,
     );
 
-    let keys = KeyBuilder::all().engine(false).keys(measurements);
+    let keys = KeyBuilder::all()
+        .engine(false)
+        .engine_flags(false)
+        .keys(measurements);
     let mut results = Vec::with_capacity(keys.len());
 
     for key in keys {
@@ -46,12 +49,12 @@ pub fn calculate<'a>(
 
         let a: behrens_fisher::Stats = key_measurements
             .iter()
-            .filter(|m| m.engine.as_ref() == engine_a)
+            .filter(|m| &m.engine == engine_a)
             .map(|m| m.count as f64)
             .collect();
         let b: behrens_fisher::Stats = key_measurements
             .iter()
-            .filter(|m| m.engine.as_ref() == engine_b)
+            .filter(|m| &m.engine == engine_b)
             .map(|m| m.count as f64)
             .collect();
 
@@ -101,21 +104,12 @@ pub fn write(
         writeln!(output_file)?;
 
         // For readability, trim the shared prefix from our two engine names.
-        let end_of_shared_prefix = effect_size
-            .a_engine
-            .char_indices()
-            .zip(effect_size.b_engine.char_indices())
-            .find_map(|((i, a), (j, b))| {
-                if a == b {
-                    None
-                } else {
-                    debug_assert_eq!(i, j);
-                    Some(i)
-                }
-            })
-            .unwrap_or(0);
-        let a_engine = &effect_size.a_engine[end_of_shared_prefix..];
-        let b_engine = &effect_size.b_engine[end_of_shared_prefix..];
+        //
+        // Furthermore, there are a few special cases:
+        // 1. If the engines are the same, show just the flags.
+        // 2. If not, show the computed full label with common prefix removed.
+        let (a_eng_label, b_eng_label) =
+            effect_size.a_engine.relative_labels(&effect_size.b_engine);
 
         if effect_size.is_significant() {
             writeln!(
@@ -132,9 +126,7 @@ pub fn write(
                 let ratio_ci = effect_size.half_width_confidence_interval / effect_size.a_mean;
                 writeln!(
                     output_file,
-                    "  {a_engine} is {ratio_min:.2}x to {ratio_max:.2}x faster than {b_engine}!",
-                    a_engine = a_engine,
-                    b_engine = b_engine,
+                    "  {a_eng_label} is {ratio_min:.2}x to {ratio_max:.2}x faster than {b_eng_label}!",
                     ratio_min = ratio - ratio_ci,
                     ratio_max = ratio + ratio_ci,
                 )?;
@@ -143,9 +135,7 @@ pub fn write(
                 let ratio_ci = effect_size.half_width_confidence_interval / effect_size.b_mean;
                 writeln!(
                     output_file,
-                    "  {b_engine} is {ratio_min:.2}x to {ratio_max:.2}x faster than {a_engine}!",
-                    a_engine = a_engine,
-                    b_engine = b_engine,
+                    "  {b_eng_label} is {ratio_min:.2}x to {ratio_max:.2}x faster than {a_eng_label}!",
                     ratio_min = ratio - ratio_ci,
                     ratio_max = ratio + ratio_ci,
                 )?;
@@ -155,13 +145,13 @@ pub fn write(
         }
         writeln!(output_file)?;
 
-        let get_summary = |engine: &str, wasm: &str, phase: Phase, event: &str| {
+        let get_summary = |engine: &Engine, wasm: &str, phase: Phase, event: &str| {
             // TODO this sorting is not using `arch` which is not guaranteed to be the same in
             // result sets; potentially this could re-use `Key` functionality.
             summaries
                 .iter()
                 .find(|s| {
-                    s.engine == engine && s.wasm == wasm && s.phase == phase && s.event == event
+                    &s.engine == engine && s.wasm == wasm && s.phase == phase && s.event == event
                 })
                 .unwrap()
         };
@@ -175,7 +165,7 @@ pub fn write(
         writeln!(
             output_file,
             "  [{} {:.2} {}] {}",
-            a_summary.min, a_summary.mean, a_summary.max, a_engine,
+            a_summary.min, a_summary.mean, a_summary.max, a_eng_label,
         )?;
 
         let b_summary = get_summary(
@@ -187,7 +177,7 @@ pub fn write(
         writeln!(
             output_file,
             "  [{} {:.2} {}] {}",
-            b_summary.min, b_summary.mean, b_summary.max, b_engine,
+            b_summary.min, b_summary.mean, b_summary.max, b_eng_label,
         )?;
     }
 

crates/analysis/src/keys.rs

@@ -1,11 +1,12 @@
-use sightglass_data::{Measurement, Phase};
+use sightglass_data::{Engine, Measurement, Phase};
 use std::{borrow::Cow, collections::BTreeSet};
 
 /// A builder for finding keys in a set of measurements.
 #[derive(Copy, Clone)]
 pub struct KeyBuilder {
     arch: bool,
     engine: bool,
+    engine_flags: bool,
     wasm: bool,
     phase: bool,
     event: bool,
@@ -20,6 +21,7 @@ impl KeyBuilder {
             wasm: true,
             phase: true,
             event: true,
+            engine_flags: true,
         }
     }
 
@@ -31,6 +33,7 @@ impl KeyBuilder {
             wasm: false,
             phase: false,
             event: false,
+            engine_flags: false,
         }
     }
 
@@ -52,6 +55,12 @@ impl KeyBuilder {
         self
     }
 
+    /// Whether to group keys by engine flags or not.
+    pub fn engine_flags(mut self, engine_flags: bool) -> Self {
+        self.engine_flags = engine_flags;
+        self
+    }
+
     /// Whether to group keys by phase or not.
     pub fn phase(mut self, phase: bool) -> Self {
         self.phase = phase;
@@ -82,10 +91,10 @@ impl KeyBuilder {
 }
 
 /// A key for grouping measurements together.
-#[derive(PartialOrd, Ord, PartialEq, Eq, Hash)]
+#[derive(PartialOrd, Ord, PartialEq, Eq, Hash, Debug)]
 pub struct Key<'a> {
     pub arch: Option<Cow<'a, str>>,
-    pub engine: Option<Cow<'a, str>>,
+    pub engine: Option<Engine<'a>>,
     pub wasm: Option<Cow<'a, str>>,
     pub phase: Option<Phase>,
     pub event: Option<Cow<'a, str>>,
@@ -111,7 +120,10 @@ mod tests {
     fn matching_fields() {
         let key = Key {
             arch: Some("x86".into()),
-            engine: Some("wasmtime".into()),
+            engine: Some(Engine {
+                name: "wasmtime".into(),
+                flags: None,
+            }),
             wasm: Some("bench.wasm".into()),
             phase: Some(Phase::Compilation),
             event: Some("cycles".into()),

crates/analysis/src/summarize.rs

@@ -3,7 +3,7 @@ use anyhow::Result;
 use sightglass_data::{Measurement, Summary};
 use std::io::Write;
 
-/// Summarize measurements grouped by: architecture, engine, benchmark file, phase and event.
+/// Summarize measurements grouped by: architecture, engine, flags, benchmark file, phase and event.
 pub fn calculate<'a>(measurements: &[Measurement<'a>]) -> Vec<Summary<'a>> {
     let mut summaries = Vec::new();
     for k in KeyBuilder::all().keys(measurements) {
@@ -106,7 +106,7 @@ pub fn write(mut summaries: Vec<Summary<'_>>, output_file: &mut dyn Write) -> Re
 #[cfg(test)]
 mod tests {
     use super::*;
-    use sightglass_data::Phase;
+    use sightglass_data::{Engine, Phase};
 
     #[test]
     fn simple_statistics() {
@@ -164,4 +164,33 @@ mod tests {
 
         assert_eq!(calculate(&measurements).len(), 2);
     }
+
+    #[test]
+    fn differing_engine_flags() {
+        use std::borrow::Cow;
+
+        fn measurement<'a>(flags: Option<Cow<'a, str>>, count: u64) -> Measurement<'a> {
+            Measurement {
+                arch: "x86".into(),
+                engine: Engine {
+                    name: "wasmtime".into(),
+                    flags,
+                },
+                wasm: "bench.wasm".into(),
+                process: 42,
+                iteration: 0,
+                phase: Phase::Execution,
+                event: "cycles".into(),
+                count,
+            }
+        }
+        let measurements = vec![
+            measurement(Some("-Wfoo=bar".into()), 0),
+            measurement(Some("-Wfoo=bar".into()), 1),
+            measurement(Some("-Wdead=beeef".into()), 2),
+            measurement(None, 3),
+        ];
+
+        assert_eq!(calculate(&measurements).len(), 3);
+    }
 }

crates/cli/src/benchmark.rs

@@ -57,6 +57,14 @@ pub struct BenchmarkCommand {
     #[structopt(long = "processes", default_value = "10", value_name = "PROCESSES")]
     processes: usize,
 
+    /// Override the "engine" name; this is useful if running experiments that might
+    /// not have a differentiating engine name (e.g. if customizing the flags).
+    ///
+    /// If multiple engines are provided, the order of names provided here should
+    /// match the order of the engines specified.
+    #[structopt(long = "name", short = "n")]
+    names: Option<Vec<String>>,
+
     /// How many times should we run a benchmark in a single process?
     #[structopt(
         long = "iterations-per-process",
@@ -166,14 +174,19 @@ impl BenchmarkCommand {
             .collect();
         let mut all_measurements = vec![];
 
-        for engine in &self.engines {
-            let engine_path = check_engine_path(engine)?;
+        for (i, engine_name) in self.engines.iter().enumerate() {
+            let engine_path = check_engine_path(engine_name)?;
+            let engine_name = self
+                .names
+                .as_ref()
+                .and_then(|names| names.get(i).map(|s| s.as_str()))
+                .unwrap_or(engine_name);
             log::info!("Using benchmark engine: {}", engine_path.display());
             let lib = unsafe { libloading::Library::new(&engine_path)? };
             let mut bench_api = unsafe { BenchApi::new(&lib)? };
 
             for wasm_file in &wasm_files {
-                log::info!("Using Wasm benchmark: {}", wasm_file);
+                log::info!("Using Wasm benchmark: {wasm_file}");
 
                 // Use the provided --working-dir, otherwise find the Wasm file's parent directory.
                 let working_dir = self.get_working_directory(&wasm_file)?;
@@ -196,6 +209,10 @@ impl BenchmarkCommand {
                 let stderr = Path::new(&stderr);
                 let stdin = None;
 
+                let engine = sightglass_data::Engine {
+                    name: engine_name.into(),
+                    flags: self.engine_flags.as_ref().map(|ef| ef.into()),
+                };
                 let mut measurements = Measurements::new(this_arch(), engine, wasm_file);
                 let mut measure = if self.measures.len() <= 1 {
                     let measure = self.measures.first().unwrap_or(&MeasureType::Cycles);
@@ -488,10 +505,10 @@ fn display_summaries(measurements: &[Measurement<'_>], output_file: &mut dyn Wri
 // engine's dylib.
 pub fn check_engine_path(engine: &str) -> Result<PathBuf> {
     if Path::new(engine).exists() {
-        log::debug!("Using engine path: {}", engine);
+        log::debug!("Using engine path: {engine}");
         Ok(PathBuf::from(engine))
     } else {
-        Err(anyhow!("invalid path to engine: {}", engine))
+        Err(anyhow!("invalid path to engine: {engine}"))
     }
 }
 

crates/cli/tests/all/benchmark.rs

@@ -117,11 +117,13 @@ fn benchmark_csv() {
 
     assert
         .stdout(
-            predicate::str::starts_with("arch,engine,wasm,process,iteration,phase,event,count\n")
-                .and(predicate::str::contains(benchmark("noop")))
-                .and(predicate::str::contains("Compilation"))
-                .and(predicate::str::contains("Instantiation"))
-                .and(predicate::str::contains("Execution")),
+            predicate::str::starts_with(
+                "arch,engine,engine_flags,wasm,process,iteration,phase,event,count\n",
+            )
+            .and(predicate::str::contains(benchmark("noop")))
+            .and(predicate::str::contains("Compilation"))
+            .and(predicate::str::contains("Instantiation"))
+            .and(predicate::str::contains("Execution")),
         )
         .success();
 }