Take magnitude into account

Author: rylev

docs/comparison-analysis.md

@@ -22,7 +22,16 @@ At the core of comparison analysis are the collection of test results for the tw
 
 Analysis of the changes is performed in order to determine whether artifact B represents a performance change over artifact A. At a high level the analysis performed takes the following form:
 
-How many _significant_ test results indicate performance changes? If all significant test results indicate regressions (i.e., all percent relative changes are positive), then artifact B represents a performance regression over artifact A. If all significant test results indicate improvements (i.e., all percent relative changes are negative), then artifact B represents a performance improvement over artifact B. If some significant test results indicate improvement and others indicate regressions, then the performance change is mixed.
+How many _significant_ test results indicate performance changes and what is the magnitude of the changes (i.e., how large are the changes regardless of the direction of change)? 
+
+* If there are improvements and regressions with magnitude of medium or above then the comparison is mixed.
+* If there are only either improvements or regressions then the comparison is labeled with that kind.
+* If one kind of changes are of medium or above magnitude (and the other kind are not), then the comparison is mixed if 15% or more of the total changes are the other (small magnitude) kind. For example:
+  * given 20 regressions (with at least 1 of medium magnitude) and all improvements of low magnitude, the comparison is only mixed if there are 4 or more improvements.
+  * given 5 regressions (with at least 1 of medium magnitude) and all improvements of low magnitude, the comparison is only mixed if there 1 or more improvements.
+* If both kinds of changes are all low magnitude changes, then the comparison is mixed unless 90% or more of total changes are of one kind. For example:
+  * given 20 changes of different kinds all of low magnitude, the result is mixed unless only 2 or fewer of the changes are of one kind.
+  * given 5 changes of different kinds all of low magnitude, the result is always mixed.
 
 Whether we actually _report_ an analysis or not depends on the context and how _confident_ we are in the summary of the results (see below for an explanation of how confidence is derived). For example, in pull request performance "try" runs, we report a performance change if we are at least confident that the results are "probably relevant", while for the triage report, we only report if the we are confident the results are "definitely relevant".
 
@@ -48,10 +57,9 @@ A noisy test case is one where of all the non-significant relative delta changes
 
 ### How is confidence in whether a test analysis is "relevant" determined?
 
-The confidence in whether a test analysis is relevant depends on the number of significant test results. Depending on that number a confidence level is reached:
-
-* Maybe relevant: 0-3 changes 
-* Probably relevant: 4-6 changes 
-* Definitely relevant: >6 changes 
+The confidence in whether a test analysis is relevant depends on the number of significant test results and their magnitude (how large a change is regardless of the direction of the change).
 
-Note: changes can be any combination of positive or negative changes.
+The actual algorithm for determining confidence may change, but in general the following rules apply:
+* Definitely relevant: any number of very large changes, a small amount of large and/or medium changes, or a large amount of small changes.
+* Probably relevant: any number of large changes, more than 1 medium change, or smaller but still substantial amount of small changes.
+* Maybe relevant: if it doesn't fit into the above two categories, it ends in this category.

site/src/comparison.rs

@@ -133,7 +133,7 @@ async fn populate_report(comparison: &Comparison, report: &mut HashMap<Direction
 }
 
 pub struct ComparisonSummary {
-    /// Significant comparisons
+    /// Significant comparisons ordered by magnitude
     comparisons: Vec<TestResultComparison>,
 }
 
@@ -150,8 +150,8 @@ impl ComparisonSummary {
         }
 
         let cmp = |b1: &TestResultComparison, b2: &TestResultComparison| {
-            b1.log_change()
-                .partial_cmp(&b2.log_change())
+            b2.log_change()
+                .partial_cmp(&b1.log_change())
                 .unwrap_or(std::cmp::Ordering::Equal)
         };
         comparisons.sort_by(cmp);
@@ -161,41 +161,108 @@ impl ComparisonSummary {
 
     /// The direction of the changes
     pub fn direction(&self) -> Option<Direction> {
-        let d = match (self.largest_improvement(), self.largest_regression()) {
-            (None, None) => return None,
-            (Some(c), None) => c.direction(),
-            (None, Some(c)) => c.direction(),
-            (Some(a), Some(b)) if a.is_increase() == b.is_increase() => a.direction(),
-            _ => Direction::Mixed,
-        };
-        Some(d)
+        if self.comparisons.len() == 0 {
+            return None;
+        }
+
+        let (regressions, improvements): (Vec<&TestResultComparison>, _) =
+            self.comparisons.iter().partition(|c| c.is_regression());
+
+        if regressions.len() == 0 {
+            return Some(Direction::Improvement);
+        }
+
+        if improvements.len() == 0 {
+            return Some(Direction::Regression);
+        }
+
+        let total_num = self.comparisons.len();
+        let regressions_ratio = regressions.len() as f64 / total_num as f64;
+
+        let has_medium_and_above_regressions = regressions
+            .iter()
+            .any(|c| c.magnitude().is_medium_or_above());
+        let has_medium_and_above_improvements = improvements
+            .iter()
+            .any(|c| c.magnitude().is_medium_or_above());
+        match (
+            has_medium_and_above_improvements,
+            has_medium_and_above_regressions,
+        ) {
+            (true, true) => return Some(Direction::Mixed),
+            (true, false) => {
+                if regressions_ratio >= 0.15 {
+                    Some(Direction::Mixed)
+                } else {
+                    Some(Direction::Improvement)
+                }
+            }
+            (false, true) => {
+                if regressions_ratio < 0.85 {
+                    Some(Direction::Mixed)
+                } else {
+                    Some(Direction::Regression)
+                }
+            }
+            (false, false) => {
+                if regressions_ratio >= 0.1 && regressions_ratio <= 0.9 {
+                    Some(Direction::Mixed)
+                } else if regressions_ratio <= 0.1 {
+                    Some(Direction::Improvement)
+                } else {
+                    Some(Direction::Regression)
+                }
+            }
+        }
     }
 
-    /// The n largest changes ordered by the magnitude of their change
-    pub fn largest_changes(&self, n: usize) -> Vec<&TestResultComparison> {
-        let mut changes: Vec<&TestResultComparison> = self.comparisons.iter().take(n).collect();
-        changes.sort_by(|a, b| {
-            b.log_change()
-                .abs()
-                .partial_cmp(&a.log_change().abs())
-                .unwrap_or(std::cmp::Ordering::Equal)
-        });
-        changes
+    pub fn relevant_changes<'a>(&'a self) -> [Option<&TestResultComparison>; 2] {
+        match self.direction() {
+            Some(Direction::Improvement) => [self.largest_improvement(), None],
+            Some(Direction::Regression) => [self.largest_regression(), None],
+            Some(Direction::Mixed) => [self.largest_improvement(), self.largest_regression()],
+            None => [None, None],
+        }
     }
 
     pub fn largest_improvement(&self) -> Option<&TestResultComparison> {
-        self.comparisons.iter().filter(|s| !s.is_increase()).next()
+        self.comparisons
+            .iter()
+            .filter(|s| !s.is_regression())
+            .next()
     }
 
     pub fn largest_regression(&self) -> Option<&TestResultComparison> {
-        self.comparisons.iter().filter(|s| s.is_increase()).next()
+        self.comparisons.iter().filter(|s| s.is_regression()).next()
     }
 
     pub fn confidence(&self) -> ComparisonConfidence {
-        match self.comparisons.len() {
-            0..=3 => ComparisonConfidence::MaybeRelevant,
-            4..=6 => ComparisonConfidence::ProbablyRelevant,
-            _ => ComparisonConfidence::DefinitelyRelevant,
+        let mut num_small_changes = 0;
+        let mut num_medium_changes = 0;
+        let mut num_large_changes = 0;
+        let mut num_very_large_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 => num_large_changes += 1,
+                Magnitude::VeryLarge => num_very_large_changes += 1,
+            }
+        }
+
+        match (
+            num_medium_changes,
+            num_large_changes,
+            num_very_large_changes,
+        ) {
+            (_, _, vl) if vl > 0 => ComparisonConfidence::DefinitelyRelevant,
+            (m, l, _) if m + (l * 2) > 5 => ComparisonConfidence::DefinitelyRelevant,
+            (m, l, _) if m > 1 || l > 0 => ComparisonConfidence::ProbablyRelevant,
+            (m, _, _) => match m * 2 + num_small_changes {
+                0..=5 => ComparisonConfidence::MaybeRelevant,
+                6..=10 => ComparisonConfidence::ProbablyRelevant,
+                _ => ComparisonConfidence::DefinitelyRelevant,
+            },
         }
     }
 
@@ -215,7 +282,7 @@ impl ComparisonSummary {
         let end = &comparison.b.artifact;
         let link = &compare_link(start, end);
 
-        for change in self.largest_changes(2) {
+        for change in self.relevant_changes().iter().filter_map(|s| *s) {
             write!(result, "- ").unwrap();
             change.summary_line(&mut result, Some(link))
         }
@@ -666,7 +733,7 @@ impl TestResultComparison {
         (b / a).ln()
     }
 
-    fn is_increase(&self) -> bool {
+    fn is_regression(&self) -> bool {
         let (a, b) = self.results;
         b > a
     }
@@ -681,6 +748,19 @@ impl TestResultComparison {
         self.relative_change().abs() > threshold
     }
 
+    fn magnitude(&self) -> Magnitude {
+        let mag = self.relative_change().abs();
+        if mag < 0.01 {
+            Magnitude::Small
+        } else if mag < 0.04 {
+            Magnitude::Medium
+        } else if mag < 0.1 {
+            Magnitude::Large
+        } else {
+            Magnitude::VeryLarge
+        }
+    }
+
     fn is_dodgy(&self) -> bool {
         self.variance
             .as_ref()
@@ -703,24 +783,13 @@ impl TestResultComparison {
 
     pub fn summary_line(&self, summary: &mut String, link: Option<&str>) {
         use std::fmt::Write;
-        let magnitude = self.relative_change().abs();
-        let size = if magnitude > 0.10 {
-            "Very large"
-        } else if magnitude > 0.05 {
-            "Large"
-        } else if magnitude > 0.01 {
-            "Moderate"
-        } else if magnitude > 0.005 {
-            "Small"
-        } else {
-            "Very small"
-        };
+        let magnitude = self.magnitude();
 
         let percent = self.relative_change() * 100.0;
         write!(
             summary,
             "{} {} in {}",
-            size,
+            magnitude,
             self.direction(),
             match link {
                 Some(l) => format!("[instruction counts]({})", l),
@@ -736,13 +805,15 @@ impl TestResultComparison {
         .unwrap();
     }
 }
+
 impl std::cmp::PartialEq for TestResultComparison {
     fn eq(&self, other: &Self) -> bool {
         self.benchmark == other.benchmark
             && self.profile == other.profile
             && self.scenario == other.scenario
     }
 }
+
 impl std::cmp::Eq for TestResultComparison {}
 
 impl std::hash::Hash for TestResultComparison {
@@ -772,6 +843,33 @@ impl std::fmt::Display for Direction {
     }
 }
 
+/// The relative size of a performance change
+#[derive(Debug)]
+enum Magnitude {
+    Small,
+    Medium,
+    Large,
+    VeryLarge,
+}
+
+impl Magnitude {
+    fn is_medium_or_above(&self) -> bool {
+        !matches!(self, Self::Small)
+    }
+}
+
+impl std::fmt::Display for Magnitude {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        let s = match self {
+            Self::Small => "Small",
+            Self::Medium => "Moderate",
+            Self::Large => "Large",
+            Self::VeryLarge => "Very large",
+        };
+        f.write_str(s)
+    }
+}
+
 async fn generate_report(
     start: &Bound,
     end: &Bound,

site/src/github.rs

@@ -630,7 +630,7 @@ async fn categorize_benchmark(
         "This change led to significant {} in compiler performance.\n",
         category
     );
-    for change in summary.largest_changes(2) {
+    for change in summary.relevant_changes().iter().filter_map(|c| *c) {
         write!(result, "- ").unwrap();
         change.summary_line(&mut result, None)
     }