Skip to content

Commit 438e38b

Browse files
committed
Improve triage readme to better reflect current practices
1 parent d6d0634 commit 438e38b

File tree

1 file changed

+58
-22
lines changed

1 file changed

+58
-22
lines changed

triage/README.md

Lines changed: 58 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,13 @@ usage.
1010
- rylev
1111
- pnkfelix
1212

13-
Monday evening in North America is a good time to do it because This Week in Rust (see below) is usually
14-
published on Tuesday, US time, and so it means the PR to include the triage
13+
Tuesday afternoon in North America is a good time to do it because This Week in Rust (see below) is usually
14+
published on Wednesday, US time, and so it means the PR to include the triage
1515
details in TWiR can be merged shortly before publication. This time is also
1616
shortly before the weekly Rust compiler meeting, where the results are looked
1717
at.
1818

19-
## Instructions
19+
## Generating the Report
2020

2121
First, check the previous triage log entry. Look for responses in PRs, and
2222
follow up on any promised actions. (i.e. nag people!)
@@ -34,7 +34,9 @@ Use the provided script to automate building the file:
3434
% ./weekly-report.py PARENT > YYYY-MM-DD.md
3535
```
3636

37-
You can also do it manually, starting by viewing the [perf website](https://perf.rust-lang.org).
37+
### Manual Generation
38+
39+
You can also build the report manually, starting by viewing the [perf website](https://perf.rust-lang.org).
3840

3941
- Determine the revision range. The start revision corresponds to the end
4042
revision from the previous log entry. The end revision is the latest measured
@@ -45,31 +47,67 @@ You can also do it manually, starting by viewing the [perf website](https://perf
4547
see.
4648
- Record the revision range, with a link, in the log entry.
4749

48-
Look for significant changes (regressions or improvements) in the instruction
49-
count graphs.
50+
## Analysis
51+
52+
The following is a list of items you should look for when interpreting performance results.
53+
54+
If you've generated a triage report, you will go through each entry in the report
55+
and verify that it is properly labeled as a regression, improvement, or a mix
56+
of the two. For instance, some entries that are labeled as regressions, are actually
57+
not regressions and have only been labeled as such due to noise.
58+
59+
### Viewing Results
60+
61+
Look for significant changes (regressions or improvements) in the following:
62+
- instruction count
63+
- max rss
64+
65+
When working with graphs:
5066
- Click and drag a region of a graph to zoom in on it. This is useful when data
5167
points are close together.
52-
- Click on a data point to open the "compare" page for that merge.
68+
- Click on a data point to open the "compare" page for that merge. This opens the comparison pages that are linked to in the generated triage report.
69+
70+
To view the code changes:
5371
- Click on the "compare" link at the top of the measurements on that page to
5472
open the page of commits in the merge.
55-
- Alternatively, it may be easier to simply click through to the "compare" page
56-
for every revision in the range (there often aren't that many), rather than
57-
looking at the graphs.
73+
74+
Understanding the comparison page:
75+
- Each benchmark is listed with the min, max and the avg percentage change
76+
(red indicates regressions, green indicates improvements) across the various
77+
benchmarks run (e.g., full, incremental-full, incremental-unchanged, etc.).
78+
- Clicking on a specific benchmark will show the results for each of the various
79+
benchmarks. Clicking on the percentages will open a more specific detail view
80+
of timing for queries run during compilation.
81+
82+
### Interpreting Results
83+
84+
*Warning*: max rss is much more variable than instruction count. Recheck the "Absolute
85+
data" checkbox otherwise the noise becomes unmanageable.
86+
5887
- A change isn't significant unless one or more of the benchmarks changed by at
5988
least 1%.
6089

61-
Easy cases: there is only a single PR in the merge.
90+
#### Dealing with noise
91+
92+
Some benchmarks are noisy. Those that are known to be noisy typically have their results
93+
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.
94+
95+
## Action
96+
97+
### Ping PR Author/Reviewer
98+
99+
Single PR in Merge:
62100
- Add a comment to the PR pointing to the "compare" page (unless someone else
63101
has already done that).
64102
- In the case of a regression, ask the author for a response. If it's a big
65-
regression, consider requesting a backout. It may be worth looking through
66-
the comments to see if any perf CI runs were done, and whether the
67-
regression was expected.
68-
- Add an entry to the triage log. Include the PR title and number, a link to
69-
the PR comment you added mentioning the performance effect, and a link to the
70-
performance results. Include useful details, such as the size of the
71-
regression/improvement, and any promises of follow-up action from authors in
72-
the case of a regression.
103+
regression, consider requesting the author revert their changes. It may
104+
be worth looking through the comments to see if any perf CI runs were done,
105+
and whether the regression was expected.
106+
- If you did not generate the triage report, add an entry.
107+
Include the PR title and number, a link to the PR comment you added mentioning the
108+
performance effect, and a link to the performance results. Include useful details,
109+
such as the size of the regression/improvement, and any promises of follow-up action
110+
from authors in the case of a regression.
73111

74112
Difficult cases: the merge was a rollup of multiple PRs.
75113
- Look through the PRs and try to determine which was the cause. Often you
@@ -84,9 +122,7 @@ Difficult cases: the merge was a rollup of multiple PRs.
84122
that are likely to affect performance.
85123
- Add an entry to the triage log, as for the easy cases.
86124

87-
Repeat with the `max-rss` graphs. These measurements are much noisier than
88-
`instructions:u`, so only larger changes will be clear; recheck the "Absolute
89-
data" checkbox otherwise the noise becomes unmanageable.
125+
### This Week in Rust
90126

91127
Once finished, file a PR adding a link to the log entry in [This Week in
92128
Rust](https://github.com/emberian/this-week-in-rust/).

0 commit comments

Comments
 (0)