Be more rigorous and consistent about combining Directions.

Currently `Direction`s are combined in two places.
- `populate_report`, which uses a match. It gives the primary and
  secondary directions equal weight.
- `summarize_run`, which uses `Option::or`. It gives the primary
  direction more weight; the secondary direction will only be used if
  the primary direction is `None`.

The latter seems suboptimal. If, for example, there are only
improvements in the primary benchmarks and only regressions in the
secondary benchmarks, the overall result should be "mixed results" not
"improvements".

This commit makes both places use the combination from
`populate_report`. `Direction` forms a simple lattice and that
combination is the "join" operation. `Option` is no longer needed.

Author: nnethercote

site/src/comparison.rs

@@ -166,12 +166,10 @@ async fn populate_report(
 ) {
     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(d), Some(_) | None) => d,
-        (None, Some(d)) => d,
-        (None, None) => return,
-    };
+    let direction = Direction::join(primary.direction(), secondary.direction());
+    if direction == Direction::None {
+        return;
+    }
 
     let include_in_triage = deserves_attention_icount(&primary, &secondary);
 
@@ -338,9 +336,9 @@ impl ArtifactComparisonSummary {
     }
 
     /// The direction of the changes
-    pub fn direction(&self) -> Option<Direction> {
+    pub fn direction(&self) -> Direction {
         if self.relevant_comparisons.len() == 0 {
-            return None;
+            return Direction::None;
         }
 
         let (regressions, improvements): (Vec<&TestResultComparison>, _) = self
@@ -349,11 +347,11 @@ impl ArtifactComparisonSummary {
             .partition(|c| c.is_regression());
 
         if regressions.len() == 0 {
-            return Some(Direction::Improvement);
+            return Direction::Improvement;
         }
 
         if improvements.len() == 0 {
-            return Some(Direction::Regression);
+            return Direction::Regression;
         }
 
         let total_num = self.relevant_comparisons.len();
@@ -369,28 +367,28 @@ impl ArtifactComparisonSummary {
             has_medium_and_above_improvements,
             has_medium_and_above_regressions,
         ) {
-            (true, true) => return Some(Direction::Mixed),
+            (true, true) => return Direction::Mixed,
             (true, false) => {
                 if regressions_ratio >= 0.15 {
-                    Some(Direction::Mixed)
+                    Direction::Mixed
                 } else {
-                    Some(Direction::Improvement)
+                    Direction::Improvement
                 }
             }
             (false, true) => {
                 if regressions_ratio < 0.85 {
-                    Some(Direction::Mixed)
+                    Direction::Mixed
                 } else {
-                    Some(Direction::Regression)
+                    Direction::Regression
                 }
             }
             (false, false) => {
                 if regressions_ratio >= 0.1 && regressions_ratio <= 0.9 {
-                    Some(Direction::Mixed)
+                    Direction::Mixed
                 } else if regressions_ratio <= 0.1 {
-                    Some(Direction::Improvement)
+                    Direction::Improvement
                 } else {
-                    Some(Direction::Regression)
+                    Direction::Regression
                 }
             }
         }
@@ -1222,19 +1220,30 @@ impl std::hash::Hash for TestResultComparison {
 // The direction of a performance change
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
 pub enum Direction {
+    None,
     Improvement,
     Regression,
     Mixed,
 }
 
-impl std::fmt::Display for Direction {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let description = match self {
-            Direction::Improvement => "improvement",
-            Direction::Regression => "regression",
-            Direction::Mixed => "mixed",
-        };
-        write!(f, "{}", description)
+// The direction of a performance change. Forms a lattice:
+//
+//          Mixed
+//          /    \
+// Improvement  Regression
+//          \    /
+//           None
+//
+impl Direction {
+    // Also known as the "least upper bound".
+    pub fn join(a: Self, b: Self) -> Self {
+        match (a, b) {
+            (Self::None, b) => b,
+            (a, Self::None) => a,
+            (Self::Improvement, Self::Improvement) => Self::Improvement,
+            (Self::Regression, Self::Regression) => Self::Regression,
+            _ => Self::Mixed,
+        }
     }
 }
 

site/src/github/comparison_summary.rs

@@ -177,7 +177,7 @@ async fn summarize_run(
     please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new).";
     let footer = format!("{DISAGREEMENT}{errors}");
 
-    let direction = inst_primary.direction().or(inst_secondary.direction());
+    let direction = Direction::join(inst_primary.direction(), inst_secondary.direction());
     let next_steps = next_steps(inst_primary, inst_secondary, direction, is_master_commit);
 
     write!(&mut message, "\n{footer}\n{next_steps}").unwrap();
@@ -227,12 +227,12 @@ fn write_metric_summary(
 fn next_steps(
     primary: ArtifactComparisonSummary,
     secondary: ArtifactComparisonSummary,
-    direction: Option<Direction>,
+    direction: Direction,
     is_master_commit: bool,
 ) -> String {
     let deserves_attention = deserves_attention_icount(&primary, &secondary);
     let (is_regression, label) = match (deserves_attention, direction) {
-        (true, Some(Direction::Regression | Direction::Mixed)) => (true, "+perf-regression"),
+        (true, Direction::Regression | Direction::Mixed) => (true, "+perf-regression"),
         _ => (false, "-perf-regression"),
     };
 
@@ -303,15 +303,15 @@ fn generate_short_summary(summary: &ArtifactComparisonSummary) -> String {
     let num_regressions = summary.number_of_regressions();
 
     match summary.direction() {
-        Some(Direction::Improvement) => format!(
+        Direction::Improvement => format!(
             "✅ relevant {} found",
             ending("improvement", num_improvements)
         ),
-        Some(Direction::Regression) => format!(
+        Direction::Regression => format!(
             "❌ relevant {} found",
             ending("regression", num_regressions)
         ),
-        Some(Direction::Mixed) => "mixed results".to_string(),
-        None => "no relevant changes found".to_string(),
+        Direction::Mixed => "mixed results".to_string(),
+        Direction::None => "no relevant changes found".to_string(),
     }
 }