From bbd0ac7ac12c00925980bda8a088c8492de88dba Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Apr 2022 13:53:51 +0200 Subject: [PATCH 1/2] Simplify artifact comparison relevance --- site/src/comparison.rs | 163 +++++++++++++---------------------------- site/src/github.rs | 88 +++++++--------------- 2 files changed, 76 insertions(+), 175 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index 39f64961f..a590623de 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -146,29 +146,31 @@ pub async fn handle_compare( async fn populate_report( ctxt: &SiteCtxt, comparison: &Comparison, - report: &mut HashMap, Vec>, + report: &mut HashMap>, ) { - if let Some(summary) = ComparisonSummary::summarize_comparison(comparison) { - let relevance = summary.relevance_level(); - if relevance.at_least_somewhat_relevant() { - if let Some(direction) = summary.direction() { - let entry = report - .entry(relevance.very_relevant().then(|| direction)) - .or_default(); - - entry.push(write_triage_summary(ctxt, comparison).await) - } - } - } + let benchmark_map = ctxt.get_benchmark_category_map().await; + let (primary, secondary) = comparison.clone().summarize_by_category(benchmark_map); + // Get the combined direction of the primary and secondary summaries + let direction = match (primary.direction(), secondary.direction()) { + (Some(d1), Some(d2)) if d1 != d2 => Direction::Mixed, + (Some(Direction::Improvement), Some(_) | None) => Direction::Improvement, + (Some(Direction::Regression), Some(_) | None) => Direction::Regression, + (Some(Direction::Mixed), Some(_) | None) => Direction::Mixed, + (None, Some(d)) => d, + (None, None) => return, + }; + + let entry = report.entry(direction).or_default(); + + entry.push(write_triage_summary(comparison, &primary, &secondary).await) } /// A summary of a given comparison /// /// This summary only includes changes that are significant and relevant (as determined by a change's magnitude). pub struct ComparisonSummary { - /// Significant comparisons of magnitude small and above - /// and ordered by magnitude from largest to smallest - comparisons: Vec, + /// Relevant comparisons ordered by magnitude from largest to smallest + relevant_comparisons: Vec, /// The cached number of comparisons that are improvements num_improvements: usize, /// The cached number of comparisons that are regressions @@ -176,7 +178,7 @@ pub struct ComparisonSummary { } impl ComparisonSummary { - pub fn summarize_comparison(comparison: &Comparison) -> Option { + pub fn summarize_comparison(comparison: &Comparison) -> Self { let mut num_improvements = 0; let mut num_regressions = 0; @@ -193,10 +195,6 @@ impl ComparisonSummary { }) .cloned() .collect::>(); - // Skip empty commits, sometimes happens if there's a compiler bug or so. - if comparisons.len() == 0 { - return None; - } let cmp = |b1: &TestResultComparison, b2: &TestResultComparison| { b2.relative_change() @@ -206,29 +204,23 @@ impl ComparisonSummary { }; comparisons.sort_by(cmp); - Some(ComparisonSummary { - comparisons, + ComparisonSummary { + relevant_comparisons: comparisons, num_improvements, num_regressions, - }) - } - - pub fn empty() -> Self { - ComparisonSummary { - comparisons: vec![], - num_improvements: 0, - num_regressions: 0, } } /// The direction of the changes pub fn direction(&self) -> Option { - if self.comparisons.len() == 0 { + if self.relevant_comparisons.len() == 0 { return None; } - let (regressions, improvements): (Vec<&TestResultComparison>, _) = - self.comparisons.iter().partition(|c| c.is_regression()); + let (regressions, improvements): (Vec<&TestResultComparison>, _) = self + .relevant_comparisons + .iter() + .partition(|c| c.is_regression()); if regressions.len() == 0 { return Some(Direction::Improvement); @@ -238,7 +230,7 @@ impl ComparisonSummary { return Some(Direction::Regression); } - let total_num = self.comparisons.len(); + let total_num = self.relevant_comparisons.len(); let regressions_ratio = regressions.len() as f64 / total_num as f64; let has_medium_and_above_regressions = regressions @@ -300,18 +292,11 @@ impl ComparisonSummary { /// Arithmetic mean of all changes as a percent pub fn arithmetic_mean_of_changes(&self) -> f64 { - self.arithmetic_mean(self.comparisons.iter()) - } - - pub fn num_significant_changes(&self) -> usize { - self.comparisons - .iter() - .filter(|c| c.is_significant()) - .count() + self.arithmetic_mean(self.relevant_comparisons.iter()) } pub fn is_empty(&self) -> bool { - self.comparisons.is_empty() + self.relevant_comparisons.is_empty() } fn arithmetic_mean<'a>( @@ -329,45 +314,38 @@ impl ComparisonSummary { } fn improvements(&self) -> impl Iterator { - self.comparisons.iter().filter(|c| c.is_improvement()) + self.relevant_comparisons + .iter() + .filter(|c| c.is_improvement()) } fn regressions(&self) -> impl Iterator { - self.comparisons.iter().filter(|c| c.is_regression()) + self.relevant_comparisons + .iter() + .filter(|c| c.is_regression()) } fn largest_improvement(&self) -> Option<&TestResultComparison> { - self.comparisons.iter().find(|s| s.is_improvement()) + self.relevant_comparisons + .iter() + .find(|s| s.is_improvement()) } fn largest_regression(&self) -> Option<&TestResultComparison> { - self.comparisons.iter().find(|s| s.is_regression()) + self.relevant_comparisons.iter().find(|s| s.is_regression()) } /// The relevance level of the entire comparison - pub fn relevance_level(&self) -> RelevanceLevel { - let mut num_small_changes = 0; - let mut num_medium_changes = 0; - for c in self.comparisons.iter() { - match c.magnitude() { - Magnitude::Small => num_small_changes += 1, - Magnitude::Medium => num_medium_changes += 1, - Magnitude::Large => return RelevanceLevel::High, - Magnitude::VeryLarge => return RelevanceLevel::High, - Magnitude::VerySmall => unreachable!(), - } - } - - match (num_small_changes, num_medium_changes) { - (_, m) if m > 1 => RelevanceLevel::High, - (_, 1) => RelevanceLevel::Medium, - (s, 0) if s > 10 => RelevanceLevel::Medium, - _ => RelevanceLevel::Low, - } + pub fn is_relevant(&self) -> bool { + !self.is_empty() } } -async fn write_triage_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> String { +async fn write_triage_summary( + comparison: &Comparison, + primary: &ComparisonSummary, + secondary: &ComparisonSummary, +) -> String { use std::fmt::Write; let mut result = if let Some(pr) = comparison.b.pr { let title = github::pr_title(pr).await; @@ -383,11 +361,6 @@ async fn write_triage_summary(ctxt: &SiteCtxt, comparison: &Comparison) -> Strin let link = &compare_link(start, end); write!(&mut result, " [(Comparison Link)]({})\n", link).unwrap(); - let benchmark_map = ctxt.get_benchmark_category_map().await; - let (primary, secondary) = comparison.clone().summarize_by_category(benchmark_map); - let primary = primary.unwrap_or_else(ComparisonSummary::empty); - let secondary = secondary.unwrap_or_else(ComparisonSummary::empty); - write_summary_table(&primary, &secondary, false, &mut result); result @@ -506,24 +479,6 @@ pub fn write_summary_table( } } -/// How relevant a set of test result comparisons are. -#[derive(Clone, Copy, Debug)] -pub enum RelevanceLevel { - Low, - Medium, - High, -} - -impl RelevanceLevel { - pub fn very_relevant(self) -> bool { - matches!(self, Self::High) - } - - pub fn at_least_somewhat_relevant(self) -> bool { - matches!(self, Self::High | Self::Medium) - } -} - /// Compare two bounds on a given stat /// /// Returns Ok(None) when no data for the end bound is present @@ -782,7 +737,7 @@ impl Comparison { pub fn summarize_by_category( self, category_map: HashMap, - ) -> (Option, Option) { + ) -> (ComparisonSummary, ComparisonSummary) { let (primary, secondary) = self .statistics .into_iter() @@ -1146,7 +1101,7 @@ impl Magnitude { async fn generate_report( start: &Bound, end: &Bound, - mut report: HashMap, Vec>, + mut report: HashMap>, num_comparisons: usize, ) -> String { fn fmt_bound(bound: &Bound) -> String { @@ -1158,14 +1113,9 @@ async fn generate_report( } let start = fmt_bound(start); let end = fmt_bound(end); - let regressions = report - .remove(&Some(Direction::Regression)) - .unwrap_or_default(); - let improvements = report - .remove(&Some(Direction::Improvement)) - .unwrap_or_default(); - let mixed = report.remove(&Some(Direction::Mixed)).unwrap_or_default(); - let unlabeled = report.remove(&None).unwrap_or_default(); + let regressions = report.remove(&Direction::Regression).unwrap_or_default(); + let improvements = report.remove(&Direction::Improvement).unwrap_or_default(); + let mixed = report.remove(&Direction::Mixed).unwrap_or_default(); let untriaged = match github::untriaged_perf_regressions().await { Ok(u) => u .iter() @@ -1205,15 +1155,6 @@ Revision range: [{first_commit}..{last_commit}](https://perf.rust-lang.org/?star {mixed} -#### Probably changed - -The following is a list of comparisons which *probably* represent real performance changes, -but we're not 100% sure. Please move things from this category into the categories -above for changes you think *are* definitely relevant and file an issue for each so that -we can consider how to change our heuristics. - -{unlabeled} - #### Untriaged Pull Requests {untriaged} @@ -1233,7 +1174,6 @@ TODO: Nags regressions = regressions.join("\n\n"), improvements = improvements.join("\n\n"), mixed = mixed.join("\n\n"), - unlabeled = unlabeled.join("\n\n"), untriaged = untriaged ) } @@ -1486,6 +1426,5 @@ mod tests { 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 a24b4ed1e..7650582a1 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -1,5 +1,5 @@ use crate::api::github::Issue; -use crate::comparison::{write_summary_table, ComparisonSummary, Direction, RelevanceLevel}; +use crate::comparison::{write_summary_table, ComparisonSummary, Direction}; use crate::load::{Config, SiteCtxt, TryCommit}; use anyhow::Context as _; @@ -562,13 +562,8 @@ async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_maste "https://perf.rust-lang.org/compare.html?start={}&end={}", commit.parent_sha, commit.sha ); - let (summary, direction) = categorize_benchmark( - ctxt, - commit.sha.clone(), - commit.parent_sha, - is_master_commit, - ) - .await; + let (summary, direction) = + categorize_benchmark(ctxt, commit.sha.clone(), commit.parent_sha).await; let body = format!( "Finished benchmarking commit ({sha}): [comparison url]({url}). @@ -652,7 +647,6 @@ async fn categorize_benchmark( ctxt: &SiteCtxt, commit_sha: String, parent_sha: String, - is_master_commit: bool, ) -> (String, Option) { let comparison = match crate::comparison::compare( collector::Bound::Commit(parent_sha), @@ -685,17 +679,15 @@ async fn categorize_benchmark( 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() { + if !primary.is_relevant() && !secondary.is_relevant() { return ( format!("This benchmark run did not return any relevant results.\n\n{footer}"), None, ); } - let (primary_short_summary, primary_direction) = - generate_short_summary(primary.as_ref(), is_master_commit); - let (secondary_short_summary, secondary_direction) = - generate_short_summary(secondary.as_ref(), is_master_commit); + let (primary_short_summary, primary_direction) = generate_short_summary(&primary); + let (secondary_short_summary, secondary_direction) = generate_short_summary(&secondary); let mut result = format!( r#" @@ -705,13 +697,7 @@ async fn categorize_benchmark( ); write!(result, "\n\n").unwrap(); - if primary_direction.is_some() || secondary_direction.is_some() { - let (primary, secondary) = ( - primary.unwrap_or_else(|| ComparisonSummary::empty()), - secondary.unwrap_or_else(|| ComparisonSummary::empty()), - ); - write_summary_table(&primary, &secondary, true, &mut result); - } + write_summary_table(&primary, &secondary, true, &mut result); write!(result, "\n{footer}").unwrap(); @@ -719,10 +705,7 @@ async fn categorize_benchmark( (result, direction) } -fn generate_short_summary( - summary: Option<&ComparisonSummary>, - is_master_commit: bool, -) -> (String, Option) { +fn generate_short_summary(summary: &ComparisonSummary) -> (String, Option) { // Add an "s" to a word unless there's only one. fn ending(word: &'static str, count: usize) -> std::borrow::Cow<'static, str> { if count == 1 { @@ -731,46 +714,25 @@ fn generate_short_summary( format!("{}s", word).into() } - match summary { - Some(summary) => { - if comparison_is_relevant(summary.relevance_level(), is_master_commit) { - let direction = summary.direction().unwrap(); - let num_improvements = summary.number_of_improvements(); - let num_regressions = summary.number_of_regressions(); - - let text = match direction { - Direction::Improvement => format!( - "🎉 relevant {} found", - ending("improvement", num_improvements) - ), - Direction::Regression => format!( - "😿 relevant {} found", - ending("regression", num_regressions) - ), - Direction::Mixed => "mixed results".to_string(), - }; - (text, Some(direction)) - } else { - ( - format!( - "changes not relevant. {} results were found to be statistically significant but the changes were too small to be relevant.", - summary.num_significant_changes(), - ), - None - ) - } - } - None => ("no relevant changes found".to_string(), None), - } -} + if summary.is_relevant() { + let direction = summary.direction().unwrap(); + let num_improvements = summary.number_of_improvements(); + let num_regressions = summary.number_of_regressions(); -/// Whether a comparison is relevant enough to show -fn comparison_is_relevant(relevance: RelevanceLevel, is_master_commit: bool) -> bool { - if is_master_commit { - relevance.very_relevant() + let text = match direction { + Direction::Improvement => format!( + "🎉 relevant {} found", + ending("improvement", num_improvements) + ), + Direction::Regression => format!( + "😿 relevant {} found", + ending("regression", num_regressions) + ), + Direction::Mixed => "mixed results".to_string(), + }; + (text, Some(direction)) } else { - // is try run - relevance.at_least_somewhat_relevant() + ("no relevant changes found".to_string(), None) } } From 4e505cf280d0602241def1f4eab9ce0b155bbce2 Mon Sep 17 00:00:00 2001 From: Ryan Levick Date: Wed, 6 Apr 2022 14:45:50 +0200 Subject: [PATCH 2/2] Simplify match expression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jakub Beránek --- site/src/comparison.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index a590623de..0ddd7658c 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -153,9 +153,7 @@ async fn populate_report( // Get the combined direction of the primary and secondary summaries let direction = match (primary.direction(), secondary.direction()) { (Some(d1), Some(d2)) if d1 != d2 => Direction::Mixed, - (Some(Direction::Improvement), Some(_) | None) => Direction::Improvement, - (Some(Direction::Regression), Some(_) | None) => Direction::Regression, - (Some(Direction::Mixed), Some(_) | None) => Direction::Mixed, + (Some(d), Some(_) | None) => d, (None, Some(d)) => d, (None, None) => return, };