From e312fa0d09d96a872a0e9c8844f14c5b814ef652 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Apr 2022 10:54:07 +0200 Subject: [PATCH 1/2] Always show errors when present Previously when there were no relevant changes to show, we didn't show benchmark errors. However, it always makes sense to show new benchmark errors. This change makes it so that new benchmark errors are always shown when present. --- site/src/comparison.rs | 27 ++++++++++----------------- site/src/github.rs | 39 ++++++++++++++++++--------------------- 2 files changed, 28 insertions(+), 38 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index a40050bd3..40b942f8b 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -125,7 +125,10 @@ pub async fn handle_compare( }) .collect(); - let mut new_errors = comparison.new_errors.into_iter().collect::>(); + let mut new_errors = comparison + .newly_failed_benchmarks + .into_iter() + .collect::>(); new_errors.sort(); Ok(api::comparison::Response { prev, @@ -172,8 +175,6 @@ pub struct ComparisonSummary { num_improvements: usize, /// The cached number of comparisons that are regressions num_regressions: usize, - /// Which benchmarks had errors - errors_in: Vec, } impl ComparisonSummary { @@ -207,13 +208,10 @@ impl ComparisonSummary { }; comparisons.sort_by(cmp); - let errors_in = comparison.new_errors.keys().cloned().collect::>(); - Some(ComparisonSummary { comparisons, num_improvements, num_regressions, - errors_in, }) } @@ -222,7 +220,6 @@ impl ComparisonSummary { comparisons: vec![], num_improvements: 0, num_regressions: 0, - errors_in: vec![], } } @@ -319,10 +316,6 @@ impl ComparisonSummary { self.comparisons.is_empty() } - pub fn errors_in(&self) -> &[String] { - &self.errors_in - } - fn arithmetic_mean<'a>( &'a self, changes: impl Iterator, @@ -607,7 +600,7 @@ async fn compare_given_commits( a: ArtifactDescription::for_artifact(&*conn, a.clone(), master_commits).await, b: ArtifactDescription::for_artifact(&*conn, b.clone(), master_commits).await, statistics, - new_errors: errors_in_b.into_iter().collect(), + newly_failed_benchmarks: errors_in_b.into_iter().collect(), })) } @@ -755,7 +748,7 @@ pub struct Comparison { /// Statistics based on test case pub statistics: HashSet, /// A map from benchmark name to an error which occured when building `b` but not `a`. - pub new_errors: HashMap, + pub newly_failed_benchmarks: HashMap, } impl Comparison { @@ -798,7 +791,7 @@ impl Comparison { .partition(|s| category_map.get(&s.benchmark()) == Some(&Category::Primary)); let (primary_errors, secondary_errors) = self - .new_errors + .newly_failed_benchmarks .into_iter() .partition(|(b, _)| category_map.get(&b.as_str().into()) == Some(&Category::Primary)); @@ -806,13 +799,13 @@ impl Comparison { a: self.a.clone(), b: self.b.clone(), statistics: primary, - new_errors: primary_errors, + newly_failed_benchmarks: primary_errors, }; let secondary = Comparison { a: self.a, b: self.b, statistics: secondary, - new_errors: secondary_errors, + newly_failed_benchmarks: secondary_errors, }; ( ComparisonSummary::summarize_comparison(&primary), @@ -1516,7 +1509,7 @@ mod tests { bootstrap_total: 0, }, statistics, - new_errors: Default::default(), + newly_failed_benchmarks: Default::default(), }; ComparisonSummary::summarize_comparison(&comparison) .unwrap_or_else(|| ComparisonSummary::empty()) diff --git a/site/src/github.rs b/site/src/github.rs index 9e8cfc6e5..d1d187814 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -666,6 +666,21 @@ async fn categorize_benchmark( _ => return (String::from("ERROR categorizing benchmark run!"), None), }; + let errors = if !comparison.newly_failed_benchmarks.is_empty() { + let benchmarks = comparison + .newly_failed_benchmarks + .iter() + .map(|(benchmark, _)| format!("- {benchmark}")) + .collect::>() + .join("\n"); + format!( + "\n**Warning ⚠**: The following benchmark(s) failed to build:\n{}\n", + benchmarks, + ) + } else { + String::new() + }; + let benchmark_map = ctxt.get_benchmark_category_map().await; let (primary, secondary) = comparison.summarize_by_category(benchmark_map); @@ -675,8 +690,8 @@ async fn categorize_benchmark( if primary.is_none() && secondary.is_none() { return ( format!( - "This benchmark run did not return any relevant results.\n\n{}", - DISAGREEMENT + "This benchmark run did not return any relevant results.\n\n{}{}", + DISAGREEMENT, errors ), None, ); @@ -701,27 +716,9 @@ async fn categorize_benchmark( secondary.unwrap_or_else(|| ComparisonSummary::empty()), ); write_summary_table(&primary, &secondary, true, &mut result); - - if !primary.errors_in().is_empty() || !secondary.errors_in().is_empty() { - let list_errored_benchmarks = |summary: ComparisonSummary| { - summary - .errors_in() - .iter() - .map(|benchmark| format!("- {benchmark}")) - .collect::>() - .join("\n") - }; - write!( - result, - "\nThe following benchmark(s) failed to build:\n{}{}\n", - list_errored_benchmarks(primary), - list_errored_benchmarks(secondary) - ) - .unwrap(); - } } - write!(result, "\n{}", DISAGREEMENT).unwrap(); + write!(result, "\n{} {}", DISAGREEMENT, errors).unwrap(); let direction = primary_direction.or(secondary_direction); (result, direction) From e08d1fb0b99cc46a9d5e5d1e3ea5012fa7ea63ef Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Tue, 5 Apr 2022 11:35:30 +0200 Subject: [PATCH 2/2] Combine DISAGREEMENT and errors into footer --- site/src/github.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/site/src/github.rs b/site/src/github.rs index d1d187814..a24b4ed1e 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -673,10 +673,7 @@ async fn categorize_benchmark( .map(|(benchmark, _)| format!("- {benchmark}")) .collect::>() .join("\n"); - format!( - "\n**Warning ⚠**: The following benchmark(s) failed to build:\n{}\n", - benchmarks, - ) + format!("\n**Warning ⚠**: The following benchmark(s) failed to build:\n{benchmarks}\n") } else { String::new() }; @@ -686,13 +683,11 @@ async fn categorize_benchmark( const DISAGREEMENT: &str = "If you disagree with this performance assessment, \ please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new)."; + let footer = format!("{DISAGREEMENT}{errors}"); if primary.is_none() && secondary.is_none() { return ( - format!( - "This benchmark run did not return any relevant results.\n\n{}{}", - DISAGREEMENT, errors - ), + format!("This benchmark run did not return any relevant results.\n\n{footer}"), None, ); } @@ -718,7 +713,7 @@ async fn categorize_benchmark( write_summary_table(&primary, &secondary, true, &mut result); } - write!(result, "\n{} {}", DISAGREEMENT, errors).unwrap(); + write!(result, "\n{footer}").unwrap(); let direction = primary_direction.or(secondary_direction); (result, direction)