From 7d937045910968fbb2c050e803d79bc1c1e5984b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 29 Apr 2024 09:27:55 -0400 Subject: [PATCH] refactor: remove conditional color code in bench reporter (#23593) There is no need for this conditional code because it's handled by the `colors` module. --- cli/tools/bench/mitata.rs | 169 +++++++++++------------------------ cli/tools/bench/reporters.rs | 9 +- 2 files changed, 55 insertions(+), 123 deletions(-) diff --git a/cli/tools/bench/mitata.rs b/cli/tools/bench/mitata.rs index 57d1fc1dff..64c5d5f69a 100644 --- a/cli/tools/bench/mitata.rs +++ b/cli/tools/bench/mitata.rs @@ -201,7 +201,6 @@ pub mod reporter { pub struct Options { size: usize, pub avg: bool, - pub colors: bool, pub min_max: bool, pub percentiles: bool, } @@ -210,7 +209,6 @@ pub mod reporter { pub fn new(names: &[&str]) -> Options { Options { avg: true, - colors: true, min_max: true, size: size(names), percentiles: true, @@ -253,23 +251,12 @@ pub mod reporter { let mut s = String::new(); s.push_str(&format!("{: s.push_str(stack), - true => s.push_str(&colors::gray(stack).to_string()), - } + s.push_str(&colors::gray(stack).to_string()); } s @@ -304,64 +291,36 @@ pub mod reporter { s.push_str(&format!("{:14}", - format!("{}/iter", fmt_duration(stats.avg)) - )); - s.push_str(&format!("{:>14}", avg_to_iter_per_s(stats.avg))); - } - if options.min_max { - s.push_str(&format!( - "{:>24}", - format!( - "({} … {})", - fmt_duration(stats.min), - fmt_duration(stats.max) - ) - )); - } - if options.percentiles { - s.push_str(&format!( - " {:>9} {:>9} {:>9}", - fmt_duration(stats.p75), - fmt_duration(stats.p99), - fmt_duration(stats.p995) - )); - } - } else { - if options.avg { - s.push_str(&format!( - "{:>30}", - format!("{}/iter", colors::yellow(fmt_duration(stats.avg))) - )); - s.push_str(&format!("{:>14}", avg_to_iter_per_s(stats.avg))); - } - if options.min_max { - s.push_str(&format!( - "{:>50}", - format!( - "({} … {})", - colors::cyan(fmt_duration(stats.min)), - colors::magenta(fmt_duration(stats.max)) - ) - )); - } - if options.percentiles { - s.push_str(&format!( - " {:>22} {:>22} {:>22}", - colors::magenta(fmt_duration(stats.p75)), - colors::magenta(fmt_duration(stats.p99)), - colors::magenta(fmt_duration(stats.p995)) - )); - } + if options.avg { + s.push_str(&format!( + "{:>30}", + format!("{}/iter", colors::yellow(fmt_duration(stats.avg))) + )); + s.push_str(&format!("{:>14}", avg_to_iter_per_s(stats.avg))); + } + if options.min_max { + s.push_str(&format!( + "{:>50}", + format!( + "({} … {})", + colors::cyan(fmt_duration(stats.min)), + colors::magenta(fmt_duration(stats.max)) + ) + )); + } + if options.percentiles { + s.push_str(&format!( + " {:>22} {:>22} {:>22}", + colors::magenta(fmt_duration(stats.p75)), + colors::magenta(fmt_duration(stats.p99)), + colors::magenta(fmt_duration(stats.p995)) + )); } s } - pub fn summary(benchmarks: &[GroupBenchmark], options: &Options) -> String { + pub fn summary(benchmarks: &[GroupBenchmark]) -> String { let mut s = String::new(); let mut benchmarks = benchmarks.to_owned(); benchmarks.sort_by(|a, b| a.stats.avg.partial_cmp(&b.stats.avg).unwrap()); @@ -370,58 +329,34 @@ pub mod reporter { .find(|b| b.baseline) .unwrap_or(&benchmarks[0]); - if !options.colors { - s.push_str(&format!("summary\n {}", baseline.name)); + s.push_str(&format!( + "{}\n {}", + colors::gray("summary"), + colors::cyan_bold(&baseline.name) + )); - for b in benchmarks.iter().filter(|b| *b != baseline) { - let faster = b.stats.avg >= baseline.stats.avg; - let diff = f64::from_str(&format!( - "{:.2}", - 1.0 / baseline.stats.avg * b.stats.avg - )) - .unwrap(); - let inv_diff = f64::from_str(&format!( - "{:.2}", - 1.0 / b.stats.avg * baseline.stats.avg - )) - .unwrap(); - s.push_str(&format!( - "\n {}x times {} than {}", - if faster { diff } else { inv_diff }, - if faster { "faster" } else { "slower" }, - b.name - )); - } - } else { + for b in benchmarks.iter().filter(|b| *b != baseline) { + let faster = b.stats.avg >= baseline.stats.avg; + let diff = f64::from_str(&format!( + "{:.2}", + 1.0 / baseline.stats.avg * b.stats.avg + )) + .unwrap(); + let inv_diff = f64::from_str(&format!( + "{:.2}", + 1.0 / b.stats.avg * baseline.stats.avg + )) + .unwrap(); s.push_str(&format!( - "{}\n {}", - colors::gray("summary"), - colors::cyan_bold(&baseline.name) + "\n {}x {} than {}", + if faster { + colors::green(diff.to_string()).to_string() + } else { + colors::red(inv_diff.to_string()).to_string() + }, + if faster { "faster" } else { "slower" }, + colors::cyan_bold(&b.name) )); - - for b in benchmarks.iter().filter(|b| *b != baseline) { - let faster = b.stats.avg >= baseline.stats.avg; - let diff = f64::from_str(&format!( - "{:.2}", - 1.0 / baseline.stats.avg * b.stats.avg - )) - .unwrap(); - let inv_diff = f64::from_str(&format!( - "{:.2}", - 1.0 / b.stats.avg * baseline.stats.avg - )) - .unwrap(); - s.push_str(&format!( - "\n {}x {} than {}", - if faster { - colors::green(diff.to_string()).to_string() - } else { - colors::red(inv_diff.to_string()).to_string() - }, - if faster { "faster" } else { "slower" }, - colors::cyan_bold(&b.name) - )); - } } s diff --git a/cli/tools/bench/reporters.rs b/cli/tools/bench/reporters.rs index face54943f..9cc035f8f1 100644 --- a/cli/tools/bench/reporters.rs +++ b/cli/tools/bench/reporters.rs @@ -138,7 +138,6 @@ impl BenchReporter for ConsoleReporter { let options = self.options.as_mut().unwrap(); options.percentiles = true; - options.colors = colors::use_color(); if FIRST_PLAN .compare_exchange(true, false, Ordering::SeqCst, Ordering::SeqCst) @@ -246,10 +245,9 @@ impl BenchReporter for ConsoleReporter { } fn report_group_summary(&mut self) { - let options = match self.options.as_ref() { - None => return, - Some(options) => options, - }; + if self.options.is_none() { + return; + } if 2 <= self.group_measurements.len() && (self.group.is_some() || (self.group.is_none() && self.baseline)) @@ -275,7 +273,6 @@ impl BenchReporter for ConsoleReporter { }, }) .collect::>(), - options ) ); }