Merge pull request #881 from rylev/categorize-perf-runs

rylev · web-flow · commit 931ac4363a3a · 2021-06-22T15:26:33.000+02:00
Categorize perf runs when posting to GitHub
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 = collector::master_commits().await?;
-    let comparison = compare(
+    let comparison = compare_given_commits(
         start.clone(),
         start.clone(),
         "instructions:u".to_owned(),
@@ -40,7 +40,7 @@ pub async fn handle_triage(
     let mut before = start.clone();
 
     loop {
-        let comparison = match compare(
+        let comparison = match compare_given_commits(
             before,
             after.clone(),
             "instructions:u".to_owned(),
@@ -87,16 +87,17 @@ pub async fn handle_compare(
     body: api::days::Request,
     data: &InputData,
 ) -> Result<api::days::Response, BoxedError> {
-    let commits = collector::master_commits().await?;
+    let master_commits = collector::master_commits().await?;
     let end = body.end;
-    let comparison = crate::comparison::compare(body.start, end.clone(), body.stat, data, &commits)
-        .await?
-        .ok_or_else(|| format!("could not find end commit for bound {:?}", end))?;
+    let comparison =
+        compare_given_commits(body.start, end.clone(), body.stat, data, &master_commits)
+            .await?
+            .ok_or_else(|| format!("could not find end commit for bound {:?}", end))?;
 
     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,
@@ -108,7 +109,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();
 
@@ -117,44 +118,44 @@ 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));
+
+        Some(ComparisonSummary { hi, lo })
+    }
+
     /// The direction of the changes
-    fn direction(&self) -> Option<Direction> {
+    pub fn direction(&self) -> Option<Direction> {
         let d = match (&self.hi, &self.lo) {
             (None, None) => return None,
             (Some(b), None) => b.direction(),
@@ -166,7 +167,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],
@@ -202,7 +203,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
     }
@@ -216,6 +217,17 @@ pub async fn compare(
     end: Bound,
     stat: String,
     data: &InputData,
+) -> Result<Option<Comparison>, BoxedError> {
+    let master_commits = collector::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: &[collector::MasterCommit],
 ) -> Result<Option<Comparison>, BoxedError> {
     let a = data
@@ -404,7 +416,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),
@@ -446,7 +458,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 {
@@ -467,7 +479,10 @@ impl BenchmarkComparison<'_> {
             "{} {} in [instruction counts]({})",
             size,
             self.direction(),
-            link
+            match link {
+                Some(l) => l,
+                None => "",
+            }
         )
         .unwrap();
         writeln!(
@@ -481,7 +496,7 @@ impl BenchmarkComparison<'_> {
 
 // The direction of a performance change
 #[derive(PartialEq, Eq, Hash)]
-enum Direction {
+pub enum Direction {
     Improvement,
     Regression,
     Mixed,
diff --git a/site/src/github.rs b/site/src/github.rs
@@ -7,7 +7,7 @@ use serde::Deserialize;
 use database::ArtifactId;
 use regex::Regex;
 use reqwest::header::USER_AGENT;
-use std::{sync::Arc, time::Duration};
+use std::{fmt::Write, sync::Arc, time::Duration};
 
 lazy_static::lazy_static! {
     static ref BODY_TRY_COMMIT: Regex =
@@ -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,56 @@ 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(Some(c)) => c,
+        _ => return String::from("ERROR categorizing benchmark run!"),
+    };
+    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 crate::comparison::ComparisonSummary::summarize_comparison(&comparison) {
+            Some(s) if s.direction().is_some() => {
+                let direction = s.direction().unwrap();
+                (s, direction)
+            }
+            _ => {
+                return format!(
+                    "This benchmark run did not return any significant changes.\n\n{}",
+                    DISAGREEMENT
+                )
+            }
+        };
+
+    use crate::comparison::Direction;
+    let category = match direction {
+        Direction::Improvement => "improvements 🎉",
+        Direction::Regression => "regressions 😿",
+        Direction::Mixed => "mixed results 🤷",
+    };
+    let mut result = format!(
+        "This change led to significant {} in compiler performance.\n",
+        category
+    );
+    for change in summary.ordered_changes() {
+        write!(result, "- ").unwrap();
+        change.summary_line(&mut result, None)
+    }
+    write!(result, "\n{}", DISAGREEMENT).unwrap();
+    result
+}
