Differentiate between primary and secondary benchmarks in summary table

Author: Kobzol

site/src/comparison.rs

@@ -11,6 +11,7 @@ use crate::selector::{self, Tag};
 use collector::Bound;
 use serde::Serialize;
 
+use database::BenchmarkData;
 use std::collections::{HashMap, HashSet};
 use std::error::Error;
 use std::hash::Hash;
@@ -193,9 +194,9 @@ impl ComparisonSummary {
             .cloned()
             .collect::<Vec<_>>();
         // Skip empty commits, sometimes happens if there's a compiler bug or so.
-        if comparisons.len() == 0 {
-            return None;
-        }
+        // if comparisons.len() == 0 {
+        //     return None;
+        // }
 
         let cmp = |b1: &TestResultComparison, b2: &TestResultComparison| {
             b2.relative_change()
@@ -421,61 +422,103 @@ impl ComparisonSummary {
             .unwrap();
         }
     }
+}
 
-    /// Writes a Markdown table containing summary of relevant results.
-    pub fn write_summary_table(&self, result: &mut String) {
-        use std::fmt::Write;
+pub type BenchmarkMap = HashMap<Benchmark, BenchmarkData>;
 
-        fn render_stat<F: FnOnce() -> Option<f64>>(count: usize, calculate: F) -> String {
-            let value = if count > 0 {
-                calculate()
-            } else {
-                None
-            };
-            value.map(|value| format!("{value:.1}%")).unwrap_or_else(|| "N/A".to_string())
-        }
+/// Writes a Markdown table containing summary of relevant results.
+pub fn write_summary_table(
+    primary: &ComparisonSummary,
+    secondary: &ComparisonSummary,
+    result: &mut String,
+) {
+    use std::fmt::Write;
 
-        writeln!(
-            result,
-            r#"| | Regressions 😿 | Improvements 🎉 | All relevant changes |
-|:---:|:---:|:---:|:---:|"#
-        )
-        .unwrap();
-        writeln!(
-            result,
-            "| count[^1] | {} | {} | {} |",
-            self.num_regressions,
-            self.num_improvements,
-            self.num_regressions + self.num_improvements
-        )
-        .unwrap();
-        writeln!(
-            result,
-            "| mean[^2] | {} | {} | {:.1}% |",
-            render_stat(self.num_regressions, || Some(self.arithmetic_mean_of_regressions())),
-            render_stat(self.num_improvements, || Some(self.arithmetic_mean_of_improvements())),
-            self.arithmetic_mean_of_changes()
-        )
-            .unwrap();
+    fn render_stat<F: FnOnce() -> Option<f64>>(count: usize, calculate: F) -> String {
+        let value = if count > 0 { calculate() } else { None };
+        value
+            .map(|value| format!("{value:.1}%"))
+            .unwrap_or_else(|| "N/A".to_string())
+    }
+
+    writeln!(
+        result,
+        r#"| | Regressions 😿 <br> (primary) | Regressions 😿 <br> (secondary) | Improvements 🎉 <br> (primary) | Improvements 🎉 <br> (secondary) | All relevant changes |
+|:---:|:---:|:---:|:---:|:---:|:---:|"#
+    )
+    .unwrap();
+    writeln!(
+        result,
+        "| count[^1] | {} | {} | {} | {} | {} |",
+        primary.num_regressions,
+        secondary.num_regressions,
+        primary.num_improvements,
+        secondary.num_improvements,
+        primary.num_regressions
+            + primary.num_improvements
+            + secondary.num_regressions
+            + secondary.num_improvements
+    )
+    .unwrap();
+
+    writeln!(
+        result,
+        "| mean[^2] | {} | {} | {} | {} | {:.1}% |",
+        render_stat(primary.num_regressions, || Some(
+            primary.arithmetic_mean_of_regressions()
+        )),
+        render_stat(secondary.num_regressions, || Some(
+            secondary.arithmetic_mean_of_regressions()
+        )),
+        render_stat(primary.num_improvements, || Some(
+            primary.arithmetic_mean_of_improvements()
+        )),
+        render_stat(secondary.num_improvements, || Some(
+            secondary.arithmetic_mean_of_improvements()
+        )),
+        // TODO:
+        // 1) compute average of averages?
+        // 2) merge two profiles and compute average?
+        // 3) pass merged profile as a parameter?
+        primary.arithmetic_mean_of_changes()
+    )
+    .unwrap();
 
-        let largest_change = self.most_relevant_changes().iter().fold(0.0, |accum: f64, item| {
+    // TODO: same question
+    let largest_change = primary
+        .most_relevant_changes()
+        .iter()
+        .fold(0.0, |accum: f64, item| {
             let change = item.map(|v| v.relative_change() * 100.0).unwrap_or(0.0);
             accum.max(change)
         });
 
-        writeln!(
-            result,
-            "| max | {} | {} | {:.1}% |",
-            render_stat(self.num_regressions, || self.largest_regression().map(|r| r.relative_change() * 100.0)),
-            render_stat(self.num_improvements, || self.largest_improvement().map(|r| r.relative_change() * 100.0)),
-            largest_change
-        )
-            .unwrap();
+    writeln!(
+        result,
+        "| max | {} | {} | {} | {} | {:.1}% |",
+        render_stat(primary.num_regressions, || primary
+            .largest_regression()
+            .map(|r| r.relative_change() * 100.0)),
+        render_stat(secondary.num_regressions, || secondary
+            .largest_regression()
+            .map(|r| r.relative_change() * 100.0)),
+        render_stat(primary.num_improvements, || primary
+            .largest_improvement()
+            .map(|r| r.relative_change() * 100.0)),
+        render_stat(secondary.num_improvements, || secondary
+            .largest_improvement()
+            .map(|r| r.relative_change() * 100.0)),
+        largest_change
+    )
+    .unwrap();
 
-        writeln!(result, r#"
+    writeln!(
+        result,
+        r#"
 [^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*"#).unwrap();
-    }
+[^2]: *the arithmetic mean of the percent change*"#
+    )
+    .unwrap();
 }
 
 /// The amount of confidence we have that a comparison actually represents a real
@@ -1291,83 +1334,130 @@ fn compare_link(start: &ArtifactId, end: &ArtifactId) -> String {
 
 #[cfg(test)]
 mod tests {
-    use std::collections::HashSet;
+    use std::collections::{HashMap, HashSet};
 
-    use database::{ArtifactId, Profile, Scenario};
+    use database::{ArtifactId, BenchmarkData, Profile, Scenario};
 
     use crate::comparison::{
-        ArtifactDescription, Comparison, ComparisonSummary, TestResultComparison,
+        write_summary_table, ArtifactDescription, Comparison, ComparisonSummary,
+        TestResultComparison,
     };
 
-    #[test]
-    fn summary_table_only_improvements() {
-        let summary = create_summary(vec![(10.0, 5.0), (8.0, 2.0)]);
-        check_table(
-            summary, r#"
-| | Regressions 😿 | Improvements 🎉 | All relevant changes |
-|:---:|:---:|:---:|:---:|
-| count[^1] | 0 | 2 | 2 |
-| mean[^2] | N/A | -62.5% | -62.5% |
-| max | N/A | -75.0% | 0.0% |
+    /*#[test]
+        fn summary_table_only_improvements() {
+            let summary = create_summary(vec![(10.0, 5.0), (8.0, 2.0)]);
+            check_table(
+                summary,
+                r#"
+    | | Regressions 😿 | Improvements 🎉 | All relevant changes |
+    |:---:|:---:|:---:|:---:|
+    | count[^1] | 0 | 2 | 2 |
+    | mean[^2] | N/A | -62.5% | -62.5% |
+    | max | N/A | -75.0% | 0.0% |
+
+    [^1]: *number of relevant changes*
+    [^2]: *the arithmetic mean of the percent change*
+    "#
+                .trim_start(),
+            );
+        }
 
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
-"#.trim_start(),
-        );
-    }
+        #[test]
+        fn summary_table_only_regressions() {
+            let summary = create_summary(vec![(5.0, 10.0), (1.0, 3.0)]);
+            check_table(
+                summary,
+                r#"
+    | | Regressions 😿 | Improvements 🎉 | All relevant changes |
+    |:---:|:---:|:---:|:---:|
+    | count[^1] | 2 | 0 | 2 |
+    | mean[^2] | 150.0% | N/A | 150.0% |
+    | max | 200.0% | N/A | 200.0% |
+
+    [^1]: *number of relevant changes*
+    [^2]: *the arithmetic mean of the percent change*
+    "#
+                .trim_start(),
+            );
+        }*/
 
     #[test]
-    fn summary_table_only_regressions() {
-        let summary = create_summary(vec![(5.0, 10.0), (1.0, 3.0)]);
+    fn summary_table_mixed_primary() {
         check_table(
-            summary, r#"
-| | Regressions 😿 | Improvements 🎉 | All relevant changes |
-|:---:|:---:|:---:|:---:|
-| count[^1] | 2 | 0 | 2 |
-| mean[^2] | 150.0% | N/A | 150.0% |
-| max | 200.0% | N/A | 200.0% |
+            vec![
+                ("primary", 10.0, 5.0),
+                ("primary", 5.0, 10.0),
+                ("primary", 1.0, 3.0),
+                ("primary", 4.0, 1.0),
+            ],
+            r#"
+| | Regressions 😿 <br> (primary) | Regressions 😿 <br> (secondary) | Improvements 🎉 <br> (primary) | Improvements 🎉 <br> (secondary) | All relevant changes |
+|:---:|:---:|:---:|:---:|:---:|:---:|
+| count[^1] | 2 | 0 | 2 | 0 | 4 |
+| mean[^2] | 150.0% | N/A | -62.5% | N/A | 43.8% |
+| max | 200.0% | N/A | -75.0% | N/A | 200.0% |
 
 [^1]: *number of relevant changes*
 [^2]: *the arithmetic mean of the percent change*
-"#.trim_start(),
+"#
+            .trim_start(),
         );
     }
 
     #[test]
-    fn summary_table_mixed() {
-        let summary = create_summary(vec![(10.0, 5.0), (5.0, 10.0), (1.0, 3.0), (4.0, 1.0)]);
+    fn summary_table_mixed_primary_secondary() {
         check_table(
-            summary, r#"
-| | Regressions 😿 | Improvements 🎉 | All relevant changes |
-|:---:|:---:|:---:|:---:|
-| count[^1] | 2 | 2 | 4 |
-| mean[^2] | 150.0% | -62.5% | 43.8% |
-| max | 200.0% | -75.0% | 200.0% |
+            vec![
+                ("primary", 10.0, 5.0),
+                ("primary", 5.0, 10.0),
+                ("secondary", 5.0, 10.0),
+                ("primary", 1.0, 3.0),
+                ("secondary", 3.0, 1.0),
+                ("primary", 4.0, 1.0),
+            ],
+            r#"
+| | Regressions 😿 <br> (primary) | Regressions 😿 <br> (secondary) | Improvements 🎉 <br> (primary) | Improvements 🎉 <br> (secondary) | All relevant changes |
+|:---:|:---:|:---:|:---:|:---:|:---:|
+| count[^1] | 2 | 1 | 2 | 1 | 6 |
+| mean[^2] | 150.0% | 100.0% | -62.5% | -66.7% | 43.8% |
+| max | 200.0% | 100.0% | -75.0% | -66.7% | 200.0% |
 
 [^1]: *number of relevant changes*
 [^2]: *the arithmetic mean of the percent change*
-"#.trim_start(),
+"#
+                .trim_start(),
         );
     }
 
-    fn check_table(summary: ComparisonSummary, expected: &str) {
-        let mut result = String::new();
-        summary.write_summary_table(&mut result);
-        assert_eq!(result, expected);
-    }
+    // (category, before, after)
+    fn check_table(values: Vec<(&str, f64, f64)>, expected: &str) {
+        let mut primary_statistics = HashSet::new();
+        let mut secondary_statistics = HashSet::new();
+        for (index, (category, before, after)) in values.into_iter().enumerate() {
+            let target = if category == "primary" {
+                &mut primary_statistics
+            } else {
+                &mut secondary_statistics
+            };
 
-    fn create_summary(values: Vec<(f64, f64)>) -> ComparisonSummary {
-        let mut statistics = HashSet::new();
-        for (index, diff) in values.into_iter().enumerate() {
-            statistics.insert(TestResultComparison {
+            target.insert(TestResultComparison {
                 benchmark: index.to_string().as_str().into(),
                 profile: Profile::Check,
                 scenario: Scenario::Empty,
                 variance: None,
-                results: diff,
+                results: (before, after),
             });
         }
 
+        let primary = create_summary(primary_statistics);
+        let secondary = create_summary(secondary_statistics);
+
+        let mut result = String::new();
+        write_summary_table(&primary, &secondary, &mut result);
+        assert_eq!(result, expected);
+    }
+
+    fn create_summary(statistics: HashSet<TestResultComparison>) -> ComparisonSummary {
         let comparison = Comparison {
             a: ArtifactDescription {
                 artifact: ArtifactId::Tag("a".to_string()),

site/src/github.rs

@@ -1,5 +1,7 @@
 use crate::api::github::Issue;
-use crate::comparison::{ComparisonConfidence, ComparisonSummary, Direction};
+use crate::comparison::{
+    write_summary_table, BenchmarkMap, ComparisonConfidence, ComparisonSummary, Direction,
+};
 use crate::load::{Config, SiteCtxt, TryCommit};
 
 use anyhow::Context as _;
@@ -648,6 +650,14 @@ compiler perf.{next_steps}
     )
 }
 
+async fn get_benchmark_map(ctxt: &SiteCtxt) -> BenchmarkMap {
+    let benchmarks = ctxt.pool.connection().await.get_benchmarks().await;
+    benchmarks
+        .into_iter()
+        .map(|bench| (bench.name.as_str().into(), bench))
+        .collect()
+}
+
 async fn categorize_benchmark(
     ctxt: &SiteCtxt,
     commit_sha: String,
@@ -710,14 +720,22 @@ async fn categorize_benchmark(
     let num_regressions = summary.number_of_regressions();
 
     let short_summary = match direction {
-        Direction::Improvement => format!("🎉 relevant {} found", ending("improvement", num_improvements)),
-        Direction::Regression => format!("😿 relevant {} found", ending("regression", num_regressions)),
+        Direction::Improvement => format!(
+            "🎉 relevant {} found",
+            ending("improvement", num_improvements)
+        ),
+        Direction::Regression => format!(
+            "😿 relevant {} found",
+            ending("regression", num_regressions)
+        ),
         Direction::Mixed => "mixed results".to_string(),
     };
 
+    let benchmark_map = get_benchmark_map(ctxt).await;
+
     let mut result = format!("Summary: {short_summary}\n");
-    summary.write_summary_table(&mut result);
-    write!(result, "\n{}", DISAGREEMENT).unwrap();
+    // write_summary_table(&summary, &benchmark_map, &mut result); TODO
+    // write!(result, "\n{}", DISAGREEMENT).unwrap();
     (result, Some(direction))
 }