diff --git a/site/src/github.rs b/site/src/github.rs index d365d5065..66a67c7cb 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -3,7 +3,7 @@ use crate::comparison::{ComparisonSummary, Direction}; use crate::load::{Config, SiteCtxt, TryCommit}; use anyhow::Context as _; -use database::ArtifactId; +use database::{ArtifactId, QueuedCommit}; use hashbrown::HashSet; use reqwest::header::USER_AGENT; use serde::{Deserialize, Serialize}; @@ -501,6 +501,8 @@ pub struct PostComment { pub body: String, } +/// Post messages to GitHub for all queued commits that have +/// not yet been marked as completed. pub async fn post_finished(ctxt: &SiteCtxt) { // If the github token is not configured, do not run this -- we don't want // to mark things as complete without posting the comment. @@ -509,92 +511,128 @@ pub async fn post_finished(ctxt: &SiteCtxt) { } let conn = ctxt.conn().await; let index = ctxt.index.load(); - let mut commits = index + let mut known_commits = index .commits() .into_iter() .map(|c| c.sha.to_string()) .collect::>(); - let queued = conn.queued_commits().await; + let (master_commits, queued_pr_commits, in_progress_artifacts) = futures::join!( + collector::master_commits(), + conn.queued_commits(), + conn.in_progress_artifacts() + ); + let master_commits = if let Ok(mcs) = master_commits { + mcs.into_iter().map(|c| c.sha).collect::>() + } else { + // If we can't fetch master commits, return. + // We'll eventually try again later + return; + }; - for aid in conn.in_progress_artifacts().await { + for aid in in_progress_artifacts { match aid { ArtifactId::Commit(c) => { - commits.remove(&c.sha); + known_commits.remove(&c.sha); } ArtifactId::Tag(_) => { - // do nothing, for now, though eventually we'll want an artifact - // queue + // do nothing, for now, though eventually we'll want an artifact queue } } } - for commit in queued { - if !commits.contains(&commit.sha) { - continue; - } - - // This commit has been benchmarked. + for queued_commit in queued_pr_commits + .into_iter() + .filter(|c| known_commits.contains(&c.sha)) + { + if let Some(completed) = conn.mark_complete(&queued_commit.sha).await { + assert_eq!(completed, queued_commit); - if let Some(completed) = conn.mark_complete(&commit.sha).await { - assert_eq!(completed, commit); + let is_master_commit = master_commits.contains(&queued_commit.sha); + post_comparison_comment(ctxt, queued_commit, is_master_commit).await; + } + } +} - let comparison_url = format!( - "https://perf.rust-lang.org/compare.html?start={}&end={}", - commit.parent_sha, commit.sha - ); - let (summary, direction) = categorize_benchmark(&commit, ctxt).await; - let label = match direction { - Some(Direction::Regression | Direction::Mixed) => "+perf-regression", - Some(Direction::Improvement) | None => "-perf-regression", - }; - let msg = direction - .map(|d| { - format!( - "While you can manually mark this PR as fit \ +/// Posts a comment to GitHub summarizing the comparison of the queued commit with its parent +/// +/// `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 comparison_url = format!( + "https://perf.rust-lang.org/compare.html?start={}&end={}", + commit.parent_sha, commit.sha + ); + let (summary, direction) = + categorize_benchmark(commit.sha.clone(), commit.parent_sha, ctxt).await; + let label = match direction { + Some(Direction::Regression | Direction::Mixed) => "+perf-regression", + Some(Direction::Improvement) | None => "-perf-regression", + }; + let rollup_msg = if is_master_commit { + "" + } else { + "Benchmarking this pull request likely means that it is \ +perf-sensitive, so we're automatically marking it as not fit \ +for rolling up. " + }; + let next_steps_msg = direction + .map(|d| { + format!( + "{}{}", + if is_master_commit { + "" + } else { + "While you can manually mark this PR as fit \ for rollup, we strongly recommend not doing so since this PR led to changes in \ - compiler perf.{}", - match d { - Direction::Regression | Direction::Mixed => - "\n\n**Next Steps**: If you can justify the \ + compiler perf." + }, + match d { + Direction::Regression | Direction::Mixed => + "\n\n**Next Steps**: If you can justify the \ regressions found in this perf run, please indicate this with \ `@rustbot label: +perf-regression-triaged` along with \ sufficient written justification. If you cannot justify the regressions \ - please fix the regressions and do another perf run. If the next run shows \ - neutral or positive results, the label will be automatically removed.", - Direction::Improvement => "", - } - ) - }) - .unwrap_or(String::new()); - - post_comment( - &ctxt.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. {} - -@bors rollup=never -@rustbot label: +S-waiting-on-review -S-waiting-on-perf {}", - commit.sha, comparison_url, summary, msg, label - ), + please fix the regressions (either in this PR if it's not yet merged or \ + in another PR), and then add the `perf-regression-triaged` label to this PR.", + Direction::Improvement => "", + } ) - .await; - } - } + }) + .unwrap_or(String::new()); + let bors_msg = if is_master_commit { + "" + } else { + "@bors rollup=never\n" + }; + post_comment( + &ctxt.config, + commit.pr, + format!( + "Finished benchmarking commit ({sha}): [comparison url]({url}). + +**Summary**: {summary} + +{rollup}{next_steps} +{bors} +@rustbot label: +S-waiting-on-review -S-waiting-on-perf {label}", + sha = commit.sha, + url = comparison_url, + summary = summary, + rollup = rollup_msg, + next_steps = next_steps_msg, + bors = bors_msg, + label = label + ), + ) + .await; } async fn categorize_benchmark( - commit: &database::QueuedCommit, + commit_sha: String, + parent_sha: String, ctxt: &SiteCtxt, ) -> (String, Option) { let comparison = match crate::comparison::compare( - collector::Bound::Commit(commit.parent_sha.clone()), - collector::Bound::Commit(commit.sha.clone()), + collector::Bound::Commit(parent_sha), + collector::Bound::Commit(commit_sha), "instructions:u".to_owned(), ctxt, ) diff --git a/site/src/load.rs b/site/src/load.rs index 4d10429ea..80d86d55d 100644 --- a/site/src/load.rs +++ b/site/src/load.rs @@ -23,6 +23,7 @@ pub enum MissingReason { /// This commmit has not yet been benchmarked Master { pr: u32, + parent_sha: String, }, TryParent, Try { @@ -145,110 +146,225 @@ impl SiteCtxt { self.pool.connection().await } + /// Returns the not yet tested commits pub async fn missing_commits(&self) -> Vec<(Commit, MissingReason)> { let conn = self.conn().await; - let (master_commits, queued_commits, in_progress_artifacts) = futures::join!( + let (master_commits, queued_pr_commits, in_progress_artifacts) = futures::join!( collector::master_commits(), conn.queued_commits(), conn.in_progress_artifacts() ); - let commits = master_commits + let master_commits = master_commits .map_err(|e| anyhow::anyhow!("{:?}", e)) .context("getting master commit list") .unwrap(); let index = self.index.load(); - let mut have = index + let all_commits = index .commits() .iter() .map(|commit| commit.sha.clone()) .collect::>(); - let now = Utc::now(); - let mut missing = commits - .iter() - .cloned() - .filter(|c| now.signed_duration_since(c.time) < Duration::days(29)) - .map(|c| { - ( - Commit { - sha: c.sha, - date: Date(c.time), - }, - // All recent master commits should have an associated PR - MissingReason::Master { - pr: c.pr.unwrap_or(0), - }, - ) - }) - .collect::>(); - missing.reverse(); - let mut commits = Vec::new(); - commits.reserve(queued_commits.len() * 2); // Two commits per every try commit - for database::QueuedCommit { - sha, - parent_sha, - pr, - include, - exclude, - runs, - } in queued_commits - { - // Enqueue the `TryParent` commit before the `TryCommit` itself, so that - // all of the `try` run's data is complete when the benchmark results - // of that commit are available. - if let Some((commit, _)) = missing.iter().find(|c| c.0.sha == *parent_sha.as_str()) { - commits.push((commit.clone(), MissingReason::TryParent)); - } - commits.push(( + calculate_missing( + master_commits, + queued_pr_commits, + in_progress_artifacts, + all_commits, + ) + } +} + +fn calculate_missing( + master_commits: Vec, + queued_pr_commits: Vec, + in_progress_artifacts: Vec, + mut all_commits: HashSet, +) -> Vec<(Commit, MissingReason)> { + let now = Utc::now(); + let mut master_commits = master_commits + .into_iter() + .filter(|c| now.signed_duration_since(c.time) < Duration::days(29)) + .map(|c| { + ( Commit { - sha: sha.to_string(), - date: Date::ymd_hms(2001, 01, 01, 0, 0, 0), + sha: c.sha, + date: Date(c.time), }, - MissingReason::Try { - pr, - include, - exclude, - runs, + // All recent master commits should have an associated PR + MissingReason::Master { + pr: c.pr.unwrap_or(0), + parent_sha: c.parent_sha, }, - )); + ) + }) + .collect::>(); + master_commits.reverse(); + let mut missing = Vec::with_capacity(queued_pr_commits.len() * 2 + master_commits.len()); + for database::QueuedCommit { + sha, + parent_sha, + pr, + include, + exclude, + runs, + } in queued_pr_commits + .into_iter() + // filter out any queued PR master commits (leaving only try commits) + .filter(|c| !master_commits.iter().any(|(mc, _)| mc.sha == c.sha)) + { + // Enqueue the `TryParent` commit before the `TryCommit` itself, so that + // all of the `try` run's data is complete when the benchmark results + // of that commit are available. + if let Some((try_parent, _)) = master_commits + .iter() + .find(|(m, _)| m.sha == parent_sha.as_str()) + { + missing.push((try_parent.clone(), MissingReason::TryParent)); } - commits.extend(missing); - - for aid in in_progress_artifacts { - match aid { - ArtifactId::Commit(c) => { - let previous = commits - .iter() - .find(|(i, _)| i.sha == c.sha) - .map(|v| Box::new(v.1.clone())); - have.remove(&c.sha); - commits.insert(0, (c, MissingReason::InProgress(previous))); - } - ArtifactId::Tag(_) => { - // do nothing, for now, though eventually we'll want an artifact - // queue - } + missing.push(( + Commit { + sha: sha.to_string(), + date: Date::ymd_hms(2001, 01, 01, 0, 0, 0), + }, + MissingReason::Try { + pr, + include, + exclude, + runs, + }, + )); + } + missing.extend(master_commits); + for aid in in_progress_artifacts { + match aid { + ArtifactId::Commit(c) => { + let previous = missing + .iter() + .find(|(i, _)| i.sha == c.sha) + .map(|v| Box::new(v.1.clone())); + all_commits.remove(&c.sha); + missing.insert(0, (c, MissingReason::InProgress(previous))); } - } - - let mut seen = HashSet::with_capacity(commits.len()); - seen.extend(have); - - // FIXME: replace with Vec::drain_filter when it stabilizes - let mut i = 0; - while i != commits.len() { - if !seen.insert(commits[i].0.sha.clone()) { - commits.remove(i); - } else { - i += 1; + ArtifactId::Tag(_) => { + // do nothing, for now, though eventually we'll want an artifact queue } } - - commits } + let mut already_tested = HashSet::with_capacity(all_commits.len()); + already_tested.extend(all_commits); + let mut i = 0; + while i != missing.len() { + if !already_tested.insert(missing[i].0.sha.clone()) { + missing.remove(i); + } else { + i += 1; + } + } + missing } /// One decimal place rounded percent #[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] pub struct Percent(#[serde(with = "collector::round_float")] pub f64); + +#[cfg(test)] +mod tests { + use std::str::FromStr; + + use collector::MasterCommit; + use database::QueuedCommit; + + use super::*; + #[test] + fn calculates_missing_correct() { + let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); + let master_commits = vec![ + // A not yet tested commit + MasterCommit { + sha: "123".into(), + parent_sha: "345".into(), + pr: Some(11), + time, + }, + // An already tested commit + MasterCommit { + sha: "abc".into(), + parent_sha: "def".into(), + pr: Some(90), + time, + }, + // A queued PR commit + MasterCommit { + sha: "foo".into(), + parent_sha: "bar".into(), + pr: Some(77), + time, + }, + ]; + let queued_pr_commits = vec![ + // A master commit + QueuedCommit { + sha: "foo".into(), + parent_sha: "bar".into(), + pr: 77, + include: None, + exclude: None, + runs: None, + }, + // A try run + QueuedCommit { + sha: "baz".into(), + parent_sha: "foo".into(), + pr: 101, + include: None, + exclude: None, + runs: None, + }, + ]; + let in_progress_artifacts = vec![]; + let mut all_commits = HashSet::new(); + all_commits.insert(master_commits[1].sha.clone()); + + let expected = vec![ + ( + Commit { + sha: "foo".into(), + date: database::Date(time), + }, + MissingReason::TryParent, + ), + ( + Commit { + sha: "baz".into(), + date: database::Date(time), + }, + MissingReason::Try { + pr: 101, + include: None, + exclude: None, + runs: None, + }, + ), + ( + Commit { + sha: "123".into(), + date: database::Date(time), + }, + MissingReason::Master { + pr: 11, + parent_sha: "345".into(), + }, + ), + ]; + assert_eq!( + expected, + calculate_missing( + master_commits, + queued_pr_commits, + in_progress_artifacts, + all_commits + ) + ); + } +} diff --git a/site/src/request_handlers/next_commit.rs b/site/src/request_handlers/next_commit.rs index 3543c6cab..def4b17e5 100644 --- a/site/src/request_handlers/next_commit.rs +++ b/site/src/request_handlers/next_commit.rs @@ -1,44 +1,54 @@ -use crate::load::SiteCtxt; +use crate::load::{MissingReason, SiteCtxt}; use collector::api::next_commit; use std::sync::Arc; pub async fn handle_next_commit(ctxt: Arc) -> next_commit::Response { - let commit = ctxt - .missing_commits() - .await - .into_iter() - .next() - .map(|(commit, missing_reason)| { - let (include, exclude, runs) = match missing_reason { - crate::load::MissingReason::Try { + let next_commit = ctxt.missing_commits().await.into_iter().next(); + + let next_commit = if let Some((commit, missing_reason)) = next_commit { + // If we're going to run a master commit next, make sure + // it's been enqueued in the pull_request_build table + if let MissingReason::Master { pr, ref parent_sha } = missing_reason { + let conn = ctxt.conn().await; + // TODO: add capability of doing the following in one step + // to avoid possibile illegal inbetween states. + conn.queue_pr(pr, None, None, None).await; + conn.pr_attach_commit(pr, &commit.sha, parent_sha).await; + } + let (include, exclude, runs) = match missing_reason { + crate::load::MissingReason::Try { + include, + exclude, + runs, + .. + } => (include, exclude, runs), + crate::load::MissingReason::InProgress(Some(previous)) => { + if let crate::load::MissingReason::Try { include, exclude, runs, .. - } => (include, exclude, runs), - crate::load::MissingReason::InProgress(Some(previous)) => { - if let crate::load::MissingReason::Try { - include, - exclude, - runs, - .. - } = *previous - { - (include, exclude, runs) - } else { - (None, None, None) - } + } = *previous + { + (include, exclude, runs) + } else { + (None, None, None) } - _ => (None, None, None), - }; - next_commit::Commit { - sha: commit.sha, - include, - exclude, - runs, } - }); + _ => (None, None, None), + }; + Some(next_commit::Commit { + sha: commit.sha, + include, + exclude, + runs, + }) + } else { + None + }; - next_commit::Response { commit } + next_commit::Response { + commit: next_commit, + } }