Categorize perf runs when posting to GitHub

rylev · rylev · commit 85364da8dfff · 2021-06-10T16:57:31.000+02:00
diff --git a/site/src/comparison.rs b/site/src/comparison.rs
@@ -25,7 +25,7 @@ pub async fn handle_triage(
     let end = body.end;
     // Compare against self to get next
     let master_commits = rustc_artifacts::master_commits().await?;
-    let comparison = compare(
+    let comparison = compare_given_commits(
         start.clone(),
         start.clone(),
         "instructions:u".to_owned(),
@@ -39,7 +39,7 @@ pub async fn handle_triage(
     let mut before = start.clone();
 
     loop {
-        let comparison = compare(
+        let comparison = compare_given_commits(
             before,
             after.clone(),
             "instructions:u".to_owned(),
@@ -76,14 +76,14 @@ pub async fn handle_compare(
     body: api::days::Request,
     data: &InputData,
 ) -> Result<api::days::Response, BoxedError> {
-    let commits = rustc_artifacts::master_commits().await?;
+    let master_commits = rustc_artifacts::master_commits().await?;
     let comparison =
-        crate::comparison::compare(body.start, body.end, body.stat, data, &commits).await?;
+        compare_given_commits(body.start, body.end, body.stat, data, &master_commits).await?;
 
     let conn = data.conn().await;
-    let prev = comparison.prev(&commits);
-    let next = comparison.next(&commits);
-    let is_contiguous = comparison.is_contiguous(&*conn, &commits).await;
+    let prev = comparison.prev(&master_commits);
+    let next = comparison.next(&master_commits);
+    let is_contiguous = comparison.is_contiguous(&*conn, &master_commits).await;
 
     Ok(api::days::Response {
         prev,
@@ -95,7 +95,7 @@ pub async fn handle_compare(
 }
 
 async fn populate_report(comparison: &Comparison, report: &mut HashMap<Direction, Vec<String>>) {
-    if let Some(summary) = summarize_comparison(comparison) {
+    if let Some(summary) = ComparisonSummary::summarize_comparison(comparison) {
         if let Some(direction) = summary.direction() {
             let entry = report.entry(direction).or_default();
 
@@ -104,42 +104,43 @@ async fn populate_report(comparison: &Comparison, report: &mut HashMap<Direction
     }
 }
 
-fn summarize_comparison<'a>(comparison: &'a Comparison) -> Option<ComparisonSummary<'a>> {
-    let mut benchmarks = comparison.get_benchmarks();
-    // Skip empty commits, sometimes happens if there's a compiler bug or so.
-    if benchmarks.len() == 0 {
-        return None;
-    }
-
-    let cmp = |b1: &BenchmarkComparison, b2: &BenchmarkComparison| {
-        b1.log_change()
-            .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));
-    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));
-
-    Some(ComparisonSummary { hi, lo })
-}
-
-struct ComparisonSummary<'a> {
+pub struct ComparisonSummary<'a> {
     hi: Option<BenchmarkComparison<'a>>,
     lo: Option<BenchmarkComparison<'a>>,
 }
 
 impl ComparisonSummary<'_> {
+    pub fn summarize_comparison<'a>(comparison: &'a Comparison) -> Option<ComparisonSummary<'a>> {
+        let mut benchmarks = comparison.get_benchmarks();
+        // Skip empty commits, sometimes happens if there's a compiler bug or so.
+        if benchmarks.len() == 0 {
+            return None;
+        }
+
+        let cmp = |b1: &BenchmarkComparison, b2: &BenchmarkComparison| {
+            b1.log_change()
+                .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));
+        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));
+
+        benchmarks.clear();
+
+        Some(ComparisonSummary { hi, lo })
+    }
     /// The direction of the changes
     fn direction(&self) -> Option<Direction> {
         let d = match (&self.hi, &self.lo) {
@@ -153,7 +154,7 @@ impl ComparisonSummary<'_> {
     }
 
     /// The changes ordered by their signficance (most significant first)
-    fn ordered_changes(&self) -> Vec<&BenchmarkComparison<'_>> {
+    pub fn ordered_changes(&self) -> Vec<&BenchmarkComparison<'_>> {
         match (&self.hi, &self.lo) {
             (None, None) => Vec::new(),
             (Some(b), None) => vec![b],
@@ -189,7 +190,7 @@ impl ComparisonSummary<'_> {
 
         for change in self.ordered_changes() {
             write!(result, "- ").unwrap();
-            change.summary_line(&mut result, link)
+            change.summary_line(&mut result, Some(link))
         }
         result
     }
@@ -201,6 +202,17 @@ pub async fn compare(
     end: Bound,
     stat: String,
     data: &InputData,
+) -> Result<Comparison, BoxedError> {
+    let master_commits = rustc_artifacts::master_commits().await?;
+    compare_given_commits(start, end, stat, data, &master_commits).await
+}
+
+/// Compare two bounds on a given stat
+pub async fn compare_given_commits(
+    start: Bound,
+    end: Bound,
+    stat: String,
+    data: &InputData,
     master_commits: &[rustc_artifacts::Commit],
 ) -> Result<Comparison, BoxedError> {
     let a = data
@@ -388,7 +400,7 @@ impl Comparison {
 
 // A single comparison based on benchmark and cache state
 #[derive(Debug)]
-struct BenchmarkComparison<'a> {
+pub struct BenchmarkComparison<'a> {
     bench_name: &'a str,
     cache_state: &'a str,
     results: (f64, f64),
@@ -430,7 +442,7 @@ impl BenchmarkComparison<'_> {
         }
     }
 
-    fn summary_line(&self, summary: &mut String, link: &str) {
+    pub fn summary_line(&self, summary: &mut String, link: Option<&str>) {
         use std::fmt::Write;
         let magnitude = self.log_change().abs();
         let size = if magnitude > 0.10 {
@@ -451,7 +463,10 @@ impl BenchmarkComparison<'_> {
             "{} {} in [instruction counts]({})",
             size,
             self.direction(),
-            link
+            match link {
+                Some(l) => l,
+                None => "",
+            }
         )
         .unwrap();
         writeln!(
diff --git a/site/src/github.rs b/site/src/github.rs
@@ -631,12 +631,15 @@ pub async fn post_finished(data: &InputData) {
                 "https://perf.rust-lang.org/compare.html?start={}&end={}",
                 commit.parent_sha, commit.sha
             );
+            let summary = categorize_benchmark(&commit, data).await;
             post_comment(
                 &data.config,
                 commit.pr,
                 format!(
                     "Finished benchmarking try commit ({}): [comparison url]({}).
 
+                    Summary: {}
+
 Benchmarking this pull request likely means that it is \
 perf-sensitive, so we're automatically marking it as not fit \
 for rolling up. Please note that if the perf results are \
@@ -649,10 +652,36 @@ regressions or improvements in the roll up.
 
 @bors rollup=never
 @rustbot label: +S-waiting-on-review -S-waiting-on-perf",
-                    commit.sha, comparison_url
+                    commit.sha, comparison_url, summary
                 ),
             )
             .await;
         }
     }
 }
+
+async fn categorize_benchmark(commit: &database::QueuedCommit, data: &InputData) -> String {
+    let comparison = match crate::comparison::compare(
+        collector::Bound::Commit(commit.parent_sha.clone()),
+        collector::Bound::Commit(commit.sha.clone()),
+        "instructions:u".to_owned(),
+        data,
+    )
+    .await
+    {
+        Ok(c) => c,
+        Err(_) => return String::from("ERROR categorizing benchmark run!"),
+    };
+    let summary = match crate::comparison::ComparisonSummary::summarize_comparison(&comparison) {
+        Some(s) => s,
+        None => return String::from("This benchmark run did not return any significant changes"),
+    };
+
+    let mut result = format!("This change led to significant changes in compiler performance.\n");
+    for change in summary.ordered_changes() {
+        use std::fmt::Write;
+        write!(result, "- ").unwrap();
+        change.summary_line(&mut result, None)
+    }
+    result
+}
