diff --git a/CHANGELOG.md b/CHANGELOG.md index fda110bee..ba23e15f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Features +- Add `--filter-failed` option to exclude results with non-zero exit codes from the relative speed comparison, see #591 (@tavian-dev) - Add `--reference-name` option to give a meaningful name to the reference command, see #808 (@niklasdewally) - The `--ignore-failure` option now supports a comma-separated list of exit codes to ignore (e.g., `--ignore-failure=1,2`), see #836 (@sharkdp) - Python scripts: Add `--time-unit` option to `advanced_statistics.py` (@sharkdp) diff --git a/src/benchmark/benchmark_result.rs b/src/benchmark/benchmark_result.rs index 287c73bef..9813aa3b5 100644 --- a/src/benchmark/benchmark_result.rs +++ b/src/benchmark/benchmark_result.rs @@ -53,3 +53,46 @@ pub struct BenchmarkResult { #[serde(skip_serializing_if = "BTreeMap::is_empty")] pub parameters: BTreeMap, } + +impl BenchmarkResult { + /// Returns true if any run had a non-zero or missing exit code. + pub fn has_failure(&self) -> bool { + self.exit_codes.iter().any(|code| *code != Some(0)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn result_with_exit_codes(codes: Vec>) -> BenchmarkResult { + BenchmarkResult { + exit_codes: codes, + ..Default::default() + } + } + + #[test] + fn test_has_failure_all_success() { + let r = result_with_exit_codes(vec![Some(0), Some(0), Some(0)]); + assert!(!r.has_failure()); + } + + #[test] + fn test_has_failure_with_nonzero() { + let r = result_with_exit_codes(vec![Some(0), Some(1), Some(0)]); + assert!(r.has_failure()); + } + + #[test] + fn test_has_failure_with_signal_kill() { + let r = result_with_exit_codes(vec![Some(0), None]); + assert!(r.has_failure()); + } + + #[test] + fn test_has_failure_empty() { + let r = result_with_exit_codes(vec![]); + assert!(!r.has_failure()); + } +} diff --git a/src/benchmark/relative_speed.rs b/src/benchmark/relative_speed.rs index 90918e527..ebbdb8ee2 100644 --- a/src/benchmark/relative_speed.rs +++ b/src/benchmark/relative_speed.rs @@ -120,6 +120,15 @@ pub fn compute( #[cfg(test)] fn create_result(name: &str, mean: Scalar) -> BenchmarkResult { + create_result_with_exit_codes(name, mean, vec![Some(0)]) +} + +#[cfg(test)] +fn create_result_with_exit_codes( + name: &str, + mean: Scalar, + exit_codes: Vec>, +) -> BenchmarkResult { use std::collections::BTreeMap; BenchmarkResult { @@ -134,7 +143,7 @@ fn create_result(name: &str, mean: Scalar) -> BenchmarkResult { max: mean, times: None, memory_usage_byte: None, - exit_codes: Vec::new(), + exit_codes, parameters: BTreeMap::new(), } } @@ -178,3 +187,26 @@ fn test_compute_relative_speed_for_zero_times() { assert!(annotated_results.is_none()); } + +#[test] +fn test_has_failure_excludes_from_comparison() { + use approx::assert_relative_eq; + + let results = vec![ + create_result("cmd1", 3.0), + create_result_with_exit_codes("cmd_fail", 0.5, vec![Some(1)]), + create_result("cmd3", 5.0), + ]; + + // Without filtering, the failed command (0.5s) would be "fastest" + let all_results = compute_with_check(&results, SortOrder::Command).unwrap(); + assert_eq!(all_results.len(), 3); + assert!(all_results[1].is_reference); // cmd_fail is fastest + + // After filtering out failed results, comparison should work on successes only + let filtered: Vec<_> = results.into_iter().filter(|r| !r.has_failure()).collect(); + let filtered_results = compute_with_check(&filtered, SortOrder::Command).unwrap(); + assert_eq!(filtered_results.len(), 2); + assert!(filtered_results[0].is_reference); // cmd1 is now fastest + assert_relative_eq!(1.0, filtered_results[0].relative_speed); +} diff --git a/src/benchmark/scheduler.rs b/src/benchmark/scheduler.rs index 242dd0e7d..0e516517d 100644 --- a/src/benchmark/scheduler.rs +++ b/src/benchmark/scheduler.rs @@ -67,15 +67,33 @@ impl<'a> Scheduler<'a> { return; } - let reference = self - .options - .reference_command - .as_ref() - .map(|_| &self.results[0]) - .unwrap_or_else(|| relative_speed::fastest_of(&self.results)); + let results: Vec<_> = if self.options.filter_failed { + self.results.iter().filter(|r| !r.has_failure()).collect() + } else { + self.results.iter().collect() + }; + + if results.len() < 2 { + return; + } + + let results_slice: Vec<_> = results.iter().map(|r| (*r).clone()).collect(); + + let reference = if self.options.reference_command.is_some() { + // When a reference command is set, it's always the first result. + // If it was filtered out, fall back to the fastest remaining result. + let ref_cmd = &self.results[0]; + if self.options.filter_failed && ref_cmd.has_failure() { + relative_speed::fastest_of(&results_slice) + } else { + &results_slice[0] + } + } else { + relative_speed::fastest_of(&results_slice) + }; if let Some(annotated_results) = relative_speed::compute_with_check_from_reference( - &self.results, + &results_slice, reference, self.options.sort_order_speed_comparison, ) { diff --git a/src/cli.rs b/src/cli.rs index b12f6d34c..8b03476f3 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -236,6 +236,14 @@ fn build_command() -> Command { 'all-non-zero', all non-zero exit codes are ignored. You can also provide \ a comma-separated list of exit codes to ignore (e.g., --ignore-failure=1,2)."), ) + .arg( + Arg::new("filter-failed") + .long("filter-failed") + .action(ArgAction::SetTrue) + .help("Exclude results with non-zero exit codes from the relative speed \ + comparison and from exported results. This is useful when using \ + '--ignore-failure' with parameter scans where some combinations fail."), + ) .arg( Arg::new("style") .long("style") diff --git a/src/options.rs b/src/options.rs index 7c83da1c5..d2ae1c988 100644 --- a/src/options.rs +++ b/src/options.rs @@ -246,6 +246,9 @@ pub struct Options { /// Which time unit to use when displaying results pub time_unit: Option, + + /// Whether to exclude results with non-zero exit codes from comparisons + pub filter_failed: bool, } impl Default for Options { @@ -267,6 +270,7 @@ impl Default for Options { executor_kind: ExecutorKind::default(), command_output_policies: vec![CommandOutputPolicy::Null], time_unit: None, + filter_failed: false, command_input_policy: CommandInputPolicy::Null, } } @@ -435,6 +439,8 @@ impl Options { }; } + options.filter_failed = matches.get_flag("filter-failed"); + options.time_unit = match matches.get_one::("time-unit").map(|s| s.as_str()) { Some("microsecond") => Some(Unit::MicroSecond), Some("millisecond") => Some(Unit::MilliSecond),