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..a24b4ed1e 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -666,18 +666,28 @@ 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{benchmarks}\n") + } else { + String::new() + }; + let benchmark_map = ctxt.get_benchmark_category_map().await; let (primary, secondary) = comparison.summarize_by_category(benchmark_map); 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 - ), + format!("This benchmark run did not return any relevant results.\n\n{footer}"), None, ); } @@ -701,27 +711,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{footer}").unwrap(); let direction = primary_direction.or(secondary_direction); (result, direction)