Turn Stat into an enum

Kobzol · Kobzol · commit 935c4da2f753 · 2022-05-16T12:24:33.000+02:00
diff --git a/site/src/api.rs b/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)]
diff --git a/site/src/comparison.rs b/site/src/comparison.rs
@@ -10,7 +10,7 @@ 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};
@@ -45,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))?
             {
@@ -172,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).
@@ -367,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 {
@@ -384,6 +416,21 @@ pub(crate) fn deserves_attention(
     }
 }
 
+pub(crate) fn deserves_attention_maxrss(
+    primary: &ArtifactComparisonSummary,
+    secondary: &ArtifactComparisonSummary,
+) -> bool {
+    let primary_big_changes = primary
+        .improvements()
+        .filter(|comparison| comparison.magnitude() >= Magnitude::Large)
+        .count();
+    let secondary_big_changes = secondary
+        .improvements()
+        .filter(|comparison| comparison.magnitude() >= Magnitude::Large)
+        .count();
+    primary_big_changes >= 10 || secondary_big_changes >= 20
+}
+
 async fn write_triage_summary(
     comparison: &ArtifactComparison,
     primary: &ArtifactComparisonSummary,
@@ -554,7 +601,7 @@ pub fn write_summary_table_footer(result: &mut String) {
 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;
@@ -566,7 +613,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> {
@@ -585,7 +632,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`
@@ -832,7 +879,7 @@ impl HistoricalDataMap {
         ctxt: &SiteCtxt,
         from: ArtifactId,
         master_commits: &[collector::MasterCommit],
-        stat: String,
+        stat: Stat,
     ) -> Result<Self, BoxedError> {
         let mut historical_data = HashMap::new();
 
@@ -854,7 +901,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())
diff --git a/site/src/github.rs b/site/src/github.rs
@@ -1,7 +1,7 @@
 use crate::api::github::Issue;
 use crate::comparison::{
-    deserves_attention, write_summary_table, write_summary_table_footer, ArtifactComparisonSummary,
-    Direction, Magnitude,
+    deserves_attention_icount, deserves_attention_maxrss, write_summary_table,
+    write_summary_table_footer, ArtifactComparisonSummary, Direction, Stat,
 };
 use crate::load::{Config, SiteCtxt, TryCommit};
 
@@ -568,23 +568,20 @@ async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_maste
     post_comment(&ctxt.config, pr, body).await;
 }
 
-fn make_comparison_url(commit: &QueuedCommit, stat: Option<&str>) -> String {
-    let mut url = format!(
-        "https://perf.rust-lang.org/compare.html?start={}&end={}",
-        commit.parent_sha, commit.sha
-    );
-    if let Some(stat) = stat {
-        write!(&mut url, "&stat={stat}").unwrap();
-    }
-    url
+fn make_comparison_url(commit: &QueuedCommit, stat: Stat) -> String {
+    format!(
+        "https://perf.rust-lang.org/compare.html?start={}&end={}&stat={}",
+        commit.parent_sha,
+        commit.sha,
+        stat.as_str()
+    )
 }
 
 async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) -> String {
-    let inst_comparison_url = make_comparison_url(&commit, None);
     let comparison = match crate::comparison::compare(
         collector::Bound::Commit(commit.parent_sha.clone()),
         collector::Bound::Commit(commit.sha.clone()),
-        "instructions:u".to_owned(),
+        Stat::Instructions,
         ctxt,
     )
     .await
@@ -612,6 +609,7 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit:
     please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new).";
     let footer = format!("{DISAGREEMENT}{errors}");
 
+    let inst_comparison_url = make_comparison_url(&commit, Stat::Instructions);
     let mut message = format!(
         "Finished benchmarking commit ({sha}): [comparison url]({inst_comparison_url}).
 
@@ -653,16 +651,16 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit:
     if let Some((primary, secondary)) = analyze_secondary_stat(
         ctxt,
         &commit,
-        "max-rss",
+        Stat::MaxRSS,
         &benchmark_map,
-        is_max_rss_interesting,
+        deserves_attention_maxrss,
     )
     .await
     {
         write!(
             &mut message,
             "\n## [Max RSS]({})",
-            make_comparison_url(&commit, Some("max-rss"))
+            make_comparison_url(&commit, Stat::MaxRSS)
         )
         .unwrap();
         write_summary_table(&primary, &secondary, true, &mut message);
@@ -680,36 +678,20 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit:
     message
 }
 
-// TODO: determine how should this work
-// Should this be separate from `deserves_attention`?
-fn is_max_rss_interesting(
-    primary: &ArtifactComparisonSummary,
-    _secondary: &ArtifactComparisonSummary,
-) -> bool {
-    if primary
-        .largest_change()
-        .map(|c| c.magnitude() >= Magnitude::Large)
-        .unwrap_or(false)
-    {
-        return true;
-    }
-
-    primary.num_changes() >= 20
-}
-
-async fn analyze_secondary_stat<
-    F: FnOnce(&ArtifactComparisonSummary, &ArtifactComparisonSummary) -> bool,
->(
+async fn analyze_secondary_stat<F>(
     ctxt: &SiteCtxt,
     commit: &QueuedCommit,
-    stat: &str,
+    stat: Stat,
     benchmark_map: &HashMap<Benchmark, Category>,
     is_interesting: F,
-) -> Option<(ArtifactComparisonSummary, ArtifactComparisonSummary)> {
+) -> Option<(ArtifactComparisonSummary, ArtifactComparisonSummary)>
+where
+    F: FnOnce(&ArtifactComparisonSummary, &ArtifactComparisonSummary) -> bool,
+{
     let comparison = match crate::comparison::compare(
         collector::Bound::Commit(commit.parent_sha.clone()),
         collector::Bound::Commit(commit.sha.clone()),
-        stat.to_string(),
+        stat,
         ctxt,
     )
     .await
@@ -736,7 +718,7 @@ fn next_steps(
     direction: Option<Direction>,
     is_master_commit: bool,
 ) -> String {
-    let deserves_attention = deserves_attention(&primary, &secondary);
+    let deserves_attention = deserves_attention_icount(&primary, &secondary);
     let label = match (deserves_attention, direction) {
         (true, Some(Direction::Regression | Direction::Mixed)) => "+perf-regression",
         _ => "-perf-regression",

Original file line number	Diff line number	Diff line change
`@@ -130,6 +130,7 @@ pub mod bootstrap {`
`130`	`130`	`}`
`131`	`131`
`132`	`132`	`pub mod comparison {`
	`133`	`+ use crate::comparison::Stat;`
`133`	`134`	`use collector::Bound;`
`134`	`135`	`use database::{BenchmarkData, Date};`
`135`	`136`	`use serde::{Deserialize, Serialize};`
`@@ -139,7 +140,7 @@ pub mod comparison {`
`139`	`140`	`pub struct Request {`
`140`	`141`	`pub start: Bound,`
`141`	`142`	`pub end: Bound,`
`142`		`- pub stat: String,`
	`143`	`+ pub stat: Stat,`
`143`	`144`	`}`
`144`	`145`
`145`	`146`	`#[derive(Debug, Clone, Serialize, Deserialize)]`