From c428e790cc435b800280215c4edd3a299bac62de Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Sun, 25 Jul 2021 23:56:51 +0800 Subject: [PATCH 1/8] Add basic github action for testing against rust-1.39 + stable + nightly. --- .github/workflows/ci.yaml | 42 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 .github/workflows/ci.yaml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml new file mode 100644 index 00000000..35ee2e37 --- /dev/null +++ b/.github/workflows/ci.yaml @@ -0,0 +1,42 @@ +on: [push, pull_request] + +name: tests + +jobs: + ci: + runs-on: ubuntu-latest + strategy: + matrix: + rust: + - stable + - beta + - nightly + - 1.39.0 # MSRV + + steps: + - uses: actions/checkout@v2 + + - uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: ${{ matrix.rust }} + override: true + components: rustfmt, clippy + + - uses: actions-rs/cargo@v1 + with: + command: build + + - uses: actions-rs/cargo@v1 + with: + command: test + + - uses: actions-rs/cargo@v1 + with: + command: fmt + args: --all -- --check + + - uses: actions-rs/cargo@v1 + with: + command: clippy + args: -- -D warnings From 715e62b95a1f27ef170d8deeea7614ff9df2d99f Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 00:12:33 +0800 Subject: [PATCH 2/8] Update MSRV to 1.40 --- .github/workflows/ci.yaml | 2 +- README.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 35ee2e37..862d7e3d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -11,7 +11,7 @@ jobs: - stable - beta - nightly - - 1.39.0 # MSRV + - 1.40.0 # MSRV steps: - uses: actions/checkout@v2 diff --git a/README.md b/README.md index bb5bbabd..7d9c3e44 100644 --- a/README.md +++ b/README.md @@ -117,9 +117,9 @@ For more details, see the [CONTRIBUTING.md file](https://github.com/bheisler/cri ### Compatibility Policy Criterion.rs supports the last three stable minor releases of Rust. At time of -writing, this means Rust 1.40 or later. Older versions may work, but are not tested or guaranteed. +writing, this means Rust 1.50 or later. Older versions may work, but are not guaranteed. -Currently, the oldest version of Rust believed to work is 1.39. Future versions of Criterion.rs may +Currently, the oldest version of Rust believed to work is 1.40. Future versions of Criterion.rs may break support for such old versions, and this will not be considered a breaking change. If you require Criterion.rs to work on old versions of Rust, you will need to stick to a specific patch version of Criterion.rs. From d2bfc91a572f6592a36e75b4b0eadc54a0e57546 Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 00:20:48 +0800 Subject: [PATCH 3/8] Use &Path rather than &PathBuf, let panic!() take care of formatting. --- src/analysis/mod.rs | 4 ++-- src/plot/gnuplot_backend/mod.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index 1c072e1c..d5b2f958 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -53,10 +53,10 @@ pub(crate) fn common( &criterion.baseline_directory, &criterion.output_directory, ) { - panic!(format!( + panic!( "Baseline '{base}' must exist before comparison is allowed; try --save-baseline {base}", base=criterion.baseline_directory, - )); + ); } } diff --git a/src/plot/gnuplot_backend/mod.rs b/src/plot/gnuplot_backend/mod.rs index e902d47f..95e07efc 100644 --- a/src/plot/gnuplot_backend/mod.rs +++ b/src/plot/gnuplot_backend/mod.rs @@ -1,5 +1,5 @@ use std::iter; -use std::path::PathBuf; +use std::path::Path; use std::process::Child; use crate::stats::univariate::Sample; @@ -40,9 +40,9 @@ const DARK_BLUE: Color = Color::Rgb(31, 120, 180); const DARK_ORANGE: Color = Color::Rgb(255, 127, 0); const DARK_RED: Color = Color::Rgb(227, 26, 28); -fn debug_script(path: &PathBuf, figure: &Figure) { +fn debug_script(path: &Path, figure: &Figure) { if crate::debug_enabled() { - let mut script_path = path.clone(); + let mut script_path = path.to_path_buf(); script_path.set_extension("gnuplot"); info!("Writing gnuplot script to {:?}", script_path); let result = figure.save(script_path.as_path()); From 320f10dcac4343ff63f5c69849270d66cebf84a5 Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 00:27:20 +0800 Subject: [PATCH 4/8] Don't build with 'beta'. --- .github/workflows/ci.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 862d7e3d..41565d75 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -9,7 +9,6 @@ jobs: matrix: rust: - stable - - beta - nightly - 1.40.0 # MSRV From 324ba60277954a93254f7bbde014085ab151c83d Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 00:38:05 +0800 Subject: [PATCH 5/8] Revert "Don't build with 'beta'." This reverts commit 320f10dcac4343ff63f5c69849270d66cebf84a5. --- .github/workflows/ci.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 41565d75..862d7e3d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -9,6 +9,7 @@ jobs: matrix: rust: - stable + - beta - nightly - 1.40.0 # MSRV From 34a742e6b427588f15c2f5784e0bdd8b3c1a69f9 Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 00:38:32 +0800 Subject: [PATCH 6/8] Apply a bunch of clippy lints. --- src/analysis/mod.rs | 2 +- src/benchmark_group.rs | 2 +- src/error.rs | 1 + src/html/mod.rs | 30 ++++++++++++++-------------- src/plot/gnuplot_backend/pdf.rs | 2 +- src/plot/gnuplot_backend/summary.rs | 6 +++--- src/plot/plotters_backend/pdf.rs | 2 +- src/plot/plotters_backend/summary.rs | 6 +++--- src/report.rs | 2 +- src/stats/bivariate/mod.rs | 4 ++-- 10 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/analysis/mod.rs b/src/analysis/mod.rs index d5b2f958..5d84bef1 100644 --- a/src/analysis/mod.rs +++ b/src/analysis/mod.rs @@ -278,7 +278,7 @@ fn regression( ) .0; - let point = Slope::fit(&data); + let point = Slope::fit(data); let (lb, ub) = distribution.confidence_interval(config.confidence_level); let se = distribution.std_dev(None); diff --git a/src/benchmark_group.rs b/src/benchmark_group.rs index ecb8258a..723d01e5 100644 --- a/src/benchmark_group.rs +++ b/src/benchmark_group.rs @@ -345,7 +345,7 @@ impl<'a, M: Measurement> BenchmarkGroup<'a, M> { func.profile( &self.criterion.measurement, &id, - &self.criterion, + self.criterion, &report_context, duration, input, diff --git a/src/error.rs b/src/error.rs index c1ecf763..9b7eb17b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -5,6 +5,7 @@ use std::fmt; use std::io; use std::path::PathBuf; +#[allow(clippy::enum_variant_names)] #[derive(Debug)] pub enum Error { AccessError { diff --git a/src/html/mod.rs b/src/html/mod.rs index 02ff0de7..11abe7ad 100644 --- a/src/html/mod.rs +++ b/src/html/mod.rs @@ -218,12 +218,12 @@ impl<'a> BenchmarkGroup<'a> { // numerically. Otherwise sort lexicographically. if values.iter().all(|os| parse_opt(os).is_some()) { values.sort_unstable_by(|v1, v2| { - let num1 = parse_opt(&v1); - let num2 = parse_opt(&v2); + let num1 = parse_opt(v1); + let num2 = parse_opt(v2); num1.partial_cmp(&num2).unwrap_or(Ordering::Less) }); - values.dedup_by_key(|os| parse_opt(&os).unwrap()); + values.dedup_by_key(|os| parse_opt(os).unwrap()); } else { values.sort_unstable(); values.dedup(); @@ -438,14 +438,14 @@ impl Report for Html { // If all of the value strings can be parsed into a number, sort/dedupe // numerically. Otherwise sort lexicographically. - if value_strs.iter().all(|os| try_parse(&*os).is_some()) { + if value_strs.iter().all(|os| try_parse(*os).is_some()) { value_strs.sort_unstable_by(|v1, v2| { - let num1 = try_parse(&v1); - let num2 = try_parse(&v2); + let num1 = try_parse(v1); + let num2 = try_parse(v2); num1.partial_cmp(&num2).unwrap_or(Ordering::Less) }); - value_strs.dedup_by_key(|os| try_parse(&os).unwrap()); + value_strs.dedup_by_key(|os| try_parse(os).unwrap()); } else { value_strs.sort_unstable(); value_strs.dedup(); @@ -455,7 +455,7 @@ impl Report for Html { let samples_with_function: Vec<_> = data .iter() .by_ref() - .filter(|&&(ref id, _)| id.function_id.as_ref() == Some(&function_id)) + .filter(|&&(id, _)| id.function_id.as_ref() == Some(function_id)) .collect(); if samples_with_function.len() > 1 { @@ -476,7 +476,7 @@ impl Report for Html { let samples_with_value: Vec<_> = data .iter() .by_ref() - .filter(|&&(ref id, _)| id.value_str.as_ref() == Some(&value_str)) + .filter(|&&(id, _)| id.value_str.as_ref() == Some(value_str)) .collect(); if samples_with_value.len() > 1 { @@ -497,7 +497,7 @@ impl Report for Html { // First sort the ids/data by value. // If all of the value strings can be parsed into a number, sort/dedupe // numerically. Otherwise sort lexicographically. - let all_values_numeric = all_data.iter().all(|(ref id, _)| { + let all_values_numeric = all_data.iter().all(|(id, _)| { id.value_str .as_ref() .map(String::as_str) @@ -577,7 +577,7 @@ impl Html { if !different_mean { explanation_str = "No change in performance detected.".to_owned(); } else { - let comparison = compare_to_threshold(&mean_est, comp.noise_threshold); + let comparison = compare_to_threshold(mean_est, comp.noise_threshold); match comparison { ComparisonResult::Improved => { explanation_str = "Performance has improved.".to_owned(); @@ -689,7 +689,7 @@ impl Html { fs::mkdirp(&both_dir) }); - let comp_data = plot_data.comparison(&comp); + let comp_data = plot_data.comparison(comp); self.plotter.borrow_mut().pdf(plot_ctx, comp_data); self.plotter.borrow_mut().pdf(plot_ctx_small, comp_data); @@ -767,12 +767,12 @@ impl Html { self.plotter.borrow_mut().violin(plot_ctx, formatter, data); - let value_types: Vec<_> = data.iter().map(|&&(ref id, _)| id.value_type()).collect(); + let value_types: Vec<_> = data.iter().map(|&&(id, _)| id.value_type()).collect(); let mut line_path = None; if value_types.iter().all(|x| x == &value_types[0]) { if let Some(value_type) = value_types[0] { - let values: Vec<_> = data.iter().map(|&&(ref id, _)| id.as_number()).collect(); + let values: Vec<_> = data.iter().map(|&&(id, _)| id.as_number()).collect(); if values.iter().any(|x| x != &values[0]) { self.plotter .borrow_mut() @@ -785,7 +785,7 @@ impl Html { let path_prefix = if full_summary { "../.." } else { "../../.." }; let benchmarks = data .iter() - .map(|&&(ref id, _)| { + .map(|&&(id, _)| { IndividualBenchmark::from_id(&report_context.output_directory, path_prefix, id) }) .collect(); diff --git a/src/plot/gnuplot_backend/pdf.rs b/src/plot/gnuplot_backend/pdf.rs index 3ee23602..a0b85c7a 100644 --- a/src/plot/gnuplot_backend/pdf.rs +++ b/src/plot/gnuplot_backend/pdf.rs @@ -33,7 +33,7 @@ pub(crate) fn pdf( format!("Iterations (x 10^{})", exponent) }; - let (xs, ys) = kde::sweep(&scaled_avg_times, KDE_POINTS, None); + let (xs, ys) = kde::sweep(scaled_avg_times, KDE_POINTS, None); let (lost, lomt, himt, hist) = avg_times.fences(); let mut fences = [lost, lomt, himt, hist]; let _ = formatter.scale_values(typical, &mut fences); diff --git a/src/plot/gnuplot_backend/summary.rs b/src/plot/gnuplot_backend/summary.rs index e9443609..d57a1749 100644 --- a/src/plot/gnuplot_backend/summary.rs +++ b/src/plot/gnuplot_backend/summary.rs @@ -83,9 +83,9 @@ pub fn line_comparison( // This assumes the curves are sorted. It also assumes that the benchmark IDs all have numeric // values or throughputs and that value is sensible (ie. not a mix of bytes and elements // or whatnot) - for (key, group) in &all_curves.iter().group_by(|&&&(ref id, _)| &id.function_id) { + for (key, group) in &all_curves.iter().group_by(|&&&(id, _)| &id.function_id) { let mut tuples: Vec<_> = group - .map(|&&(ref id, ref sample)| { + .map(|&&(id, ref sample)| { // Unwrap is fine here because it will only fail if the assumptions above are not true // ie. programmer error. let x = id.as_number().unwrap(); @@ -185,7 +185,7 @@ pub fn violin( positions: tics(), labels: all_curves .iter() - .map(|&&(ref id, _)| gnuplot_escape(id.as_title())), + .map(|&&(id, _)| gnuplot_escape(id.as_title())), }) }); diff --git a/src/plot/plotters_backend/pdf.rs b/src/plot/plotters_backend/pdf.rs index 1297b796..333893fc 100644 --- a/src/plot/plotters_backend/pdf.rs +++ b/src/plot/plotters_backend/pdf.rs @@ -197,7 +197,7 @@ pub(crate) fn pdf( format!("Iterations (x 10^{})", exponent) }; - let (xs, ys) = kde::sweep(&scaled_avg_times, KDE_POINTS, None); + let (xs, ys) = kde::sweep(scaled_avg_times, KDE_POINTS, None); let (lost, lomt, himt, hist) = avg_times.fences(); let mut fences = [lost, lomt, himt, hist]; let _ = formatter.scale_values(typical, &mut fences); diff --git a/src/plot/plotters_backend/summary.rs b/src/plot/plotters_backend/summary.rs index 7c5ca422..dad8a5bf 100644 --- a/src/plot/plotters_backend/summary.rs +++ b/src/plot/plotters_backend/summary.rs @@ -132,9 +132,9 @@ fn line_comparison_series_data<'a>( // This assumes the curves are sorted. It also assumes that the benchmark IDs all have numeric // values or throughputs and that value is sensible (ie. not a mix of bytes and elements // or whatnot) - for (key, group) in &all_curves.iter().group_by(|&&&(ref id, _)| &id.function_id) { + for (key, group) in &all_curves.iter().group_by(|&&&(id, _)| &id.function_id) { let mut tuples: Vec<_> = group - .map(|&&(ref id, ref sample)| { + .map(|&&(id, ref sample)| { // Unwrap is fine here because it will only fail if the assumptions above are not true // ie. programmer error. let x = id.as_number().unwrap(); @@ -164,7 +164,7 @@ pub fn violin( let mut kdes = all_curves .iter() - .map(|&&(ref id, ref sample)| { + .map(|&&(id, ref sample)| { let (x, mut y) = kde::sweep(Sample::new(sample), KDE_POINTS, None); let y_max = Sample::new(&y).max(); for y in y.iter_mut() { diff --git a/src/report.rs b/src/report.rs index 9148a9ea..60a144a2 100644 --- a/src/report.rs +++ b/src/report.rs @@ -609,7 +609,7 @@ impl Report for CliReport { if !different_mean { explanation_str = "No change in performance detected.".to_owned(); } else { - let comparison = compare_to_threshold(&mean_est, comp.noise_threshold); + let comparison = compare_to_threshold(mean_est, comp.noise_threshold); match comparison { ComparisonResult::Improved => { point_estimate_str = self.green(self.bold(point_estimate_str)); diff --git a/src/stats/bivariate/mod.rs b/src/stats/bivariate/mod.rs index b233b60c..d1e8df70 100644 --- a/src/stats/bivariate/mod.rs +++ b/src/stats/bivariate/mod.rs @@ -97,12 +97,12 @@ where /// Returns a view into the `X` data pub fn x(&self) -> &'a Sample { - Sample::new(&self.0) + Sample::new(self.0) } /// Returns a view into the `Y` data pub fn y(&self) -> &'a Sample { - Sample::new(&self.1) + Sample::new(self.1) } } From 7db3732a8d7e2e49b99acf262ecfcd6ad0e131eb Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 00:56:18 +0800 Subject: [PATCH 7/8] Only run 'fmt' on stable. --- .github/workflows/ci.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 862d7e3d..d06fa931 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -32,6 +32,7 @@ jobs: command: test - uses: actions-rs/cargo@v1 + if: ${{ matrix.rust == 'stable' }} with: command: fmt args: --all -- --check From ae96ce645de116dbc413c1654301167ee650a2e3 Mon Sep 17 00:00:00 2001 From: David Himmelstrup Date: Mon, 26 Jul 2021 01:03:33 +0800 Subject: [PATCH 8/8] Caching, limit hints on versions older than stable, don't run ci on every push. --- .github/workflows/ci.yaml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d06fa931..2a451fbe 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1,4 +1,10 @@ -on: [push, pull_request] +on: + push: + branches: + - master + pull_request: + branches: + - master name: tests @@ -23,6 +29,8 @@ jobs: override: true components: rustfmt, clippy + - uses: Swatinem/rust-cache@v1 + - uses: actions-rs/cargo@v1 with: command: build @@ -38,6 +46,7 @@ jobs: args: --all -- --check - uses: actions-rs/cargo@v1 + if: ${{ matrix.rust != '1.40.0' }} # 1.40 has horrible lints. with: command: clippy args: -- -D warnings