Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Leave perf result comment on PRs after the post-merge perf run is completed #992

Merged
merged 4 commits into from
Sep 6, 2021
Merged

Leave perf result comment on PRs after the post-merge perf run is completed #992

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ef307e
Refactor missing_commits to be clearer
rylev Sep 3, 2021
f78ebf6
Enqueue master commits in pr_queue
rylev Sep 3, 2021
75d14ff
Don't return master commits as try commit
rylev Sep 6, 2021
b2383e9
Add a test
rylev Sep 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 83 additions & 58 deletions site/src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
)
Expand Down
64 changes: 39 additions & 25 deletions site/src/load.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub enum MissingReason {
/// This commmit has not yet been benchmarked
Master {
pr: u32,
parent_sha: String,
},
TryParent,
Try {
Expand All @@ -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,
Expand Down Expand Up @@ -147,27 +158,26 @@ impl SiteCtxt {

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_try_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 mut all_commits = index
.commits()
.iter()
.map(|commit| commit.sha.clone())
.collect::<HashSet<_>>();

let now = Utc::now();
let mut missing = commits
.iter()
.cloned()
let mut master_commits = master_commits
.into_iter()
.filter(|c| now.signed_duration_since(c.time) < Duration::days(29))
.map(|c| {
(
Expand All @@ -178,29 +188,33 @@ impl SiteCtxt {
// All recent master commits should have an associated PR
MissingReason::Master {
pr: c.pr.unwrap_or(0),
parent_sha: c.parent_sha,
},
)
})
.collect::<Vec<_>>();
missing.reverse();
let mut commits = Vec::new();
commits.reserve(queued_commits.len() * 2); // Two commits per every try commit
master_commits.reverse();

let mut missing = Vec::with_capacity(queued_try_commits.len() * 2 + master_commits.len()); // Two commits per every try commit and all master commits
for database::QueuedCommit {
sha,
parent_sha,
pr,
include,
exclude,
runs,
} in queued_commits
} in queued_try_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));
if let Some((try_parent, _)) = master_commits
.iter()
.find(|(m, _)| m.sha == parent_sha.as_str())
{
missing.push((try_parent.clone(), MissingReason::TryParent));
}
commits.push((
missing.push((
Commit {
sha: sha.to_string(),
date: Date::ymd_hms(2001, 01, 01, 0, 0, 0),
Expand All @@ -213,39 +227,39 @@ impl SiteCtxt {
},
));
}
commits.extend(missing);
missing.extend(master_commits);

for aid in in_progress_artifacts {
match aid {
ArtifactId::Commit(c) => {
let previous = commits
let previous = missing
.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)));
all_commits.remove(&c.sha);
missing.insert(0, (c, MissingReason::InProgress(previous)));
}
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
}
}
}

let mut seen = HashSet::with_capacity(commits.len());
seen.extend(have);
let mut already_tested = HashSet::with_capacity(all_commits.len());
already_tested.extend(all_commits);

// Remove commits from missing that have already been tested
// 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);
while i != missing.len() {
if !already_tested.insert(missing[i].0.sha.clone()) {
missing.remove(i);
} else {
i += 1;
}
}

commits
missing
}
}

Expand Down
Loading