Skip to content

Fix triage endpoint not going past current commit #1568

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.

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

Merged
merged 2 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion collector/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ pub struct DeltaTime(#[serde(with = "round_float")] pub f64);
/// In the case of commits or tags this is an exact bound, but for dates
/// it's a best effort (i.e., if the bound is a date but there are no artifacts
/// for that date, we'll find the artifact that most closely matches).
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Default, Debug, Clone, PartialEq, Eq)]
pub enum Bound {
/// An unverified git commit (in sha form) or a tag of a commit (e.g., "1.53.0")
Commit(String),
/// A date in time
Date(NaiveDate),
/// No bound
#[default]
None,
}

Expand Down
1 change: 1 addition & 0 deletions site/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ pub mod triage {
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Request {
pub start: Bound,
#[serde(default)]
pub end: Bound,
}

Expand Down
9 changes: 4 additions & 5 deletions site/src/comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ pub async fn handle_triage(
let start_artifact = ctxt
.artifact_id_for_bound(start.clone(), true)
.ok_or(format!("could not find start commit for bound {:?}", start))?;
let end_artifact = ctxt
.artifact_id_for_bound(end.clone(), false)
.ok_or(format!("could not find end commit for bound {:?}", end))?;
// This gives a better error, but is still not great -- the common case here
// is that we've had a 422 error and as such had a fork. It's possible we
// could diagnose that and give a nicer error here telling the user which
Expand All @@ -49,8 +52,6 @@ pub async fn handle_triage(
let mut num_comparisons = 0;
let metric = Metric::InstructionsUser;
let benchmark_map = ctxt.get_benchmark_category_map().await;
// Track whether we are on the known last iteration
let mut last_iteration = false;

let end = loop {
let comparison = match compare_given_commits(
Expand Down Expand Up @@ -83,7 +84,7 @@ pub async fn handle_triage(
populate_report(&comparison, &benchmark_map, &mut report).await;

// If we already know this is the last iteration, we can stop
if last_iteration {
if comparison.b.artifact == end_artifact {
break before;
}

Expand All @@ -93,8 +94,6 @@ pub async fn handle_triage(
// We keep doing comparisons...
Some(n) => {
before = next;
// The next iteration is the last if the next bound is equal to the end bound
last_iteration = n != end;
next = n;
}
// There is no next commit so we stop.
Expand Down