From ee2d1fd224dbd713609a02457e63119c73af38b5 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Mon, 27 Jul 2026 14:26:22 -0700 Subject: [PATCH] Save raw benchmark results to `sightglass-data.csv` Benchmark runs are expensive, so every run saves a copy of its raw results to `sightglass-data.csv` in the current directory in addition to printing the human-readable analysis. If there was already a `sightglass-data.csv`, it is first moved to `sightglass-data.old.csv`, overwriting any file that was already there. --- .gitignore | 4 + README.md | 20 ++ crates/analysis/src/effect_size.rs | 5 +- crates/cli/src/benchmark.rs | 211 ++++++++++++++++-- crates/cli/src/pca_metrics/dynamic_metrics.rs | 4 +- crates/cli/tests/all/benchmark.rs | 181 ++++++++++++++- crates/cli/tests/all/util.rs | 25 ++- crates/recorder/src/measure/insts.rs | 2 +- 8 files changed, 421 insertions(+), 31 deletions(-) diff --git a/.gitignore b/.gitignore index c649bce2..edc40790 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,7 @@ benchmarks/**/*.o # Log files produced by running sightglass benchmarks. **/stdout-*-*.log **/stderr-*-*.log + +# Benchmark results saved by `sightglass-cli benchmark`. +sightglass-data.csv +sightglass-data.old.csv diff --git a/README.md b/README.md index c24242ae..c2d35986 100644 --- a/README.md +++ b/README.md @@ -222,6 +222,26 @@ with a fixed cache model and forces single-threaded Wasmtime compilation with Valgrind version when comparing data recorded on different machines for best results. +### Re-analyzing a Previous Run's Results + +Benchmark runs are expensive, so every run saves a copy of its raw results to +`sightglass-data.csv` in the current directory in addition to printing the +human-readable analysis. If there was already a `sightglass-data.csv`, it is +first moved to `sightglass-data.old.csv`, overwriting any file that was already +there. + +This means you can analyze a run's results a different way without having to +re-run the benchmarks: + +``` +$ cargo run -- summarize --input-format csv -f sightglass-data.csv +$ cargo run -- effect-size --input-format csv -f sightglass-data.csv \ + --significance-level 0.05 +``` + +The data file is not saved in `--raw` mode, where you are already choosing what +to do with the raw results yourself. + ### Getting Raw JSON or CSV Results If you don't want the results to be summarized and displayed in a human-readable diff --git a/crates/analysis/src/effect_size.rs b/crates/analysis/src/effect_size.rs index cb84c34b..258bd505 100644 --- a/crates/analysis/src/effect_size.rs +++ b/crates/analysis/src/effect_size.rs @@ -168,10 +168,7 @@ pub fn write( crate::write_in( output_file, &crate::stats_parenthetical_spec(), - &format!( - "(confidence = {}%)", - (1.0 - significance_level) * 100.0, - ), + &format!("(confidence = {}%)", (1.0 - significance_level) * 100.0,), )?; writeln!(output_file)?; writeln!(output_file)?; diff --git a/crates/cli/src/benchmark.rs b/crates/cli/src/benchmark.rs index 99358326..4a6e0204 100644 --- a/crates/cli/src/benchmark.rs +++ b/crates/cli/src/benchmark.rs @@ -1,12 +1,12 @@ use crate::suite::BenchmarkOrSuite; -use anyhow::{Context, Result, anyhow}; +use anyhow::{anyhow, Context, Result}; use clap::Parser; -use rand::{Rng, SeedableRng, rngs::SmallRng}; +use rand::{rngs::SmallRng, Rng, SeedableRng}; use sightglass_data::{Format, Measurement, Phase}; use sightglass_recorder::bench_api::Engine; use sightglass_recorder::cpu_affinity::bind_to_single_core; -use sightglass_recorder::measure::Measurements; use sightglass_recorder::measure::multi::MultiMeasure; +use sightglass_recorder::measure::Measurements; use sightglass_recorder::{bench_api::BenchApi, benchmark, measure::MeasureType}; use std::{ fs, @@ -19,6 +19,10 @@ use termcolor::{ColorChoice, NoColor, StandardStream, WriteColor}; const DEFAULT_PROCESSES: usize = 10; const DEFAULT_ITERATIONS_PER_PROCESS: usize = 10; +const DATA_FILE: &str = "sightglass-data.csv"; +const OLD_DATA_FILE: &str = "sightglass-data.old.csv"; +const DATA_DIR_ENV_VAR: &str = "SIGHTGLASS_DATA_DIR"; + #[cfg(all(target_os = "linux", feature = "callgrind"))] mod callgrind { use super::*; @@ -690,8 +694,13 @@ impl BenchmarkCommand { all_measurements.retain(|m| m.phase == phase); } + // Save a copy of the raw data before analyzing it, so that an expensive + // benchmark run is never lost to an analysis error. We hold on to any + // error until after the results have been written, so that a failure to + // save doesn't also cost the user the report they just waited for. + let saved = self.save_data_file(&all_measurements); self.write_results(&all_measurements, &mut output_file)?; - Ok(()) + saved } /// Assert that our actual `stdout` and `stderr` match our expectations. @@ -878,8 +887,11 @@ impl BenchmarkCommand { let secs = elapsed.as_secs() % 60; eprintln!("\n\nFinished benchmarking in {hours:02}h:{mins:02}m:{secs:02}s"); + // See the comment on the equivalent lines in + // `execute_in_current_process`. + let saved = self.save_data_file(&measurements); self.write_results(&measurements, &mut output_file)?; - Ok(()) + saved } /// Open the output stream for results, honoring `--output-file` and @@ -903,6 +915,37 @@ impl BenchmarkCommand { } } + /// Save a copy of the raw `measurements` to `sightglass-data.csv`, first + /// moving any existing data file to `sightglass-data.old.csv`. + /// + /// This is how we can both print human-readable analysis by default *and* + /// preserve the raw data, so that a slightly different analysis doesn't + /// require re-running the benchmarks. It is a no-op in `--raw` mode, where + /// the user is already deciding what to do with the raw data themselves. + fn save_data_file(&self, measurements: &[Measurement<'_>]) -> Result<()> { + if self.raw { + return Ok(()); + } + + let dir = std::env::var_os(DATA_DIR_ENV_VAR) + .map(PathBuf::from) + .unwrap_or_default(); + let data = dir.join(DATA_FILE); + let old_data = dir.join(OLD_DATA_FILE); + + if save_measurements(measurements, &data, &old_data)? { + eprintln!( + "\nSaved benchmark data to `{}`; the previous data was moved to `{}`.", + data.display(), + old_data.display(), + ); + } else { + eprintln!("\nSaved benchmark data to `{}`.", data.display()); + } + + Ok(()) + } + fn write_results( &self, measurements: &[Measurement<'_>], @@ -1083,6 +1126,43 @@ fn this_arch() -> &'static str { } } +/// Write `measurements` to `data` as CSV, first moving any existing `data` file +/// to `old_data`. +/// +/// Returns whether an existing data file was rotated to `old_data`. +fn save_measurements( + measurements: &[Measurement<'_>], + data: &Path, + old_data: &Path, +) -> Result { + // Note that `rename` replaces `old_data` when it already exists, which is + // what we want. We also tolerate `data` not existing (this is the first run + // in this directory) rather than checking for it first, which would race + // with any concurrent runs in the same directory. + let rotated = match fs::rename(data, old_data) { + Ok(()) => true, + Err(e) if e.kind() == io::ErrorKind::NotFound => false, + Err(e) => { + return Err(e).with_context(|| { + format!( + "failed to move `{}` to `{}`", + data.display(), + old_data.display() + ) + }); + } + }; + + let file = + fs::File::create(data).with_context(|| format!("failed to create `{}`", data.display()))?; + + Format::csv(true) + .write(measurements, file) + .with_context(|| format!("failed to write `{}`", data.display()))?; + + Ok(rotated) +} + fn display_summaries( measurements: &[Measurement<'_>], output_file: &mut dyn WriteColor, @@ -1250,6 +1330,99 @@ mod tests { assert_eq!(by_iteration, vec![(0, 110), (1, 220)]); } + /// Build a measurement whose `count` identifies it. + fn measurement(count: u64) -> Measurement<'static> { + Measurement { + arch: "x86_64".into(), + engine: sightglass_data::Engine { + name: "engine.so".into(), + flags: Some("-Ccompiler=winch".into()), + }, + wasm: "benchmarks/noop/benchmark.wasm".into(), + process: 0, + iteration: 0, + phase: Phase::Execution, + event: "cycles".into(), + count, + } + } + + /// Read back the measurements saved in `path`, projected down to the fields + /// we assert on (`Measurement` itself is not `PartialEq`). + fn saved_counts(path: &Path) -> Result> { + let measurements: Vec> = Format::csv(true).read(fs::File::open(path)?)?; + Ok(measurements + .into_iter() + .map(|m| (m.wasm.into_owned(), m.phase, m.event.into_owned(), m.count)) + .collect()) + } + + #[test] + fn save_measurements_without_an_existing_data_file() -> Result<()> { + let dir = tempfile::TempDir::new()?; + let data = dir.path().join(DATA_FILE); + let old_data = dir.path().join(OLD_DATA_FILE); + + let measurements = vec![measurement(1), measurement(2)]; + assert!( + !save_measurements(&measurements, &data, &old_data)?, + "there was nothing to rotate" + ); + + assert!(!old_data.exists(), "no old data file should be created"); + assert_eq!( + saved_counts(&data)?, + [ + ( + "benchmarks/noop/benchmark.wasm".to_string(), + Phase::Execution, + "cycles".to_string(), + 1 + ), + ( + "benchmarks/noop/benchmark.wasm".to_string(), + Phase::Execution, + "cycles".to_string(), + 2 + ), + ] + ); + + Ok(()) + } + + #[test] + fn save_measurements_rotates_an_existing_data_file() -> Result<()> { + let dir = tempfile::TempDir::new()?; + let data = dir.path().join(DATA_FILE); + let old_data = dir.path().join(OLD_DATA_FILE); + + // An existing old data file is overwritten by the rotation, rather than + // making us refuse to save. + fs::write(&old_data, "stale data from two runs ago")?; + + assert!(!save_measurements(&[measurement(1)], &data, &old_data)?); + let first = fs::read(&data)?; + + assert!( + save_measurements(&[measurement(2)], &data, &old_data)?, + "the first run's data should have been rotated" + ); + + assert_eq!( + fs::read(&old_data)?, + first, + "the old data file should hold the first run's data verbatim" + ); + assert_eq!( + saved_counts(&data)?.iter().map(|m| m.3).collect::>(), + [2], + "the data file should hold the second run's data" + ); + + Ok(()) + } + #[test] fn test_display_summaries() -> Result<()> { let fixture = std::fs::read("../../test/fixtures/old-backends.json") @@ -1551,21 +1724,19 @@ execution // Any other mismatch between the engine and per-engine flag counts is // an error. - assert!( - pairs(&[ - "-e", - "a", - "-e", - "b", - "-e", - "c", - "--engine-flags", - "x", - "--engine-flags", - "y", - ]) - .is_err() - ); + assert!(pairs(&[ + "-e", + "a", + "-e", + "b", + "-e", + "c", + "--engine-flags", + "x", + "--engine-flags", + "y", + ]) + .is_err()); Ok(()) } diff --git a/crates/cli/src/pca_metrics/dynamic_metrics.rs b/crates/cli/src/pca_metrics/dynamic_metrics.rs index 59c29294..f64ca2aa 100644 --- a/crates/cli/src/pca_metrics/dynamic_metrics.rs +++ b/crates/cli/src/pca_metrics/dynamic_metrics.rs @@ -4,9 +4,9 @@ mod component; -use super::Counts; use super::category::{Category, NUM_CATEGORIES}; -use anyhow::{Context, Result, bail}; +use super::Counts; +use anyhow::{bail, Context, Result}; #[cfg(any(test, all(target_os = "linux", feature = "callgrind")))] use sightglass_data::{Measurement, Phase}; use std::path::Path; diff --git a/crates/cli/tests/all/benchmark.rs b/crates/cli/tests/all/benchmark.rs index 8d700fc0..909d78d8 100644 --- a/crates/cli/tests/all/benchmark.rs +++ b/crates/cli/tests/all/benchmark.rs @@ -1,10 +1,23 @@ -use super::util::{benchmark, sightglass_cli, sightglass_cli_benchmark, test_engine}; +use super::util::{ + benchmark, sightglass_cli, sightglass_cli_benchmark, test_engine, DATA_DIR_ENV_VAR, +}; use assert_cmd::prelude::*; use predicates::prelude::*; -use sightglass_data::Measurement; -use std::path::PathBuf; +use sightglass_data::{Measurement, Phase}; +use std::path::{Path, PathBuf}; use tempfile::TempDir; +/// Read the measurements saved in the data file at `path`. +fn read_data_file(path: &Path) -> Vec> { + let contents = std::fs::read_to_string(path).unwrap(); + eprintln!("=== {} ===\n{contents}\n===========", path.display()); + let mut reader = csv::Reader::from_reader(contents.as_bytes()); + reader + .deserialize::>() + .map(|m| m.unwrap()) + .collect() +} + #[test] fn benchmark_output_format_requires_raw() { sightglass_cli() @@ -425,3 +438,165 @@ fn benchmark_measure_noop() { .success() .stdout(predicate::str::contains("nanoseconds")); } + +/// Without `--raw`, a copy of the raw results is saved to `sightglass-data.csv` +/// so that they can be re-analyzed without re-running the benchmarks. +#[test] +fn benchmark_saves_data_file() -> anyhow::Result<()> { + let dir = TempDir::new()?; + let data = dir.path().join("sightglass-data.csv"); + let old_data = dir.path().join("sightglass-data.old.csv"); + + sightglass_cli_benchmark() + .env(DATA_DIR_ENV_VAR, dir.path()) + .arg("--processes") + .arg("1") + .arg("--iterations-per-process") + .arg("2") + .arg("--") + .arg(benchmark("noop")) + .assert() + .success() + .stderr(predicate::str::contains("sightglass-data.csv")); + + assert!(data.exists(), "the data file was not saved"); + assert!( + !old_data.exists(), + "nothing should have been rotated on the first run" + ); + + let measurements = read_data_file(&data); + assert!( + !measurements.is_empty(), + "the data file has no measurements" + ); + for phase in [Phase::Compilation, Phase::Instantiation, Phase::Execution] { + assert!( + measurements.iter().any(|m| m.phase == phase), + "expected {phase} measurements in the data file" + ); + } + assert!( + measurements.iter().all(|m| m.wasm != "Sum Total"), + "the data file should hold the recorded data, not our synthetic totals" + ); + + // The whole point of saving the data: re-analyze it without re-benchmarking. + sightglass_cli() + .arg("summarize") + .arg("--input-format") + .arg("csv") + .arg("-f") + .arg(&data) + .assert() + .success() + .stdout(predicate::str::contains("noop")); + + Ok(()) +} + +/// The data file is also saved when the benchmark runs in multiple subprocesses. +/// The subprocesses themselves are run with `--raw`, so only the parent saves it. +#[test] +fn benchmark_saves_data_file_from_subprocesses() -> anyhow::Result<()> { + let dir = TempDir::new()?; + let data = dir.path().join("sightglass-data.csv"); + + sightglass_cli_benchmark() + .env(DATA_DIR_ENV_VAR, dir.path()) + .arg("--processes") + .arg("2") + .arg("--iterations-per-process") + .arg("1") + .arg("--") + .arg(benchmark("noop")) + .assert() + .success(); + + assert!(data.exists(), "the data file was not saved"); + let measurements = read_data_file(&data); + assert!( + !measurements.is_empty(), + "the data file has no measurements" + ); + assert!( + measurements + .iter() + .any(|m| m.process != measurements[0].process), + "expected measurements from more than one process: {measurements:?}" + ); + + Ok(()) +} + +/// A second run moves the first run's data file to `sightglass-data.old.csv`, +/// overwriting any older file that was already there. +#[test] +fn benchmark_rotates_data_file() -> anyhow::Result<()> { + let dir = TempDir::new()?; + let data = dir.path().join("sightglass-data.csv"); + let old_data = dir.path().join("sightglass-data.old.csv"); + + // Stale data from a hypothetical earlier run, which we expect to be + // overwritten once there is a real data file to rotate. + let stale = "stale data from two runs ago"; + std::fs::write(&old_data, stale)?; + + let run = || { + sightglass_cli_benchmark() + .env(DATA_DIR_ENV_VAR, dir.path()) + .arg("--processes") + .arg("1") + .arg("--iterations-per-process") + .arg("1") + .arg("--") + .arg(benchmark("noop")) + .assert() + .success(); + }; + + run(); + let first = std::fs::read(&data)?; + assert_eq!( + std::fs::read_to_string(&old_data)?, + stale, + "the first run had no data file to rotate" + ); + + run(); + assert_eq!( + std::fs::read(&old_data)?, + first, + "the first run's data should have been moved to the old data file" + ); + assert!( + !read_data_file(&data).is_empty(), + "the second run's data file has no measurements" + ); + + Ok(()) +} + +/// In `--raw` mode the user is already getting the raw data themselves, so no +/// data file is saved. +#[test] +fn benchmark_raw_does_not_save_data_file() -> anyhow::Result<()> { + let dir = TempDir::new()?; + + sightglass_cli_benchmark() + .env(DATA_DIR_ENV_VAR, dir.path()) + .arg("--raw") + .arg("--processes") + .arg("1") + .arg("--iterations-per-process") + .arg("1") + .arg("--") + .arg(benchmark("noop")) + .assert() + .success(); + + assert!(!dir.path().join("sightglass-data.csv").exists()); + assert!(!dir.path().join("sightglass-data.old.csv").exists()); + + Ok(()) +} diff --git a/crates/cli/tests/all/util.rs b/crates/cli/tests/all/util.rs index 0affab49..291990d7 100644 --- a/crates/cli/tests/all/util.rs +++ b/crates/cli/tests/all/util.rs @@ -1,12 +1,35 @@ use assert_cmd::prelude::*; use std::path::PathBuf; use std::process::Command; +use std::sync::atomic::{AtomicUsize, Ordering}; + +pub const DATA_DIR_ENV_VAR: &str = "SIGHTGLASS_DATA_DIR"; /// Get a `Command` for this crate's `sightglass-cli` executable. pub fn sightglass_cli() -> Command { drop(env_logger::try_init()); - Command::cargo_bin("sightglass-cli").unwrap() + let mut cmd = Command::cargo_bin("sightglass-cli").unwrap(); + cmd.env(DATA_DIR_ENV_VAR, data_dir()); + cmd } + +/// Get a fresh directory for `sightglass-cli` to save its benchmark data file +/// in. +/// +/// Our working directory is the crate root, so without this every non-`--raw` +/// benchmark run in the test suite would leave a `sightglass-data.csv` in the +/// source tree, and tests running in parallel would race over it. Tests that +/// assert on the data file override this with a directory of their own. +fn data_dir() -> PathBuf { + static NEXT: AtomicUsize = AtomicUsize::new(0); + let n = NEXT.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir() + .join("sightglass-test-data") + .join(format!("{}-{n}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + /// Get the path to the engine we are testing with. pub fn test_engine() -> PathBuf { if let Ok(engine) = std::env::var("SIGHTGLASS_TEST_ENGINE") { diff --git a/crates/recorder/src/measure/insts.rs b/crates/recorder/src/measure/insts.rs index 205bc639..a27b6052 100644 --- a/crates/recorder/src/measure/insts.rs +++ b/crates/recorder/src/measure/insts.rs @@ -8,7 +8,7 @@ use sightglass_data::Phase; /// counters. #[cfg(target_os = "linux")] mod linux { - use perf_event::{Builder, events::Hardware}; + use perf_event::{events::Hardware, Builder}; pub use perf_event::Counter as State;