diff --git a/site/src/api.rs b/site/src/api.rs index 85d1a8d9b..cfa2f064d 100644 --- 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 index 62f19c4f4..2f2e4b317 100644 --- a/site/src/comparison.rs +++ b/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,7 +172,7 @@ 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(); @@ -179,6 +180,38 @@ async fn populate_report( } } +#[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 Option>(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, 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, BoxedError> { @@ -587,7 +617,7 @@ async fn compare_given_commits( .set::(Tag::Benchmark, selector::Selector::All) .set::(Tag::Scenario, selector::Selector::All) .set::(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 { let mut historical_data = HashMap::new(); @@ -856,7 +886,7 @@ impl HistoricalDataMap { .set::(Tag::Benchmark, selector::Selector::All) .set::(Tag::Scenario, selector::Selector::All) .set::(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(), ); diff --git a/site/src/github.rs b/site/src/github.rs index a797cda22..f05ad8e06 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -1,6 +1,7 @@ use crate::api::github::Issue; use crate::comparison::{ - deserves_attention, write_summary_table, ArtifactComparisonSummary, Direction, + deserves_attention_icount, write_summary_table, write_summary_table_footer, ArtifactComparison, + ArtifactComparisonSummary, Direction, Stat, }; use crate::load::{Config, SiteCtxt, TryCommit}; @@ -10,6 +11,7 @@ use reqwest::header::USER_AGENT; use serde::{Deserialize, Serialize}; use std::collections::HashSet; + use std::{fmt::Write, sync::Arc, time::Duration}; type BoxedError = Box; @@ -561,30 +563,60 @@ pub async fn post_finished(ctxt: &SiteCtxt) { /// `is_master_commit` is used to differentiate messages for try runs and post-merge runs. async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) { let pr = commit.pr; - let body = summarize_run(ctxt, commit, is_master_commit).await; + let body = match summarize_run(ctxt, commit, is_master_commit).await { + Ok(message) => message, + Err(error) => error, + }; post_comment(&ctxt.config, pr, body).await; } -async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) -> String { - let comparison_url = format!( - "https://perf.rust-lang.org/compare.html?start={}&end={}", - commit.parent_sha, commit.sha - ); - let comparison = match crate::comparison::compare( - collector::Bound::Commit(commit.parent_sha), +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 calculate_stat_comparison( + ctxt: &SiteCtxt, + commit: &QueuedCommit, + stat: Stat, +) -> Result { + match crate::comparison::compare( + collector::Bound::Commit(commit.parent_sha.clone()), collector::Bound::Commit(commit.sha.clone()), - "instructions:u".to_owned(), + stat, ctxt, ) .await { - Ok(Some(c)) => c, - _ => return String::from("ERROR categorizing benchmark run!"), - }; + Ok(Some(c)) => Ok(c), + _ => Err("ERROR categorizing benchmark run!".to_owned()), + } +} - let errors = if !comparison.newly_failed_benchmarks.is_empty() { - let benchmarks = comparison +async fn summarize_run( + ctxt: &SiteCtxt, + commit: QueuedCommit, + is_master_commit: bool, +) -> Result { + let benchmark_map = ctxt.get_benchmark_category_map().await; + + let mut message = format!( + "Finished benchmarking commit ({sha}): [comparison url]({comparison_url}). + +**Summary**:\n\n", + sha = commit.sha, + comparison_url = make_comparison_url(&commit, Stat::Instructions) + ); + + let inst_comparison = calculate_stat_comparison(ctxt, &commit, Stat::Instructions).await?; + + let errors = if !inst_comparison.newly_failed_benchmarks.is_empty() { + let benchmarks = inst_comparison .newly_failed_benchmarks .iter() .map(|(benchmark, _)| format!("- {benchmark}")) @@ -594,33 +626,79 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: } else { String::new() }; + let (inst_primary, inst_secondary) = inst_comparison + .clone() + .summarize_by_category(&benchmark_map); + + let mut table_written = false; + let stats = vec![ + ( + "Instruction count", + Stat::Instructions, + false, + inst_comparison, + ), + ( + "Max RSS (memory usage)", + Stat::MaxRSS, + true, + calculate_stat_comparison(ctxt, &commit, Stat::MaxRSS).await?, + ), + ( + "Cycles", + Stat::Cycles, + true, + calculate_stat_comparison(ctxt, &commit, Stat::Cycles).await?, + ), + ]; - let benchmark_map = ctxt.get_benchmark_category_map().await; - let (primary, secondary) = comparison.summarize_by_category(&benchmark_map); + for (title, stat, hidden, comparison) in stats { + message.push_str(&format!( + "## [{title}]({})\n", + make_comparison_url(&commit, stat) + )); + + let (primary, secondary) = comparison.summarize_by_category(&benchmark_map); + table_written |= write_stat_summary(primary, secondary, hidden, &mut message).await; + } + + if table_written { + write_summary_table_footer(&mut message); + } 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 footer = format!("{DISAGREEMENT}{errors}"); - let mut message = format!( - "Finished benchmarking commit ({sha}): [comparison url]({comparison_url}). + let direction = inst_primary.direction().or(inst_secondary.direction()); + let next_steps = next_steps(inst_primary, inst_secondary, direction, is_master_commit); -**Summary**: ", - sha = commit.sha, - ); + write!(&mut message, "\n{footer}\n{next_steps}").unwrap(); + + Ok(message) +} +/// Returns true if a summary table was written to `message`. +async fn write_stat_summary( + primary: ArtifactComparisonSummary, + secondary: ArtifactComparisonSummary, + hidden: bool, + message: &mut String, +) -> bool { if !primary.is_relevant() && !secondary.is_relevant() { - write!( - &mut message, - "This benchmark run did not return any relevant results.\n" - ) - .unwrap(); + message + .push_str("This benchmark run did not return any relevant results for this metric.\n"); + false } else { let primary_short_summary = generate_short_summary(&primary); let secondary_short_summary = generate_short_summary(&secondary); + if hidden { + message.push_str("
\nResults\n\n"); + } + write!( - &mut message, + message, r#" - Primary benchmarks: {primary_short_summary} - Secondary benchmarks: {secondary_short_summary} @@ -629,14 +707,14 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: ) .unwrap(); - write_summary_table(&primary, &secondary, true, &mut message); - } + write_summary_table(&primary, &secondary, true, message); - let direction = primary.direction().or(secondary.direction()); - let next_steps = next_steps(primary, secondary, direction, is_master_commit); - write!(&mut message, "\n{footer}\n{next_steps}").unwrap(); + if hidden { + message.push_str("
\n"); + } - message + true + } } fn next_steps( @@ -645,7 +723,7 @@ fn next_steps( direction: Option, 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",