From e1087d2f94c3dbf779ab5706b8c05b96d9208c45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 15 May 2022 12:58:39 +0200 Subject: [PATCH 1/5] Separate footer from summary table --- site/src/comparison.rs | 40 +++++++--------------------------------- site/src/github.rs | 1 + 2 files changed, 8 insertions(+), 33 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index 62f19c4f4..13a8a0676 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -17,6 +17,7 @@ use std::collections::{HashMap, HashSet}; use std::error::Error; use std::hash::Hash; use std::sync::Arc; +use std::fmt::Write; type BoxedError = Box; @@ -388,7 +389,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 +415,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 +536,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(); - } } /// Compare two bounds on a given stat @@ -1272,9 +1270,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 +1289,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 +1308,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 +1327,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 +1347,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 +1369,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 +1387,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 +1405,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..2565e7c31 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -630,6 +630,7 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: .unwrap(); write_summary_table(&primary, &secondary, true, &mut message); + write_summary_table_footer(&mut message); } let direction = primary.direction().or(secondary.direction()); From 33a346178b5fd5e3ca81cb7cd9742dacc14328d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sun, 15 May 2022 13:09:53 +0200 Subject: [PATCH 2/5] Include more stats in GH summary table --- site/src/comparison.rs | 4 +- site/src/github.rs | 104 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 99 insertions(+), 9 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index 13a8a0676..1ff565cea 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -15,9 +15,9 @@ use serde::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; -use std::fmt::Write; type BoxedError = Box; @@ -545,7 +545,7 @@ pub fn write_summary_table_footer(result: &mut String) { [^1]: *number of relevant changes* [^2]: *the arithmetic mean of the percent change*"# ) - .unwrap(); + .unwrap(); } /// Compare two bounds on a given stat diff --git a/site/src/github.rs b/site/src/github.rs index 2565e7c31..20d3d0678 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -1,15 +1,17 @@ use crate::api::github::Issue; use crate::comparison::{ - deserves_attention, write_summary_table, ArtifactComparisonSummary, Direction, + deserves_attention, write_summary_table, write_summary_table_footer, ArtifactComparisonSummary, + Direction, Magnitude, }; use crate::load::{Config, SiteCtxt, TryCommit}; use anyhow::Context as _; -use database::{ArtifactId, QueuedCommit}; +use database::{ArtifactId, Benchmark, QueuedCommit}; use reqwest::header::USER_AGENT; use serde::{Deserialize, Serialize}; -use std::collections::HashSet; +use collector::category::Category; +use std::collections::{HashMap, HashSet}; use std::{fmt::Write, sync::Arc, time::Duration}; type BoxedError = Box; @@ -566,13 +568,21 @@ async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_maste post_comment(&ctxt.config, pr, body).await; } -async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) -> String { - let comparison_url = format!( +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 +} + +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), + collector::Bound::Commit(commit.parent_sha.clone()), collector::Bound::Commit(commit.sha.clone()), "instructions:u".to_owned(), ctxt, @@ -603,12 +613,19 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: let footer = format!("{DISAGREEMENT}{errors}"); let mut message = format!( - "Finished benchmarking commit ({sha}): [comparison url]({comparison_url}). + "Finished benchmarking commit ({sha}): [comparison url]({inst_comparison_url}). **Summary**: ", sha = commit.sha, ); + let mut table_written = false; + + write!( + &mut message, + "## [Instruction count]({inst_comparison_url})" + ) + .unwrap(); if !primary.is_relevant() && !secondary.is_relevant() { write!( &mut message, @@ -630,6 +647,29 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: .unwrap(); write_summary_table(&primary, &secondary, true, &mut message); + table_written = true; + } + + if let Some((primary, secondary)) = analyze_secondary_stat( + ctxt, + &commit, + "max-rss", + &benchmark_map, + is_max_rss_interesting, + ) + .await + { + write!( + &mut message, + "\n## [Max RSS]({})", + make_comparison_url(&commit, Some("max-rss")) + ) + .unwrap(); + write_summary_table(&primary, &secondary, true, &mut message); + table_written = true; + } + + if table_written { write_summary_table_footer(&mut message); } @@ -640,6 +680,56 @@ 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, +>( + ctxt: &SiteCtxt, + commit: &QueuedCommit, + stat: &str, + benchmark_map: &HashMap, + is_interesting: F, +) -> Option<(ArtifactComparisonSummary, ArtifactComparisonSummary)> { + let comparison = match crate::comparison::compare( + collector::Bound::Commit(commit.parent_sha.clone()), + collector::Bound::Commit(commit.sha.clone()), + stat.to_string(), + ctxt, + ) + .await + { + Ok(Some(c)) => c, + _ => return None, + }; + + let (primary, secondary) = comparison.summarize_by_category(benchmark_map); + if !primary.is_relevant() && !secondary.is_relevant() { + return None; + } + + if is_interesting(&primary, &secondary) { + Some((primary, secondary)) + } else { + None + } +} + fn next_steps( primary: ArtifactComparisonSummary, secondary: ArtifactComparisonSummary, From 935c4da2f7536d3bba3f287cb272c5db105f0adf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 16 May 2022 12:24:33 +0200 Subject: [PATCH 3/5] Turn Stat into an enum --- site/src/api.rs | 3 +- site/src/comparison.rs | 67 +++++++++++++++++++++++++++++++++++------- site/src/github.rs | 62 ++++++++++++++------------------------ 3 files changed, 81 insertions(+), 51 deletions(-) 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 1ff565cea..fb8870f0d 100644 --- 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,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(); @@ -180,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). @@ -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, 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, BoxedError> { @@ -585,7 +632,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` @@ -832,7 +879,7 @@ impl HistoricalDataMap { ctxt: &SiteCtxt, from: ArtifactId, master_commits: &[collector::MasterCommit], - stat: String, + stat: Stat, ) -> Result { let mut historical_data = HashMap::new(); @@ -854,7 +901,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()) diff --git a/site/src/github.rs b/site/src/github.rs index 20d3d0678..fb98c4702 100644 --- 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( ctxt: &SiteCtxt, commit: &QueuedCommit, - stat: &str, + stat: Stat, benchmark_map: &HashMap, 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, 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", From 98b4abad27551d9c73deadc08a50422fb4fda715 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 18 May 2022 14:16:40 +0200 Subject: [PATCH 4/5] Always add Max RSS and cycles to PR summary --- site/src/comparison.rs | 15 ---- site/src/github.rs | 160 ++++++++++++++++++++--------------------- 2 files changed, 78 insertions(+), 97 deletions(-) diff --git a/site/src/comparison.rs b/site/src/comparison.rs index fb8870f0d..2f2e4b317 100644 --- a/site/src/comparison.rs +++ b/site/src/comparison.rs @@ -416,21 +416,6 @@ pub(crate) fn deserves_attention_icount( } } -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, diff --git a/site/src/github.rs b/site/src/github.rs index fb98c4702..70f0f3b77 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -1,7 +1,7 @@ use crate::api::github::Issue; use crate::comparison::{ - deserves_attention_icount, deserves_attention_maxrss, write_summary_table, - write_summary_table_footer, ArtifactComparisonSummary, Direction, Stat, + deserves_attention_icount, write_summary_table, write_summary_table_footer, + ArtifactComparisonSummary, Direction, Stat, }; use crate::load::{Config, SiteCtxt, TryCommit}; @@ -11,7 +11,9 @@ use reqwest::header::USER_AGENT; use serde::{Deserialize, Serialize}; use collector::category::Category; + use std::collections::{HashMap, HashSet}; + use std::{fmt::Write, sync::Arc, time::Duration}; type BoxedError = Box; @@ -578,6 +580,40 @@ fn make_comparison_url(commit: &QueuedCommit, stat: Stat) -> String { } async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) -> String { + let benchmark_map = ctxt.get_benchmark_category_map().await; + + let mut message = format!( + "Finished benchmarking commit ({sha}): [comparison url]({comparison_url}). + +**Summary**: ", + sha = commit.sha, + comparison_url = make_comparison_url(&commit, Stat::Instructions) + ); + + let mut table_written = false; + let stats = vec![ + ("Instruction count", Stat::Instructions, false), + ("Max RSS (memory usage)", Stat::MaxRSS, true), + ("Cycles", Stat::Cycles, true), + ]; + + for (title, stat, hidden) in stats { + table_written |= write_stat_summary( + &benchmark_map, + &commit, + ctxt, + &title, + stat, + hidden, + &mut message, + ) + .await; + } + + if table_written { + write_summary_table_footer(&mut message); + } + let comparison = match crate::comparison::compare( collector::Bound::Commit(commit.parent_sha.clone()), collector::Bound::Commit(commit.sha.clone()), @@ -589,7 +625,6 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: Ok(Some(c)) => c, _ => return String::from("ERROR categorizing benchmark run!"), }; - let errors = if !comparison.newly_failed_benchmarks.is_empty() { let benchmarks = comparison .newly_failed_benchmarks @@ -601,76 +636,12 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: } else { String::new() }; - - let benchmark_map = ctxt.get_benchmark_category_map().await; let (primary, secondary) = comparison.summarize_by_category(&benchmark_map); 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 inst_comparison_url = make_comparison_url(&commit, Stat::Instructions); - let mut message = format!( - "Finished benchmarking commit ({sha}): [comparison url]({inst_comparison_url}). - -**Summary**: ", - sha = commit.sha, - ); - - let mut table_written = false; - - write!( - &mut message, - "## [Instruction count]({inst_comparison_url})" - ) - .unwrap(); - if !primary.is_relevant() && !secondary.is_relevant() { - write!( - &mut message, - "This benchmark run did not return any relevant results.\n" - ) - .unwrap(); - } else { - let primary_short_summary = generate_short_summary(&primary); - let secondary_short_summary = generate_short_summary(&secondary); - - write!( - &mut message, - r#" -- Primary benchmarks: {primary_short_summary} -- Secondary benchmarks: {secondary_short_summary} - -"# - ) - .unwrap(); - - write_summary_table(&primary, &secondary, true, &mut message); - table_written = true; - } - - if let Some((primary, secondary)) = analyze_secondary_stat( - ctxt, - &commit, - Stat::MaxRSS, - &benchmark_map, - deserves_attention_maxrss, - ) - .await - { - write!( - &mut message, - "\n## [Max RSS]({})", - make_comparison_url(&commit, Stat::MaxRSS) - ) - .unwrap(); - write_summary_table(&primary, &secondary, true, &mut message); - table_written = true; - } - - if table_written { - write_summary_table_footer(&mut 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(); @@ -678,16 +649,15 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: message } -async fn analyze_secondary_stat( - ctxt: &SiteCtxt, +async fn write_stat_summary( + benchmark_map: &HashMap, commit: &QueuedCommit, + ctxt: &SiteCtxt, + title: &str, stat: Stat, - benchmark_map: &HashMap, - is_interesting: F, -) -> Option<(ArtifactComparisonSummary, ArtifactComparisonSummary)> -where - F: FnOnce(&ArtifactComparisonSummary, &ArtifactComparisonSummary) -> bool, -{ + hidden: bool, + message: &mut String, +) -> bool { let comparison = match crate::comparison::compare( collector::Bound::Commit(commit.parent_sha.clone()), collector::Bound::Commit(commit.sha.clone()), @@ -697,18 +667,44 @@ where .await { Ok(Some(c)) => c, - _ => return None, + _ => panic!(), //return Err("ERROR categorizing benchmark run!".to_owned()), }; + message.push_str(&format!( + "## [{title}]({})\n", + make_comparison_url(commit, stat) + )); + let (primary, secondary) = comparison.summarize_by_category(benchmark_map); if !primary.is_relevant() && !secondary.is_relevant() { - return None; - } - - if is_interesting(&primary, &secondary) { - Some((primary, secondary)) + message + .push_str("This benchmark run did not return any relevant results for this metric.\n"); + false } else { - None + 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!( + message, + r#" +- Primary benchmarks: {primary_short_summary} +- Secondary benchmarks: {secondary_short_summary} + +"# + ) + .unwrap(); + + write_summary_table(&primary, &secondary, true, message); + + if hidden { + message.push_str("
\n"); + } + + true } } From a77669f96cca8ed27016c06df2474862c157a32b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Wed, 18 May 2022 14:48:26 +0200 Subject: [PATCH 5/5] Refactor stat summaries --- site/src/github.rs | 151 ++++++++++++++++++++++++--------------------- 1 file changed, 80 insertions(+), 71 deletions(-) diff --git a/site/src/github.rs b/site/src/github.rs index 70f0f3b77..f05ad8e06 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -1,18 +1,16 @@ use crate::api::github::Issue; use crate::comparison::{ - deserves_attention_icount, write_summary_table, write_summary_table_footer, + deserves_attention_icount, write_summary_table, write_summary_table_footer, ArtifactComparison, ArtifactComparisonSummary, Direction, Stat, }; use crate::load::{Config, SiteCtxt, TryCommit}; use anyhow::Context as _; -use database::{ArtifactId, Benchmark, QueuedCommit}; +use database::{ArtifactId, QueuedCommit}; use reqwest::header::USER_AGENT; use serde::{Deserialize, Serialize}; -use collector::category::Category; - -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::{fmt::Write, sync::Arc, time::Duration}; @@ -565,7 +563,10 @@ 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; } @@ -579,54 +580,43 @@ fn make_comparison_url(commit: &QueuedCommit, stat: Stat) -> String { ) } -async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) -> String { +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()), + stat, + ctxt, + ) + .await + { + Ok(Some(c)) => Ok(c), + _ => Err("ERROR categorizing benchmark run!".to_owned()), + } +} + +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**: ", +**Summary**:\n\n", sha = commit.sha, comparison_url = make_comparison_url(&commit, Stat::Instructions) ); - let mut table_written = false; - let stats = vec![ - ("Instruction count", Stat::Instructions, false), - ("Max RSS (memory usage)", Stat::MaxRSS, true), - ("Cycles", Stat::Cycles, true), - ]; + let inst_comparison = calculate_stat_comparison(ctxt, &commit, Stat::Instructions).await?; - for (title, stat, hidden) in stats { - table_written |= write_stat_summary( - &benchmark_map, - &commit, - ctxt, - &title, - stat, - hidden, - &mut message, - ) - .await; - } - - if table_written { - write_summary_table_footer(&mut message); - } - - let comparison = match crate::comparison::compare( - collector::Bound::Commit(commit.parent_sha.clone()), - collector::Bound::Commit(commit.sha.clone()), - Stat::Instructions, - ctxt, - ) - .await - { - Ok(Some(c)) => c, - _ => return String::from("ERROR categorizing benchmark run!"), - }; - let errors = if !comparison.newly_failed_benchmarks.is_empty() { - let benchmarks = comparison + let errors = if !inst_comparison.newly_failed_benchmarks.is_empty() { + let benchmarks = inst_comparison .newly_failed_benchmarks .iter() .map(|(benchmark, _)| format!("- {benchmark}")) @@ -636,46 +626,65 @@ async fn summarize_run(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: } else { String::new() }; - let (primary, secondary) = comparison.summarize_by_category(&benchmark_map); + 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?, + ), + ]; + + 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 direction = primary.direction().or(secondary.direction()); - let next_steps = next_steps(primary, secondary, direction, is_master_commit); + let direction = inst_primary.direction().or(inst_secondary.direction()); + let next_steps = next_steps(inst_primary, inst_secondary, direction, is_master_commit); + write!(&mut message, "\n{footer}\n{next_steps}").unwrap(); - message + Ok(message) } +/// Returns true if a summary table was written to `message`. async fn write_stat_summary( - benchmark_map: &HashMap, - commit: &QueuedCommit, - ctxt: &SiteCtxt, - title: &str, - stat: Stat, + primary: ArtifactComparisonSummary, + secondary: ArtifactComparisonSummary, hidden: bool, message: &mut String, ) -> bool { - let comparison = match crate::comparison::compare( - collector::Bound::Commit(commit.parent_sha.clone()), - collector::Bound::Commit(commit.sha.clone()), - stat, - ctxt, - ) - .await - { - Ok(Some(c)) => c, - _ => panic!(), //return Err("ERROR categorizing benchmark run!".to_owned()), - }; - - message.push_str(&format!( - "## [{title}]({})\n", - make_comparison_url(commit, stat) - )); - - let (primary, secondary) = comparison.summarize_by_category(benchmark_map); if !primary.is_relevant() && !secondary.is_relevant() { message .push_str("This benchmark run did not return any relevant results for this metric.\n");