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,7 +1,7 @@
 use crate::keys::KeyBuilder;
 use anyhow::Result;
-use sightglass_data::{EffectSize, Measurement, Phase, Summary};
-use std::{borrow::Cow, collections::BTreeSet, io::Write};
+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
 /// engines (i.e. two different commits of Wasmtime).
@@ -35,10 +35,7 @@ pub fn calculate<'a>(
         let key_measurements: Vec<_> = measurements.iter().filter(|m| key.matches(m)).collect();
 
         // NB: `BTreeSet` so they're always sorted.
-        let engines: BTreeSet<_> = key_measurements
-            .iter()
-            .map(|m| (&m.engine, &m.engine_flags))
-            .collect();
+        let engines: BTreeSet<_> = key_measurements.iter().map(|m| &m.engine).collect();
         anyhow::ensure!(
             engines.len() == 2,
             "Can only test significance between exactly two different engines. Found {} \
@@ -47,17 +44,17 @@ pub fn calculate<'a>(
         );
 
         let mut engines = engines.into_iter();
-        let (engine_a, engine_a_flags) = engines.next().unwrap();
-        let (engine_b, engine_b_flags) = engines.next().unwrap();
+        let engine_a = engines.next().unwrap();
+        let engine_b = engines.next().unwrap();
 
         let a: behrens_fisher::Stats = key_measurements
             .iter()
-            .filter(|m| m.engine.as_ref() == engine_a && &m.engine_flags == engine_a_flags)
+            .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 && &m.engine_flags == engine_b_flags)
+            .filter(|m| &m.engine == engine_b)
             .map(|m| m.count as f64)
             .collect();
 
@@ -68,10 +65,8 @@ pub fn calculate<'a>(
             phase: key.phase.unwrap(),
             event: key.event.unwrap(),
             a_engine: engine_a.clone(),
-            a_engine_flags: engine_a_flags.clone(),
             a_mean: a.mean,
             b_engine: engine_b.clone(),
-            b_engine_flags: engine_b_flags.clone(),
             b_mean: b.mean,
             significance_level,
             half_width_confidence_interval: ci,
@@ -81,18 +76,6 @@ pub fn calculate<'a>(
     Ok(results)
 }
 
-fn engine_label(engine: &str, engine_flags: &Option<Cow<str>>) -> String {
-    format!(
-        "{}{}",
-        engine,
-        if let Some(ef) = engine_flags {
-            format!(" ({ef})")
-        } else {
-            "".into()
-        }
-    )
-}
-
 /// Write a vector of [EffectSize] structures to the passed `output_file` in human-readable form.
 /// The `summaries` are needed
 pub fn write(
@@ -120,50 +103,13 @@ pub fn write(
         )?;
         writeln!(output_file)?;
 
-        let end_of_shared_prefix = |astr: &str, bstr: &str| {
-            astr.char_indices()
-                .zip(bstr.char_indices())
-                .find_map(|((i, a), (j, b))| {
-                    if a == b {
-                        None
-                    } else {
-                        debug_assert_eq!(i, j);
-                        Some(i)
-                    }
-                })
-                .unwrap_or(0)
-        };
-
         // For readability, trim the shared prefix from our two engine names.
         //
         // 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) = if effect_size.a_engine == effect_size.b_engine {
-            (
-                effect_size
-                    .a_engine_flags
-                    .as_ref()
-                    .map(|ref ef| ef.to_string())
-                    .unwrap_or_else(|| "(no flags)".into())
-                    .to_string(),
-                effect_size
-                    .b_engine_flags
-                    .as_ref()
-                    .map(|ref ef| ef.to_string())
-                    .unwrap_or_else(|| "(no flags)".into())
-                    .to_string(),
-            )
-        } else {
-            let a_label = engine_label(&effect_size.a_engine, &effect_size.a_engine_flags);
-            let b_label = engine_label(&effect_size.b_engine, &effect_size.b_engine_flags);
-            let idx_end_of_shared = end_of_shared_prefix(&a_label, &b_label);
-
-            (
-                a_label[idx_end_of_shared..].into(),
-                b_label[idx_end_of_shared..].into(),
-            )
-        };
+        let (a_eng_label, b_eng_label) =
+            effect_size.a_engine.relative_labels(&effect_size.b_engine);
 
         if effect_size.is_significant() {
             writeln!(
@@ -199,28 +145,19 @@ pub fn write(
         }
         writeln!(output_file)?;
 
-        let get_summary = |engine: &str,
-                           engine_flags: Option<Cow<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.engine_flags == engine_flags
-                        && s.wasm == wasm
-                        && s.phase == phase
-                        && s.event == event
+                    &s.engine == engine && s.wasm == wasm && s.phase == phase && s.event == event
                 })
                 .unwrap()
         };
 
         let a_summary = get_summary(
             &effect_size.a_engine,
-            effect_size.a_engine_flags,
             &effect_size.wasm,
             effect_size.phase,
             &effect_size.event,
@@ -233,7 +170,6 @@ pub fn write(
 
         let b_summary = get_summary(
             &effect_size.b_engine,
-            effect_size.b_engine_flags,
             &effect_size.wasm,
             effect_size.phase,
             &effect_size.event,

crates/analysis/src/keys.rs

@@ -1,4 +1,4 @@
-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.
@@ -81,11 +81,6 @@ impl KeyBuilder {
             .map(|m| Key {
                 arch: if self.arch { Some(m.arch) } else { None },
                 engine: if self.engine { Some(m.engine) } else { None },
-                engine_flags: if self.engine_flags {
-                    Some(m.engine_flags)
-                } else {
-                    None
-                },
                 wasm: if self.wasm { Some(m.wasm) } else { None },
                 phase: if self.phase { Some(m.phase) } else { None },
                 event: if self.event { Some(m.event) } else { None },
@@ -99,8 +94,7 @@ impl KeyBuilder {
 #[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_flags: Option<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,10 +105,6 @@ impl Key<'_> {
     pub fn matches(&self, m: &Measurement) -> bool {
         self.arch.as_ref().is_none_or(|x| *x == m.arch)
             && self.engine.as_ref().is_none_or(|x| *x == m.engine)
-            && self
-                .engine_flags
-                .as_ref()
-                .is_none_or(|x| *x == m.engine_flags)
             && self.wasm.as_ref().is_none_or(|x| *x == m.wasm)
             && self.phase.as_ref().is_none_or(|x| *x == m.phase)
             && self.event.as_ref().is_none_or(|x| *x == m.event)
@@ -130,11 +120,13 @@ 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()),
-            engine_flags: Some(Some("-Wfoo=bar".into())),
         };
 
         // More test cases are needed, but this provides a sanity check for the matched key and
@@ -148,7 +140,6 @@ mod tests {
             phase: Phase::Compilation,
             event: "cycles".into(),
             count: 42,
-            engine_flags: Some("-Wfoo=bar".into()),
         }));
     }
 }

crates/analysis/src/summarize.rs

@@ -15,7 +15,6 @@ pub fn calculate<'a>(measurements: &[Measurement<'a>]) -> Vec<Summary<'a>> {
         summaries.push(Summary {
             arch: k.arch.unwrap(),
             engine: k.engine.unwrap(),
-            engine_flags: k.engine_flags.unwrap(),
             wasm: k.wasm.unwrap(),
             phase: k.phase.unwrap(),
             event: k.event.unwrap(),
@@ -70,7 +69,6 @@ pub fn write(mut summaries: Vec<Summary<'_>>, output_file: &mut dyn Write) -> Re
             .then_with(|| x.wasm.cmp(&y.wasm))
             .then_with(|| x.event.cmp(&y.event))
             .then_with(|| x.engine.cmp(&y.engine))
-            .then_with(|| x.engine_flags.cmp(&y.engine_flags))
     });
 
     let mut last_phase = None;
@@ -95,16 +93,9 @@ pub fn write(mut summaries: Vec<Summary<'_>>, output_file: &mut dyn Write) -> Re
             writeln!(output_file, "    {}", summary.event)?;
         }
 
-        let engine_flags = match summary.engine_flags {
-            None => "".into(),
-            Some(ef) => {
-                format!(" ({ef})")
-            }
-        };
-
         writeln!(
             output_file,
-            "      [{} {:.2} {}] {}{engine_flags}",
+            "      [{} {:.2} {}] {}",
             summary.min, summary.mean, summary.max, summary.engine,
         )?;
     }
@@ -115,15 +106,14 @@ 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() {
         fn measurement<'a>(count: u64) -> Measurement<'a> {
             Measurement {
                 arch: "x86".into(),
                 engine: "wasmtime".into(),
-                engine_flags: None,
                 wasm: "bench.wasm".into(),
                 process: 42,
                 iteration: 0,
@@ -140,7 +130,6 @@ mod tests {
             vec![Summary {
                 arch: "x86".into(),
                 engine: "wasmtime".into(),
-                engine_flags: None,
                 wasm: "bench.wasm".into(),
                 phase: Phase::Compilation,
                 event: "cycles".into(),
@@ -159,7 +148,6 @@ mod tests {
             Measurement {
                 arch: "x86".into(),
                 engine: "wasmtime".into(),
-                engine_flags: None,
                 wasm: "bench.wasm".into(),
                 process: 42,
                 iteration: 0,
@@ -184,8 +172,10 @@ mod tests {
         fn measurement<'a>(flags: Option<Cow<'a, str>>, count: u64) -> Measurement<'a> {
             Measurement {
                 arch: "x86".into(),
-                engine: "wasmtime".into(),
-                engine_flags: flags,
+                engine: Engine {
+                    name: "wasmtime".into(),
+                    flags,
+                },
                 wasm: "bench.wasm".into(),
                 process: 42,
                 iteration: 0,

crates/cli/src/benchmark.rs

@@ -174,13 +174,13 @@ impl BenchmarkCommand {
             .collect();
         let mut all_measurements = vec![];
 
-        for (i, engine) in self.engines.iter().enumerate() {
-            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);
+                .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)? };
@@ -209,12 +209,11 @@ impl BenchmarkCommand {
                 let stderr = Path::new(&stderr);
                 let stdin = None;
 
-                let mut measurements = Measurements::with_flags(
-                    this_arch(),
-                    engine_name,
-                    wasm_file,
-                    self.engine_flags.as_deref(),
-                );
+                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);
                     measure.build()

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();
 }