Merge pull request #1322 from rust-lang/summary-extended-stats

Author: rylev

site/src/api.rs

@@ -130,6 +130,7 @@ pub mod bootstrap {
 }
 
 pub mod comparison {
+    use crate::comparison::Stat;
     use collector::Bound;
     use database::{BenchmarkData, Date};
     use serde::{Deserialize, Serialize};
@@ -139,7 +140,7 @@ pub mod comparison {
     pub struct Request {
         pub start: Bound,
         pub end: Bound,
-        pub stat: String,
+        pub stat: Stat,
     }
 
     #[derive(Debug, Clone, Serialize, Deserialize)]

site/src/comparison.rs

@@ -10,11 +10,12 @@ use crate::selector::{self, Tag};
 
 use collector::category::Category;
 use collector::Bound;
-use serde::Serialize;
+use serde::{Deserialize, Serialize};
 
 use std::cmp::Ordering;
 use std::collections::{HashMap, HashSet};
 use std::error::Error;
+use std::fmt::Write;
 use std::hash::Hash;
 use std::sync::Arc;
 
@@ -44,11 +45,11 @@ pub async fn handle_triage(
     let mut before = start.clone();
 
     let mut num_comparisons = 0;
-    let stat = "instructions:u".to_owned();
+    let stat = Stat::Instructions;
     let benchmark_map = ctxt.get_benchmark_category_map().await;
     loop {
         let comparison =
-            match compare_given_commits(before, next.clone(), stat.clone(), ctxt, &master_commits)
+            match compare_given_commits(before, next.clone(), stat, ctxt, &master_commits)
                 .await
                 .map_err(|e| format!("error comparing commits: {}", e))?
             {
@@ -171,14 +172,46 @@ async fn populate_report(
         (None, None) => return,
     };
 
-    let include_in_triage = deserves_attention(&primary, &secondary);
+    let include_in_triage = deserves_attention_icount(&primary, &secondary);
 
     if include_in_triage {
         let entry = report.entry(direction).or_default();
         entry.push(write_triage_summary(comparison, &primary, &secondary).await);
     }
 }
 
+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub enum Stat {
+    #[serde(rename = "instructions:u")]
+    Instructions,
+    #[serde(rename = "cycles:u")]
+    Cycles,
+    #[serde(rename = "faults")]
+    Faults,
+    #[serde(rename = "max-rss")]
+    MaxRSS,
+    #[serde(rename = "task-clock")]
+    TaskClock,
+    #[serde(rename = "wall-time")]
+    WallTime,
+    #[serde(rename = "cpu-clock")]
+    CpuClock,
+}
+
+impl Stat {
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Instructions => "instructions:u",
+            Self::Cycles => "cycles:u",
+            Self::Faults => "faults",
+            Self::MaxRSS => "max-rss",
+            Self::TaskClock => "task-clock",
+            Self::WallTime => "wall-time",
+            Self::CpuClock => "cpu-clock",
+        }
+    }
+}
+
 /// A summary of a given comparison
 ///
 /// This summary only includes changes that are significant and relevant (as determined by a change's magnitude).
@@ -366,7 +399,7 @@ impl ArtifactComparisonSummary {
 ///
 /// For example, this can be used to determine if artifact comparisons with regressions should be labeled with the
 /// `perf-regression` GitHub label or should be shown in the perf triage report.
-pub(crate) fn deserves_attention(
+pub(crate) fn deserves_attention_icount(
     primary: &ArtifactComparisonSummary,
     secondary: &ArtifactComparisonSummary,
 ) -> bool {
@@ -388,7 +421,6 @@ async fn write_triage_summary(
     primary: &ArtifactComparisonSummary,
     secondary: &ArtifactComparisonSummary,
 ) -> String {
-    use std::fmt::Write;
     let mut result = if let Some(pr) = comparison.b.pr {
         let title = github::pr_title(pr).await;
         format!(
@@ -415,8 +447,6 @@ pub fn write_summary_table(
     with_footnotes: bool,
     result: &mut String,
 ) {
-    use std::fmt::Write;
-
     fn render_stat<F: FnOnce() -> Option<f64>>(count: usize, calculate: F) -> String {
         let value = if count > 0 { calculate() } else { None };
         value
@@ -538,16 +568,16 @@ pub fn write_summary_table(
         }),
         largest_change,
     ]);
+}
 
-    if with_footnotes {
-        writeln!(
-            result,
-            r#"
+pub fn write_summary_table_footer(result: &mut String) {
+    writeln!(
+        result,
+        r#"
 [^1]: *number of relevant changes*
 [^2]: *the arithmetic mean of the percent change*"#
-        )
-        .unwrap();
-    }
+    )
+    .unwrap();
 }
 
 /// Compare two bounds on a given stat
@@ -556,7 +586,7 @@ pub fn write_summary_table(
 pub async fn compare(
     start: Bound,
     end: Bound,
-    stat: String,
+    stat: Stat,
     ctxt: &SiteCtxt,
 ) -> Result<Option<ArtifactComparison>, BoxedError> {
     let master_commits = &ctxt.get_master_commits().commits;
@@ -568,7 +598,7 @@ pub async fn compare(
 async fn compare_given_commits(
     start: Bound,
     end: Bound,
-    stat: String,
+    stat: Stat,
     ctxt: &SiteCtxt,
     master_commits: &[collector::MasterCommit],
 ) -> Result<Option<ArtifactComparison>, BoxedError> {
@@ -587,7 +617,7 @@ async fn compare_given_commits(
         .set::<String>(Tag::Benchmark, selector::Selector::All)
         .set::<String>(Tag::Scenario, selector::Selector::All)
         .set::<String>(Tag::Profile, selector::Selector::All)
-        .set(Tag::Metric, selector::Selector::One(stat.clone()));
+        .set(Tag::Metric, selector::Selector::One(stat.as_str()));
 
     // `responses` contains series iterators. The first element in the iterator is the data
     // for `a` and the second is the data for `b`
@@ -834,7 +864,7 @@ impl HistoricalDataMap {
         ctxt: &SiteCtxt,
         from: ArtifactId,
         master_commits: &[collector::MasterCommit],
-        stat: String,
+        stat: Stat,
     ) -> Result<Self, BoxedError> {
         let mut historical_data = HashMap::new();
 
@@ -856,7 +886,7 @@ impl HistoricalDataMap {
             .set::<String>(Tag::Benchmark, selector::Selector::All)
             .set::<String>(Tag::Scenario, selector::Selector::All)
             .set::<String>(Tag::Profile, selector::Selector::All)
-            .set(Tag::Metric, selector::Selector::One(stat));
+            .set(Tag::Metric, selector::Selector::One(stat.as_str()));
 
         let mut previous_commit_series = ctxt
             .statistic_series(query, previous_commits.clone())
@@ -1272,9 +1302,6 @@ mod tests {
 | count[^1]  | 3                              | 0                                | 0                               | 0                                 | 3                        |
 | mean[^2]   | 146.7%                         | N/A                              | N/A                             | N/A                               | 146.7%                   |
 | max        | 200.0%                         | N/A                              | N/A                             | N/A                               | 200.0%                   |
-
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
 "#
             .trim_start(),
         );
@@ -1294,9 +1321,6 @@ mod tests {
 | count[^1]  | 0                              | 0                                | 3                               | 0                                 | 3                        |
 | mean[^2]   | N/A                            | N/A                              | -71.7%                          | N/A                               | -71.7%                   |
 | max        | N/A                            | N/A                              | -80.0%                          | N/A                               | -80.0%                   |
-
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
 "#
             .trim_start(),
         );
@@ -1316,9 +1340,6 @@ mod tests {
 | count[^1]  | 0                              | 0                                | 0                               | 3                                 | 0                        |
 | mean[^2]   | N/A                            | N/A                              | N/A                             | -71.7%                            | N/A                      |
 | max        | N/A                            | N/A                              | N/A                             | -80.0%                            | N/A                      |
-
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
 "#
                 .trim_start(),
         );
@@ -1338,9 +1359,6 @@ mod tests {
 | count[^1]  | 0                              | 3                                | 0                               | 0                                 | 0                        |
 | mean[^2]   | N/A                            | 146.7%                           | N/A                             | N/A                               | N/A                      |
 | max        | N/A                            | 200.0%                           | N/A                             | N/A                               | N/A                      |
-
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
 "#
                 .trim_start(),
         );
@@ -1361,9 +1379,6 @@ mod tests {
 | 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(),
         );
@@ -1386,9 +1401,6 @@ mod tests {
 | count[^1]  | 2                              | 1                                | 2                               | 1                                 | 4                        |
 | 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(),
         );
@@ -1407,9 +1419,6 @@ mod tests {
 | count[^1]  | 1                              | 0                                | 1                               | 0                                 | 2                        |
 | mean[^2]   | 20.0%                          | N/A                              | -50.0%                          | N/A                               | -15.0%                   |
 | max        | 20.0%                          | N/A                              | -50.0%                          | N/A                               | -50.0%                   |
-
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
 "#
                 .trim_start(),
         );
@@ -1428,9 +1437,6 @@ mod tests {
 | count[^1]  | 1                              | 0                                | 1                               | 0                                 | 2                        |
 | mean[^2]   | 100.0%                         | N/A                              | -16.7%                          | N/A                               | 41.7%                    |
 | max        | 100.0%                         | N/A                              | -16.7%                          | N/A                               | 100.0%                   |
-
-[^1]: *number of relevant changes*
-[^2]: *the arithmetic mean of the percent change*
 "#
                 .trim_start(),
         );