Skip to content

Commit d9aedf5

Browse files
committed
Address PR feedback
1 parent 7a258c8 commit d9aedf5

File tree

3 files changed

+10
-9
lines changed

3 files changed

+10
-9
lines changed

docs/comparison-analysis.md

+5-5
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ How many _significant_ test results indicate performance changes and what is the
3333
* given 20 changes of different kinds all of low magnitude, the result is mixed unless only 2 or fewer of the changes are of one kind.
3434
* given 5 changes of different kinds all of low magnitude, the result is always mixed.
3535

36-
Whether we actually _report_ an analysis or not depends on the context and how relevant we find the summary of the results over all (see below for an explanation of how summary relevant is determined). For example, in pull request performance "try" runs, we report a performance change if results are "somewhat relevant", while for the triage report, we only report if the we are confident the results are "definitely relevant".
36+
Whether we actually _report_ an analysis or not depends on the context and how relevant we find the summary of the results over all (see below for an explanation of how the relevance of a summary is determined). For example, in pull request performance "try" runs, we report a performance change if results are "somewhat relevant", while for the triage report, we only report if the we are confident the results are "definitely relevant".
3737

3838
### What makes a test result significant?
3939

@@ -60,14 +60,14 @@ Magnitude is a combination of two factors:
6060
* how large a change is regardless of the direction of the change
6161
* how much that change went over the significance threshold
6262

63-
If a large change only happens to go over the significance threshold by a small factor, then the over magnitude of the change is considered small.
63+
As an example, if a change that is large in absolute terms only exceeds the significance threshold by a small factor, then the overall magnitude of the change is considered small.
6464

6565
#### Relevance algorithm
6666

6767
The actual algorithm for determining relevance of a comparison summary may change, but in general the following rules apply:
68-
* High relevance: any number of very large or large changes, a small amount of medium changes, or a large amount of small or very small changes.
69-
* Medium relevance: any number of very large or large changes, any medium change, or smaller but still substantial amount of small or very small changes.
70-
* Small relevance: if it doesn't fit into the above two categories, it ends in this category.
68+
* High relevance: any number of very large or large changes, a small amount of medium changes, or a large number of small or very small changes.
69+
* Medium relevance: any number of very large or large changes, any medium change, or smaller but still substantial number of small or very small changes.
70+
* Low relevance: if it doesn't fit into the above two categories, it ends in this category.
7171

7272
### "Dodgy" Test Cases
7373

docs/glossary.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ The following is a glossary of domain specific terminology. Although benchmarks
3636
* **test result comparison**: the delta between two test results for the same test case but (optionally) different artifacts. The [comparison page](https://perf.rust-lang.org/compare.html) lists all the test result comparisons as percentages between two runs.
3737
* **significance threshold**: the threshold at which a test result comparison is considered "significant" (i.e., a real change in performance and not just noise). You can see how this is calculated [here](https://github.com/rust-lang/rustc-perf/blob/master/docs/comparison-analysis.md#what-makes-a-test-result-significant).
3838
* **significant test result comparison**: a test result comparison above the significance threshold. Significant test result comparisons can be thought of as being "statistically significant".
39-
* **relevant test result comparison**: a test result comparison can be significant but still not be relevant (i.e., worth paying attention to). Relevance is a factor of the test result comparison's *magnitude*. Comparisons are considered relevant if they have a small magnitude or more. This term is often used to mean "significant *and* relevant" since relevant changes are necessarily also significant.
39+
* **relevant test result comparison**: a test result comparison can be significant but still not be relevant (i.e., worth paying attention to). Relevance is a factor of the test result comparison's significance and magnitude. Comparisons are considered relevant if they are significant and have at least a small magnitude .
4040
* **test result comparison magnitude**: how "large" the delta is between the two test result's under comparison. This is determined by the average of two factors: the absolute size of the change (i.e., a change of 5% is larger than a change of 1%) and the amount above the significance threshold (i.e., a change that is 5x the significance threshold is larger than a change 1.5x the significance threshold).
4141
* **dodgy test case**: a test case for which the significance threshold is significantly large indicating a high amount of variability in the test and thus making it necessary to be somewhat skeptical of any results too close to the significance threshold.
4242

site/src/comparison.rs

+4-3
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,6 @@ impl ComparisonSummary {
184184
let mut comparisons = comparison
185185
.statistics
186186
.iter()
187-
.filter(|c| c.is_significant())
188187
.filter(|c| c.is_relevant())
189188
.inspect(|c| {
190189
if c.is_improvement() {
@@ -1037,9 +1036,11 @@ impl TestResultComparison {
10371036
Some(change.abs() / threshold)
10381037
}
10391038

1040-
/// Whether the comparison is relevant or not
1039+
/// Whether the comparison is relevant or not.
1040+
///
1041+
/// Relevance is a function of significance and magnitude.
10411042
fn is_relevant(&self) -> bool {
1042-
self.magnitude().is_small_or_above()
1043+
self.is_significant() && self.magnitude().is_small_or_above()
10431044
}
10441045

10451046
/// The magnitude of the change.

0 commit comments

Comments
 (0)