Introduce confidence levels

Author: rylev

docs/comparison-analysis.md

@@ -22,13 +22,15 @@ 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:
 
-Are there 1 or more _significant_ test results that 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? 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.
 
-* What makes a test result significant?
+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".
+
+### What makes a test result significant?
 
 A test result is significant if the relative change percentage meets some threshold. What the threshold is depends of whether the test case is "dodgy" or not (see below for an examination of "dodginess"). For dodgy test cases, the threshold is set at 1%. For non-dodgy test cases, the threshold is set to 0.1%.
 
-* What makes a test case "dodgy"?
+### What makes a test case "dodgy"?
 
 A test case is "dodgy" if it shows signs of either being noisy or highly variable.
 
@@ -42,4 +44,14 @@ Any relative delta change that is above a threshold (currently 0.1) is considere
 
 A highly variable test case is one where a certain percentage (currently 5%) of relative delta changes are significant. The logic being that test cases should only display significant relative delta changes a small percentage of the time.
 
-A noisy test case is one where of all the non-significant relative delta changes, the average delta change is still above some threshold (0.001). The logic being that non-significant changes should, on average, being very close to 0. If they are not close to zero, then they are noisy.
+A noisy test case is one where of all the non-significant relative delta changes, the average delta change is still above some threshold (0.001). The logic being that non-significant changes should, on average, being very close to 0. If they are not close to zero, then they are noisy.
+
+### 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 
+
+Note: changes can be any combination of positive or negative changes.

site/src/comparison.rs

@@ -122,24 +122,30 @@ pub async fn handle_compare(
 
 async fn populate_report(comparison: &Comparison, report: &mut HashMap<Direction, Vec<String>>) {
     if let Some(summary) = ComparisonSummary::summarize_comparison(comparison) {
-        if let Some(direction) = summary.direction() {
-            let entry = report.entry(direction).or_default();
+        if summary.confidence().is_definitely_relevant() {
+            if let Some(direction) = summary.direction() {
+                let entry = report.entry(direction).or_default();
 
-            entry.push(summary.write(comparison).await)
+                entry.push(summary.write(comparison).await)
+            }
         }
     }
 }
 
 pub struct ComparisonSummary {
-    hi: Option<TestResultComparison>,
-    lo: Option<TestResultComparison>,
+    /// Significant comparisons
+    comparisons: Vec<TestResultComparison>,
 }
 
 impl ComparisonSummary {
     pub fn summarize_comparison(comparison: &Comparison) -> Option<ComparisonSummary> {
-        let mut benchmarks = comparison.get_benchmarks().collect::<Vec<_>>();
+        let mut comparisons = comparison
+            .get_benchmarks()
+            .filter(|c| c.is_significant())
+            .cloned()
+            .collect::<Vec<_>>();
         // Skip empty commits, sometimes happens if there's a compiler bug or so.
-        if benchmarks.len() == 0 {
+        if comparisons.len() == 0 {
             return None;
         }
 
@@ -148,27 +154,17 @@ impl ComparisonSummary {
                 .partial_cmp(&b2.log_change())
                 .unwrap_or(std::cmp::Ordering::Equal)
         };
-        let lo = benchmarks
-            .iter()
-            .enumerate()
-            .min_by(|&(_, b1), &(_, b2)| cmp(b1, b2))
-            .filter(|(_, c)| c.is_significant() && !c.is_increase())
-            .map(|(i, _)| i);
-        let lo = lo.map(|lo| benchmarks.remove(lo)).cloned();
-        let hi = benchmarks
-            .iter()
-            .enumerate()
-            .max_by(|&(_, b1), &(_, b2)| cmp(b1, b2))
-            .filter(|(_, c)| c.is_significant() && c.is_increase())
-            .map(|(i, _)| i);
-        let hi = hi.map(|hi| benchmarks.remove(hi)).cloned();
+        comparisons.sort_by(cmp);
 
-        Some(ComparisonSummary { hi, lo })
+        Some(ComparisonSummary { comparisons })
     }
 
     /// The direction of the changes
     pub fn direction(&self) -> Option<Direction> {
-        let d = match (&self.hi, &self.lo) {
+        let d = match (
+            self.largest_positive_change(),
+            &self.largest_negative_change(),
+        ) {
             (None, None) => return None,
             (Some(b), None) => b.direction(),
             (None, Some(b)) => b.direction(),
@@ -178,22 +174,31 @@ impl ComparisonSummary {
         Some(d)
     }
 
-    /// The changes ordered by their signficance (most significant first)
-    pub fn ordered_changes(&self) -> Vec<&TestResultComparison> {
-        match (&self.hi, &self.lo) {
-            (None, None) => Vec::new(),
-            (Some(b), None) => vec![b],
-            (None, Some(b)) => vec![b],
-            (Some(a), Some(b))
-                if b.log_change()
-                    .abs()
-                    .partial_cmp(&a.log_change().abs())
-                    .unwrap_or(std::cmp::Ordering::Equal)
-                    == std::cmp::Ordering::Greater =>
-            {
-                vec![b, a]
-            }
-            (Some(a), Some(b)) => vec![a, b],
+    /// 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 largest_positive_change(&self) -> Option<&TestResultComparison> {
+        self.comparisons.first().filter(|s| s.is_increase())
+    }
+
+    pub fn largest_negative_change(&self) -> Option<&TestResultComparison> {
+        self.comparisons.last().filter(|s| !s.is_increase())
+    }
+
+    pub fn confidence(&self) -> ComparisonConfidence {
+        match self.comparisons.len() {
+            0..=3 => ComparisonConfidence::MaybeRelevant,
+            4..=6 => ComparisonConfidence::ProbablyRelevant,
+            _ => ComparisonConfidence::DefinitelyRelevant,
         }
     }
 
@@ -213,14 +218,33 @@ impl ComparisonSummary {
         let end = &comparison.b.artifact;
         let link = &compare_link(start, end);
 
-        for change in self.ordered_changes() {
+        for change in self.largest_changes(2) {
             write!(result, "- ").unwrap();
             change.summary_line(&mut result, Some(link))
         }
         result
     }
 }
 
+/// The amount of confidence we have that a comparison actually represents a real
+/// change in the performance characteristics.
+#[derive(Clone, Copy)]
+pub enum ComparisonConfidence {
+    MaybeRelevant,
+    ProbablyRelevant,
+    DefinitelyRelevant,
+}
+
+impl ComparisonConfidence {
+    pub fn is_definitely_relevant(self) -> bool {
+        matches!(self, Self::DefinitelyRelevant)
+    }
+
+    pub fn is_atleast_probably_relevant(self) -> bool {
+        matches!(self, Self::DefinitelyRelevant | Self::ProbablyRelevant)
+    }
+}
+
 /// Compare two bounds on a given stat
 ///
 /// Returns Ok(None) when no data for the end bound is present

site/src/github.rs

@@ -606,7 +606,7 @@ 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 (summary, direction) = match ComparisonSummary::summarize_comparison(&comparison) {
-        Some(s) if s.direction().is_some() => {
+        Some(s) if s.confidence().is_atleast_probably_relevant() => {
             let direction = s.direction().unwrap();
             (s, direction)
         }
@@ -630,7 +630,7 @@ async fn categorize_benchmark(
         "This change led to significant {} in compiler performance.\n",
         category
     );
-    for change in summary.ordered_changes() {
+    for change in summary.largest_changes(2) {
         write!(result, "- ").unwrap();
         change.summary_line(&mut result, None)
     }