Skip to content

Limit the inclusion of some artifact comparisons in triage #1282

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 8, 2022
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
25 changes: 22 additions & 3 deletions site/src/comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,20 @@ async fn populate_report(
(None, None) => return,
};

let entry = report.entry(direction).or_default();
let include_in_triage = match (primary.largest_change(), secondary.largest_change()) {
(Some(c), _) if c.magnitude() >= Magnitude::Medium => true,
(_, Some(c)) if c.magnitude() >= Magnitude::Medium => true,
_ => {
let primary_n = primary.num_changes();
let secondary_n = secondary.num_changes();
(primary_n * 2 + secondary_n) >= 6
}
};

entry.push(write_triage_summary(comparison, &primary, &secondary).await)
if include_in_triage {
let entry = report.entry(direction).or_default();
entry.push(write_triage_summary(comparison, &primary, &secondary).await);
}
}

/// A summary of a given comparison
Expand Down Expand Up @@ -323,6 +334,14 @@ impl ComparisonSummary {
.filter(|c| c.is_regression())
}

fn num_changes(&self) -> usize {
self.relevant_comparisons.len()
}

fn largest_change(&self) -> Option<&TestResultComparison> {
self.relevant_comparisons.first()
}

fn largest_improvement(&self) -> Option<&TestResultComparison> {
self.relevant_comparisons
.iter()
Expand Down Expand Up @@ -1058,7 +1077,7 @@ impl std::hash::Hash for TestResultComparison {
}

// The direction of a performance change
#[derive(PartialEq, Eq, Hash, Debug)]
#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
pub enum Direction {
Improvement,
Regression,
Expand Down
11 changes: 2 additions & 9 deletions triage/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ usage.

## Roster

- ecstatic-morse
- Mark-Simulacrum
- rylev
- pnkfelix
Expand Down Expand Up @@ -67,13 +66,7 @@ Understanding the comparison page:
*Warning*: max rss is much more variable than instruction count. Recheck the "Absolute
data" checkbox otherwise the noise becomes unmanageable.

- A change isn't significant unless one or more of the benchmarks changed by at
least 1%.

#### Dealing with noise

Some benchmarks are noisy. Those that are known to be noisy typically have their results
labeled as such with a `?`. These results can usually be dismissed if they are below 2.5% and are not accompanied by other regressions in other benchmarks.
For help understanding how to interpret results, consult the [comparison analysis documentation](../docs/comparison-analysis.md).

## Action

Expand All @@ -91,7 +84,7 @@ Difficult cases: the merge was a rollup of multiple PRs.
- Look through the PRs and try to determine which was the cause. Often you
can easily tell that one or more PRs could not have caused the change, e.g.
because they made trivial changes, documentation-only changes, etc.
- If there are still PRs left over, look at the 'detailed-query' page on perf.rlo: often, there is a single timing pass that improved significantly, and the name may give you a hint. You can find the page by expanding the dropdown for the build with the greatest change, then clicking on the percent change. Note that this does not work for `-doc` builds.
- If there are still PRs left over, look at the 'detailed-query' page on perf.rlo: often, there is a single timing pass that improved significantly, and the name may give you a hint. You can find the page by expanding the dropdown for the build with the greatest change, then clicking on the percent change.
- If you can't narrow it down to a single PR, in the rollup PR ask all the
authors who might be responsible.
- Once you have narrowed it down to a single PR, treat it like an easy case,
Expand Down