From dacee4a741e337831e0db5f58e772b112c740075 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 18 Dec 2024 19:06:10 -0800 Subject: [PATCH 1/3] Refactor `benchmark` function to take an `Engine` --- crates/cli/src/benchmark.rs | 13 +++++++------ crates/recorder/src/benchmark.rs | 27 ++++----------------------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/crates/cli/src/benchmark.rs b/crates/cli/src/benchmark.rs index ae496ac4..40fa653e 100644 --- a/crates/cli/src/benchmark.rs +++ b/crates/cli/src/benchmark.rs @@ -2,6 +2,7 @@ use crate::suite::BenchmarkOrSuite; use anyhow::{anyhow, Context, Result}; 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::{bench_api::BenchApi, benchmark::benchmark, measure::MeasureType}; @@ -197,18 +198,18 @@ impl BenchmarkCommand { let stderr = Path::new(&stderr); let stdin = None; - benchmark( + let engine = Engine::new( &mut bench_api, &working_dir, stdout, stderr, stdin, - &bytes, - self.stop_after_phase.clone(), - self.engine_flags.as_deref(), - &mut measure, &mut measurements, - )?; + &mut measure, + self.engine_flags.as_deref(), + ); + + benchmark(engine, &bytes, self.stop_after_phase.clone())?; self.check_output(Path::new(wasm_file), stdout, stderr)?; measurements.next_iteration(); diff --git a/crates/recorder/src/benchmark.rs b/crates/recorder/src/benchmark.rs index 375824b3..d72a4040 100644 --- a/crates/recorder/src/benchmark.rs +++ b/crates/recorder/src/benchmark.rs @@ -1,9 +1,8 @@ -use crate::bench_api::{BenchApi, Engine}; -use crate::measure::{Measure, Measurements}; +use crate::bench_api::Engine; +use crate::measure::Measure; use anyhow::Result; use log::info; use sightglass_data::Phase; -use std::path::Path; /// Measure various phases of a Wasm module's lifetime. /// @@ -12,34 +11,16 @@ use std::path::Path; /// /// Optionally stop after the given `stop_after_phase`, rather than running all /// phases. -pub fn benchmark<'a, 'b, 'c>( - bench_api: &'a mut BenchApi<'b>, - working_dir: &Path, - stdout_path: &Path, - stderr_path: &Path, - stdin_path: Option<&Path>, +pub fn benchmark<'a, 'b, 'c, M: Measure>( + engine: Engine<'_, '_, '_, M>, wasm_bytes: &[u8], stop_after_phase: Option, - execution_flags: Option<&str>, - measure: &'a mut impl Measure, - measurements: &'a mut Measurements<'c>, ) -> Result<()> { #[cfg(target_os = "linux")] info!("Benchmark scheduled on CPU: {}", unsafe { libc::sched_getcpu() }); - let engine = Engine::new( - bench_api, - working_dir, - stdout_path, - stderr_path, - stdin_path, - measurements, - measure, - execution_flags, - ); - // Measure the module compilation. let module = engine.compile(wasm_bytes); info!("Compiled successfully"); From dccb292f16f3747f701366758d27821b615a05c8 Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 18 Dec 2024 19:29:04 -0800 Subject: [PATCH 2/3] Reuse `bench_api::Engine`s across benchmark iterations --- Cargo.toml | 1 + crates/cli/src/benchmark.rs | 61 ++++++++++++++++++-------------- crates/recorder/src/bench_api.rs | 10 ++++++ crates/recorder/src/benchmark.rs | 12 +++---- 4 files changed, 51 insertions(+), 33 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1e618b1a..240d8f55 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,4 +1,5 @@ [workspace] +resolver = "2" members = [ "crates/analysis", "crates/api", diff --git a/crates/cli/src/benchmark.rs b/crates/cli/src/benchmark.rs index 40fa653e..15d13014 100644 --- a/crates/cli/src/benchmark.rs +++ b/crates/cli/src/benchmark.rs @@ -179,42 +179,49 @@ impl BenchmarkCommand { let bytes = fs::read(&wasm_file).context("Attempting to read Wasm bytes")?; log::debug!("Wasm benchmark size: {} bytes", bytes.len()); + let wasm_hash = { + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + let mut hasher = DefaultHasher::new(); + wasm_file.hash(&mut hasher); + hasher.finish() + }; + let stdout = format!("stdout-{:x}-{}.log", wasm_hash, std::process::id()); + let stdout = Path::new(&stdout); + let stderr = format!("stderr-{:x}-{}.log", wasm_hash, std::process::id()); + let stderr = Path::new(&stderr); + let stdin = None; + let mut measurements = Measurements::new(this_arch(), engine, wasm_file); let mut measure = self.measure.build(); + let engine = Engine::new( + &mut bench_api, + &working_dir, + stdout, + stderr, + stdin, + &mut measurements, + &mut measure, + self.engine_flags.as_deref(), + ); + let mut engine = Some(engine); + // Run the benchmark (compilation, instantiation, and execution) several times in // this process. - for i in 0..self.iterations_per_process { - let wasm_hash = { - use std::collections::hash_map::DefaultHasher; - use std::hash::{Hash, Hasher}; - let mut hasher = DefaultHasher::new(); - wasm_file.hash(&mut hasher); - hasher.finish() - }; - let stdout = format!("stdout-{:x}-{}-{}.log", wasm_hash, std::process::id(), i); - let stdout = Path::new(&stdout); - let stderr = format!("stderr-{:x}-{}-{}.log", wasm_hash, std::process::id(), i); - let stderr = Path::new(&stderr); - let stdin = None; - - let engine = Engine::new( - &mut bench_api, - &working_dir, - stdout, - stderr, - stdin, - &mut measurements, - &mut measure, - self.engine_flags.as_deref(), - ); - - benchmark(engine, &bytes, self.stop_after_phase.clone())?; + for _ in 0..self.iterations_per_process { + let new_engine = benchmark( + engine.take().unwrap(), + &bytes, + self.stop_after_phase.clone(), + )?; + engine = Some(new_engine); self.check_output(Path::new(wasm_file), stdout, stderr)?; - measurements.next_iteration(); + engine.as_mut().unwrap().measurements().next_iteration(); } + drop(engine); all_measurements.extend(measurements.finish()); } } diff --git a/crates/recorder/src/bench_api.rs b/crates/recorder/src/bench_api.rs index 874cf4a8..3ab9d27d 100644 --- a/crates/recorder/src/bench_api.rs +++ b/crates/recorder/src/bench_api.rs @@ -134,6 +134,11 @@ where } } + /// Get this engine's measurements. + pub fn measurements(&mut self) -> &mut Measurements<'c> { + unsafe { (*self.measurement_data).get_mut().1 } + } + /// Compile the Wasm into a module. pub fn compile(self, wasm: &[u8]) -> Module<'a, 'b, 'c, M> { let result = @@ -215,6 +220,11 @@ pub struct Module<'a, 'b, 'c, M> { } impl<'a, 'b, 'c, M> Module<'a, 'b, 'c, M> { + /// Turn this module back into an engine. + pub fn into_engine(self) -> Engine<'a, 'b, 'c, M> { + self.engine + } + /// Instantiate this module, returning the resulting `Instance`. pub fn instantiate(self) -> Instance<'a, 'b, 'c, M> { let result = unsafe { (self.engine.bench_api.wasm_bench_instantiate)(self.engine.engine) }; diff --git a/crates/recorder/src/benchmark.rs b/crates/recorder/src/benchmark.rs index d72a4040..421dc3f5 100644 --- a/crates/recorder/src/benchmark.rs +++ b/crates/recorder/src/benchmark.rs @@ -12,10 +12,10 @@ use sightglass_data::Phase; /// Optionally stop after the given `stop_after_phase`, rather than running all /// phases. pub fn benchmark<'a, 'b, 'c, M: Measure>( - engine: Engine<'_, '_, '_, M>, + engine: Engine<'a, 'b, 'c, M>, wasm_bytes: &[u8], stop_after_phase: Option, -) -> Result<()> { +) -> Result> { #[cfg(target_os = "linux")] info!("Benchmark scheduled on CPU: {}", unsafe { libc::sched_getcpu() @@ -26,7 +26,7 @@ pub fn benchmark<'a, 'b, 'c, M: Measure>( info!("Compiled successfully"); if stop_after_phase == Some(Phase::Compilation) { - return Ok(()); + return Ok(module.into_engine()); } // Measure the module instantiation. @@ -34,11 +34,11 @@ pub fn benchmark<'a, 'b, 'c, M: Measure>( info!("Instantiated successfully"); if stop_after_phase == Some(Phase::Instantiation) { - return Ok(()); + return Ok(instance.into_module().into_engine()); } - instance.execute(); + let module = instance.execute(); info!("Executed successfully"); - Ok(()) + Ok(module.into_engine()) } From 51e1d7c832933e4d63cf5a77457d425acfcf2e7e Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Wed, 18 Dec 2024 20:01:43 -0800 Subject: [PATCH 3/3] Allow benchmarking just one particular compilation/instantiation/execution phase This avoids recompiling the Wasm module many times when we are only interested in benchmarking instantiation or execution. Note that it is still compiled once per process, but you can compile it exactly once if you do `--processes 1`. --- crates/cli/src/benchmark.rs | 72 +++++++++++++++++++++++-------- crates/cli/tests/all/benchmark.rs | 30 +++++++++++-- crates/recorder/src/bench_api.rs | 45 ++++++++++++++----- crates/recorder/src/benchmark.rs | 64 ++++++++++++++++++--------- 4 files changed, 158 insertions(+), 53 deletions(-) diff --git a/crates/cli/src/benchmark.rs b/crates/cli/src/benchmark.rs index 15d13014..2ffa20b1 100644 --- a/crates/cli/src/benchmark.rs +++ b/crates/cli/src/benchmark.rs @@ -5,7 +5,7 @@ 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::{bench_api::BenchApi, benchmark::benchmark, measure::MeasureType}; +use sightglass_recorder::{bench_api::BenchApi, benchmark, measure::MeasureType}; use std::{ fs, io::{self, BufWriter, Write}, @@ -106,9 +106,10 @@ pub struct BenchmarkCommand { #[structopt(short("d"), long("working-dir"), parse(from_os_str))] working_dir: Option, - /// Stop measuring after the given phase (compilation/instantiation/execution). - #[structopt(long("stop-after"))] - stop_after_phase: Option, + /// Benchmark only the given phase (compilation, instantiation, or + /// execution). Benchmarks all phases if omitted. + #[structopt(long("benchmark-phase"))] + benchmark_phase: Option, /// The significance level for confidence intervals. Typical values are 0.01 /// and 0.05, which correspond to 99% and 95% confidence respectively. This @@ -195,6 +196,8 @@ impl BenchmarkCommand { let mut measurements = Measurements::new(this_arch(), engine, wasm_file); let mut measure = self.measure.build(); + // Create the bench API engine and cache it for reuse across all + // iterations of this benchmark. let engine = Engine::new( &mut bench_api, &working_dir, @@ -207,35 +210,68 @@ impl BenchmarkCommand { ); let mut engine = Some(engine); + // And if we are benchmarking just a post-compilation phase, + // then eagerly compile the Wasm module for reuse. + let mut module = None; + if let Some(Phase::Instantiation | Phase::Execution) = self.benchmark_phase { + module = Some(engine.take().unwrap().compile(&bytes)); + } + // Run the benchmark (compilation, instantiation, and execution) several times in // this process. for _ in 0..self.iterations_per_process { - let new_engine = benchmark( - engine.take().unwrap(), - &bytes, - self.stop_after_phase.clone(), - )?; - engine = Some(new_engine); + match self.benchmark_phase { + None => { + let new_engine = benchmark::all(engine.take().unwrap(), &bytes)?; + engine = Some(new_engine); + } + Some(Phase::Compilation) => { + let new_engine = + benchmark::compilation(engine.take().unwrap(), &bytes)?; + engine = Some(new_engine); + } + Some(Phase::Instantiation) => { + let new_module = benchmark::instantiation(module.take().unwrap())?; + module = Some(new_module); + } + Some(Phase::Execution) => { + let new_module = benchmark::execution(module.take().unwrap())?; + module = Some(new_module); + } + } self.check_output(Path::new(wasm_file), stdout, stderr)?; - engine.as_mut().unwrap().measurements().next_iteration(); + engine + .as_mut() + .map(|e| e.measurements()) + .or_else(|| module.as_mut().map(|m| m.measurements())) + .unwrap() + .next_iteration(); } - drop(engine); + drop((engine, module)); all_measurements.extend(measurements.finish()); } } + // If we are only benchmarking one phase then filter out any + // measurements for other phases. These get included because we have to + // compile at least once to measure instantiation, for example. + if let Some(phase) = self.benchmark_phase { + all_measurements.retain(|m| m.phase == phase); + } + self.write_results(&all_measurements, &mut output_file)?; Ok(()) } /// Assert that our actual `stdout` and `stderr` match our expectations. fn check_output(&self, wasm_file: &Path, stdout: &Path, stderr: &Path) -> Result<()> { - // If we aren't going through all phases and executing the Wasm, then we - // won't have any actual output to check. - if self.stop_after_phase.is_some() { - return Ok(()); + match self.benchmark_phase { + None | Some(Phase::Execution) => {} + // If we aren't executing the Wasm, then we won't have any actual + // output to check. + Some(Phase::Compilation | Phase::Instantiation) => return Ok(()), } let wasm_file_dir: PathBuf = if let Some(dir) = wasm_file.parent() { @@ -328,8 +364,8 @@ impl BenchmarkCommand { command.env("WASM_BENCH_USE_SMALL_WORKLOAD", "1"); } - if let Some(phase) = self.stop_after_phase { - command.arg("--stop-after").arg(phase.to_string()); + if let Some(phase) = self.benchmark_phase { + command.arg("--benchmark-phase").arg(phase.to_string()); } if let Some(flags) = &self.engine_flags { diff --git a/crates/cli/tests/all/benchmark.rs b/crates/cli/tests/all/benchmark.rs index a1714162..f2ffa88d 100644 --- a/crates/cli/tests/all/benchmark.rs +++ b/crates/cli/tests/all/benchmark.rs @@ -5,14 +5,14 @@ use sightglass_data::Measurement; use std::path::PathBuf; #[test] -fn benchmark_stop_after_compilation() { +fn benchmark_phase_compilation() { sightglass_cli_benchmark() .arg("--raw") .arg("--processes") .arg("2") .arg("--iterations-per-process") .arg("1") - .arg("--stop-after") + .arg("--benchmark-phase") .arg("compilation") .arg(benchmark("noop")) .assert() @@ -25,25 +25,47 @@ fn benchmark_stop_after_compilation() { } #[test] -fn benchmark_stop_after_instantiation() { +fn benchmark_phase_instantiation() { sightglass_cli_benchmark() .arg("--raw") .arg("--processes") .arg("2") .arg("--iterations-per-process") .arg("1") - .arg("--stop-after") + .arg("--benchmark-phase") .arg("instantiation") .arg(benchmark("noop")) .assert() .success() .stdout( predicate::str::contains("Compilation") + .not() .and(predicate::str::contains("Instantiation")) .and(predicate::str::contains("Execution").not()), ); } +#[test] +fn benchmark_phase_execution() { + sightglass_cli_benchmark() + .arg("--raw") + .arg("--processes") + .arg("2") + .arg("--iterations-per-process") + .arg("1") + .arg("--benchmark-phase") + .arg("execution") + .arg(benchmark("noop")) + .assert() + .success() + .stdout( + predicate::str::contains("Compilation") + .not() + .and(predicate::str::contains("Instantiation").not()) + .and(predicate::str::contains("Execution")), + ); +} + #[test] fn benchmark_json() { let assert = sightglass_cli_benchmark() diff --git a/crates/recorder/src/bench_api.rs b/crates/recorder/src/bench_api.rs index 3ab9d27d..17f82115 100644 --- a/crates/recorder/src/bench_api.rs +++ b/crates/recorder/src/bench_api.rs @@ -74,10 +74,7 @@ pub struct Engine<'a, 'b, 'c, M> { engine: *mut c_void, } -impl<'a, 'b, 'c, M> Engine<'a, 'b, 'c, M> -where - M: Measure, -{ +impl<'a, 'b, 'c, M> Engine<'a, 'b, 'c, M> { /// Construct a new engine from the given `BenchApi`. // NB: take a mutable reference to the `BenchApi` so that no one else can // call its API methods out of order. @@ -90,7 +87,10 @@ where measurements: &'a mut Measurements<'c>, measure: &'a mut M, execution_flags: Option<&'a str>, - ) -> Self { + ) -> Self + where + M: Measure, + { let working_dir = working_dir.display().to_string(); let stdout_path = stdout_path.display().to_string(); let stderr_path = stderr_path.display().to_string(); @@ -148,7 +148,10 @@ where } /// Bench API callback for the start of compilation. - extern "C" fn compilation_start(data: *mut u8) { + extern "C" fn compilation_start(data: *mut u8) + where + M: Measure, + { log::debug!("Starting compilation measurement"); let data = data as *mut (*mut M, *mut Measurements<'b>); let measure = unsafe { data.as_mut().unwrap().0.as_mut().unwrap() }; @@ -156,7 +159,10 @@ where } /// Bench API callback for the start of instantiation. - extern "C" fn instantiation_start(data: *mut u8) { + extern "C" fn instantiation_start(data: *mut u8) + where + M: Measure, + { log::debug!("Starting instantiation measurement"); let data = data as *mut (*mut M, *mut Measurements<'b>); let measure = unsafe { data.as_mut().unwrap().0.as_mut().unwrap() }; @@ -164,7 +170,10 @@ where } /// Bench API callback for the start of execution. - extern "C" fn execution_start(data: *mut u8) { + extern "C" fn execution_start(data: *mut u8) + where + M: Measure, + { log::debug!("Starting execution measurement"); let data = data as *mut (*mut M, *mut Measurements<'b>); let measure = unsafe { data.as_mut().unwrap().0.as_mut().unwrap() }; @@ -172,7 +181,10 @@ where } /// Bench API callback for the end of compilation. - extern "C" fn compilation_end(data: *mut u8) { + extern "C" fn compilation_end(data: *mut u8) + where + M: Measure, + { let data = data as *mut (*mut M, *mut Measurements<'b>); let (measure, measurements) = unsafe { let data = data.as_mut().unwrap(); @@ -183,7 +195,10 @@ where } /// Bench API callback for the end of instantiation. - extern "C" fn instantiation_end(data: *mut u8) { + extern "C" fn instantiation_end(data: *mut u8) + where + M: Measure, + { let data = data as *mut (*mut M, *mut Measurements<'b>); let (measure, measurements) = unsafe { let data = data.as_mut().unwrap(); @@ -194,7 +209,10 @@ where } /// Bench API callback for the end of execution. - extern "C" fn execution_end(data: *mut u8) { + extern "C" fn execution_end(data: *mut u8) + where + M: Measure, + { let data = data as *mut (*mut M, *mut Measurements<'b>); let (measure, measurements) = unsafe { let data = data.as_mut().unwrap(); @@ -225,6 +243,11 @@ impl<'a, 'b, 'c, M> Module<'a, 'b, 'c, M> { self.engine } + /// Get this engine's measurements. + pub fn measurements(&mut self) -> &mut Measurements<'c> { + self.engine.measurements() + } + /// Instantiate this module, returning the resulting `Instance`. pub fn instantiate(self) -> Instance<'a, 'b, 'c, M> { let result = unsafe { (self.engine.bench_api.wasm_bench_instantiate)(self.engine.engine) }; diff --git a/crates/recorder/src/benchmark.rs b/crates/recorder/src/benchmark.rs index 421dc3f5..397eaa35 100644 --- a/crates/recorder/src/benchmark.rs +++ b/crates/recorder/src/benchmark.rs @@ -1,20 +1,13 @@ -use crate::bench_api::Engine; +use crate::bench_api::{Engine, Module}; use crate::measure::Measure; use anyhow::Result; use log::info; -use sightglass_data::Phase; - -/// Measure various phases of a Wasm module's lifetime. -/// -/// Provide paths to files created for logging the Wasm's `stdout` and `stderr` -/// and (optionally) a file read and piped into the Wasm execution as `stdin`. -/// -/// Optionally stop after the given `stop_after_phase`, rather than running all -/// phases. -pub fn benchmark<'a, 'b, 'c, M: Measure>( + +/// Measure all phases of a Wasm module's lifetime: compilation, instantiation, +/// and execution. +pub fn all<'a, 'b, 'c, M: Measure>( engine: Engine<'a, 'b, 'c, M>, wasm_bytes: &[u8], - stop_after_phase: Option, ) -> Result> { #[cfg(target_os = "linux")] info!("Benchmark scheduled on CPU: {}", unsafe { @@ -25,20 +18,51 @@ pub fn benchmark<'a, 'b, 'c, M: Measure>( let module = engine.compile(wasm_bytes); info!("Compiled successfully"); - if stop_after_phase == Some(Phase::Compilation) { - return Ok(module.into_engine()); - } - // Measure the module instantiation. let instance = module.instantiate(); info!("Instantiated successfully"); - if stop_after_phase == Some(Phase::Instantiation) { - return Ok(instance.into_module().into_engine()); - } - let module = instance.execute(); info!("Executed successfully"); Ok(module.into_engine()) } + +/// Measure just the compilation phase of a Wasm module's lifetime. +pub fn compilation<'a, 'b, 'c, M: Measure>( + engine: Engine<'a, 'b, 'c, M>, + wasm_bytes: &[u8], +) -> Result> { + #[cfg(target_os = "linux")] + info!("Benchmark scheduled on CPU: {}", unsafe { + libc::sched_getcpu() + }); + + let module = engine.compile(wasm_bytes); + info!("Compiled successfully"); + + Ok(module.into_engine()) +} + +/// Measure just the instantiation phase of a Wasm module's lifetime. +pub fn instantiation<'a, 'b, 'c, M: Measure>( + module: Module<'a, 'b, 'c, M>, +) -> Result> { + let instance = module.instantiate(); + info!("Instantiated successfully"); + + Ok(instance.into_module()) +} + +/// Measure just the execution phase of a Wasm module's lifetime. +pub fn execution<'a, 'b, 'c, M: Measure>( + module: Module<'a, 'b, 'c, M>, +) -> Result> { + let instance = module.instantiate(); + info!("Instantiated successfully"); + + let module = instance.execute(); + info!("Executed successfully"); + + Ok(module) +}