Enqueue master commits in pr_queue

Author: rylev

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};
@@ -509,92 +509,117 @@ pub async fn post_finished(ctxt: &SiteCtxt) {
     }
     let conn = ctxt.conn().await;
     let index = ctxt.index.load();
-    let mut commits = index
+    let mut already_tested_commits = index
         .commits()
         .into_iter()
         .map(|c| c.sha.to_string())
         .collect::<HashSet<_>>();
-    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 = master_commits
+        .unwrap()
+        .into_iter()
+        .map(|c| c.sha)
+        .collect::<HashSet<_>>();
 
-    for aid in conn.in_progress_artifacts().await {
+    for aid in in_progress_artifacts {
         match aid {
             ArtifactId::Commit(c) => {
-                commits.remove(&c.sha);
+                already_tested_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 commit in queued_pr_commits
+        .into_iter()
+        .filter(|c| already_tested_commits.contains(&c.sha))
+    {
         if let Some(completed) = conn.mark_complete(&commit.sha).await {
             assert_eq!(completed, commit);
 
-            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 \
+            let is_master_commit = master_commits.contains(&commit.sha);
+            post_comparison_comment(commit, ctxt, is_master_commit).await;
+        }
+    }
+}
+
+async fn post_comparison_comment(commit: QueuedCommit, ctxt: &SiteCtxt, 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 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]({}).
+                please fix the regressions (either in this PR if it's not yet merged or \
+                in another PR) and do another perf run.",
+                    Direction::Improvement => "",
+                }
+            )
+        })
+        .unwrap_or(String::new());
+    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 bors_msg = if is_master_commit {
+        ""
+    } else {
+        "@bors rollup=never\n"
+    };
+    post_comment(
+        &ctxt.config,
+        commit.pr,
+        format!(
+            "Finished benchmarking 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
-                ),
-            )
-            .await;
-        }
-    }
+            commit.sha, comparison_url, summary, rollup_msg, next_steps_msg, bors_msg, label
+        ),
+    )
+    .await;
 }
 
 async fn categorize_benchmark(
-    commit: &database::QueuedCommit,
+    commit_sha: String,
+    parent_sha: String,
     ctxt: &SiteCtxt,
 ) -> (String, Option<Direction>) {
     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,
     )

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 {
@@ -34,6 +35,16 @@ pub enum MissingReason {
     InProgress(Option<Box<MissingReason>>),
 }
 
+impl MissingReason {
+    /// If the commit is a master commit get its PR and parent_sha
+    pub fn master_commit_pr_and_parent(&self) -> Option<(u32, &str)> {
+        match self {
+            Self::Master { pr, parent_sha } => Some((*pr, parent_sha)),
+            _ => None,
+        }
+    }
+}
+
 #[derive(Clone, Deserialize, Serialize, Debug, PartialEq, Eq)]
 pub struct TryCommit {
     pub sha: String,
@@ -177,6 +188,7 @@ impl SiteCtxt {
                     // All recent master commits should have an associated PR
                     MissingReason::Master {
                         pr: c.pr.unwrap_or(0),
+                        parent_sha: c.parent_sha,
                     },
                 )
             })

site/src/request_handlers/next_commit.rs

@@ -4,41 +4,47 @@ use collector::api::next_commit;
 use std::sync::Arc;
 
 pub async fn handle_next_commit(ctxt: Arc<SiteCtxt>) -> 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 commit = ctxt.missing_commits().await.into_iter().next();
+
+    let commit = if let Some((commit, missing_reason)) = 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 Some((pr, parent_sha)) = missing_reason.master_commit_pr_and_parent() {
+            let conn = ctxt.conn().await;
+            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 }
 }