Skip to content

Commit 4c15f46

Browse files
rylevrylev
committed
Add triage for 2022-04-26
1 parent 9efb478 commit 4c15f46

File tree

1 file changed

+132
-0
lines changed

1 file changed

+132
-0
lines changed

‎triage/2022-04-26.md

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
# 2022-04-26 Triage Log
2+
3+
This was, in general, a positive week for compiler performance. There were many concentrated efforts on improving rustdoc performance with a lot of real world crates showing ~4-7% improvements in full build times. Additionally, there was further improvement to `macro_rules!` performance with many real world crates improving performance by as much as 18% in full builds! On the other hand, the regressions were mostly minor and largely relegated to secondary benchmarks.
4+
5+
Triage done by **@rylev**.
6+
Revision range: [4ca19e09d302a4cbde14f9cb1bc109179dc824cd..1c988cfa0b7f4d3bc5b1cb40dc5002f5adbfb9ad](https://perf.rust-lang.org/?start=4ca19e09d302a4cbde14f9cb1bc109179dc824cd&end=1c988cfa0b7f4d3bc5b1cb40dc5002f5adbfb9ad&absolute=false&stat=instructions%3Au)
7+
8+
4 Regressions, 6 Improvements, 3 Mixed; 1 of them in rollups
9+
45 artifact comparisons made in total
10+
11+
#### Regressions
12+
13+
Rollup of 5 pull requests [#96263](https://github.com/rust-lang/rust/pull/96263) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=09ccb6c59d026b94edae50ba949b22dfc5d65ed1&end=7be1da0319eb5f381bc0aa8559367bb33dfe90a5&stat=instructions:u)
14+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
15+
|:---:|:---:|:---:|:---:|:---:|:---:|
16+
| count | 2 | 6 | 0 | 0 | 2 |
17+
| mean | 2.0% | 6.3% | N/A | N/A | 2.0% |
18+
| max | 3.2% | 6.9% | N/A | N/A | 3.2% |
19+
- The regression in primary benchmarks is dominated by a noisy `syn-1.0.89` (see [rustc-perf#1301](https://github.com/rust-lang/rustc-perf/issues/1301)). However, the regressions in the secondary benchmarks seem real and point towards [#96236](https://github.com/rust-lang/rust/pull/96236) as the possible cause.
20+
- I ran cachegrind diff on `wg-grammar check full` and got [these results](https://gist.github.com/rylev/8dad0b2dc24733908de28ce34a906692) which shows `<rustc_borrowck::region_infer::Trace as alloc::vec::spec_from_elem::SpecFromElem>::from_elem` being called a lot more often after this change.
21+
- The regressions are not *huge* but they are certainly significant and real. If something obvious stands out to those more familiar with this code, it might be worth poking around.
22+
23+
24+
rustdoc: Unindent doc fragments on `Attributes` construction [#96282](https://github.com/rust-lang/rust/pull/96282) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=0b3404b01b251401e6b45cb1c4df8f883dfab2d7&end=8b2393086f4c41007b5fb02ef0579ffa7046bff2&stat=instructions:u)
25+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
26+
|:---:|:---:|:---:|:---:|:---:|:---:|
27+
| count | 1 | 10 | 0 | 0 | 1 |
28+
| mean | 0.4% | 0.4% | N/A | N/A | 0.4% |
29+
| max | 0.4% | 0.5% | N/A | N/A | 0.4% |
30+
- An [issue](https://github.com/rust-lang/rust/issues/96309) was opened to investigate whether more work is being done that strictly necessary.
31+
- Since the regressions are relatively minor and only constrained to somewhat "artificial" crates (i.e., `hello-world` is the only primary crate impacted), we can [mark this as triaged](https://github.com/rust-lang/rust/pull/96282#issuecomment-1109535756).
32+
33+
34+
Generate synthetic object file to ensure all exported and used symbols participate in the linking [#95604](https://github.com/rust-lang/rust/pull/95604) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=7417110cefda899a685a77557ac2bd7d7ee07e54&end=18b53cefdf7456bf68937b08e377b7e622a115c2&stat=instructions:u)
35+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
36+
|:---:|:---:|:---:|:---:|:---:|:---:|
37+
| count | 3 | 13 | 0 | 0 | 3 |
38+
| mean | 0.4% | 0.4% | N/A | N/A | 0.4% |
39+
| max | 0.5% | 0.5% | N/A | N/A | 0.5% |
40+
- This seems to be regressing a bunch of full doc builds. Given this doesn't touch rustdoc, but does touch metadata encoding/decoding, I would assume that's where the perf hit is coming from.
41+
- I ran cachegrind diff on `helloworld doc full` and [got these results](https://gist.github.com/rylev/772e7dd5e7f133e48bf318a583e44845). It indeed looks like we're calling decoding functions on certain items (attributes and idents) more than previously (albeit with less decoding of spans).
42+
- [Left a comment to the author/reviewer](https://github.com/rust-lang/rust/pull/95604#issuecomment-1109652483) to get more clarification.
43+
44+
45+
Display function path in unsafety violations - E0133 [#96294](https://github.com/rust-lang/rust/pull/96294) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=055bf4ccd521c2c2185166c86951be7be145727c&end=ec8619dca239f57201a3ceb59e93149659c07b58&stat=instructions:u)
46+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
47+
|:---:|:---:|:---:|:---:|:---:|:---:|
48+
| count | 44 | 0 | 0 | 0 | 44 |
49+
| mean | 1.3% | N/A | N/A | N/A | 1.3% |
50+
| max | 2.9% | N/A | N/A | N/A | 2.9% |
51+
- Performance runs [were done](https://github.com/rust-lang/rust/pull/96294#issuecomment-1106397651) on this change before merging and they did not show perf regressions
52+
- Added a [comment](https://github.com/rust-lang/rust/pull/96294#issuecomment-1109540254) to ensure the issue doesn't get lost. It seems it might be caused by inclusion of `DefId`s when they aren't needed.
53+
54+
55+
#### Improvements
56+
57+
Inline `ty::Const::ty()` and `ty::Const::val()` getters [#96022](https://github.com/rust-lang/rust/pull/96022) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=27af5175497936ea3413bef5816e7c0172514b9c&end=0034bbca260bfa00798d70150970924221688ede&stat=instructions:u)
58+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
59+
|:---:|:---:|:---:|:---:|:---:|:---:|
60+
| count | 0 | 0 | 7 | 6 | 7 |
61+
| mean | N/A | N/A | -1.9% | -0.7% | -1.9% |
62+
| max | N/A | N/A | -3.0% | -0.9% | -3.0% |
63+
64+
65+
Speed up `TokenCursor` [#96210](https://github.com/rust-lang/rust/pull/96210) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=1dec35a1b0df406da5d7cae55a7fa8d186a2b028&end=b04c5329e1e145fb2fb46c5a7e775638712b03aa&stat=instructions:u)
66+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
67+
|:---:|:---:|:---:|:---:|:---:|:---:|
68+
| count | 0 | 0 | 94 | 83 | 94 |
69+
| mean | N/A | N/A | -0.9% | -2.4% | -0.9% |
70+
| max | N/A | N/A | -2.4% | -17.2% | -2.4% |
71+
72+
73+
rustdoc: Optimize IdMap [#96260](https://github.com/rust-lang/rust/pull/96260) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=b04c5329e1e145fb2fb46c5a7e775638712b03aa&end=de1bc0008be096cf7ed67b93402250d3b3e480d0&stat=instructions:u)
74+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
75+
|:---:|:---:|:---:|:---:|:---:|:---:|
76+
| count | 1 | 0 | 11 | 15 | 12 |
77+
| mean | 1.0% | N/A | -0.7% | -0.6% | -0.6% |
78+
| max | 1.0% | N/A | -3.1% | -2.3% | -3.1% |
79+
80+
81+
rustdoc: Resolve some more doc links early [#96261](https://github.com/rust-lang/rust/pull/96261) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=10baaa6ed243c3e026491b1068c44356eaf24069&end=0b3404b01b251401e6b45cb1c4df8f883dfab2d7&stat=instructions:u)
82+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
83+
|:---:|:---:|:---:|:---:|:---:|:---:|
84+
| count | 0 | 0 | 14 | 22 | 14 |
85+
| mean | N/A | N/A | -2.2% | -3.4% | -2.2% |
86+
| max | N/A | N/A | -4.9% | -5.4% | -4.9% |
87+
88+
89+
rustdoc: make primitive synthetic impls for correct doc module [#96301](https://github.com/rust-lang/rust/pull/96301) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=41ef7678061dde625bf273ab6b036aebd7153a43&end=5ffebc2cb3a089c27a4c7da13d09fd2365c288aa&stat=instructions:u)
90+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
91+
|:---:|:---:|:---:|:---:|:---:|:---:|
92+
| count | 0 | 0 | 16 | 21 | 16 |
93+
| mean | N/A | N/A | -1.2% | -2.1% | -1.2% |
94+
| max | N/A | N/A | -3.0% | -3.3% | -3.0% |
95+
96+
incr. comp.: Don't export impl_stable_hash_via_hash!() and warn about using it. [#96082](https://github.com/rust-lang/rust/pull/96082) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=a2df8baea6fb7199822d39cfcfddb197604aa8a2&end=27af5175497936ea3413bef5816e7c0172514b9c&stat=instructions:u)
97+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
98+
|:---:|:---:|:---:|:---:|:---:|:---:|
99+
| count | 1 | 0 | 23 | 9 | 24 |
100+
| mean | 3.1% | N/A | -0.4% | -0.5% | -0.2% |
101+
| max | 3.1% | N/A | -0.6% | -0.7% | 3.1% |
102+
- The regression is `syn-1.0.89` which has become noisy (see [here](https://github.com/rust-lang/rustc-perf/issues/1301) for more details)
103+
104+
#### Mixed
105+
106+
rustdoc: Optimize and refactor doc link resolution [#96135](https://github.com/rust-lang/rust/pull/96135) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=0034bbca260bfa00798d70150970924221688ede&end=d39864d64e6e0762d418f6beeedb4510942fc828&stat=instructions:u)
107+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
108+
|:---:|:---:|:---:|:---:|:---:|:---:|
109+
| count | 6 | 0 | 14 | 20 | 20 |
110+
| mean | 0.3% | N/A | -1.3% | -0.7% | -0.9% |
111+
| max | 0.3% | N/A | -2.7% | -1.0% | -2.7% |
112+
- The regression in [stm32f4 is expected](https://github.com/rust-lang/rust/pull/96135#issuecomment-1101397244) and given this is otherwise a big perf win, we'll take the slight perf hit in one benchmark.
113+
114+
115+
Remove visibility information from HIR [#93970](https://github.com/rust-lang/rust/pull/93970) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=de1026a67b0a3f5d6b61a1f77093af97d4799376&end=143eaa8d441641251ab41ed73deaba0d8d0cf4a5&stat=instructions:u)
116+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
117+
|:---:|:---:|:---:|:---:|:---:|:---:|
118+
| count | 0 | 16 | 2 | 2 | 2 |
119+
| mean | N/A | 1.3% | -0.2% | -0.3% | -0.2% |
120+
| max | N/A | 2.2% | -0.2% | -0.3% | -0.2% |
121+
- The regression happens in a stress test that is expected to stress the code being changed.
122+
- After this change visibility is now being correctly hashed which is strictly more work but is the "correct" thing to do. Given this [the regressions are worth it](https://github.com/rust-lang/rust/pull/93970#issuecomment-1107778525).
123+
124+
125+
Make derefer work everwhere [#96116](https://github.com/rust-lang/rust/pull/96116) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=18b53cefdf7456bf68937b08e377b7e622a115c2&end=055bf4ccd521c2c2185166c86951be7be145727c&stat=instructions:u)
126+
| | Regressions 😿 <br />(primary) | Regressions 😿 <br />(secondary) | Improvements 🎉 <br />(primary) | Improvements 🎉 <br />(secondary) | All 😿 🎉 <br />(primary) |
127+
|:---:|:---:|:---:|:---:|:---:|:---:|
128+
| count | 8 | 16 | 1 | 0 | 9 |
129+
| mean | 0.5% | 1.2% | -1.7% | N/A | 0.2% |
130+
| max | 0.5% | 2.5% | -1.7% | N/A | -1.7% |
131+
- Unfortunate regressions but they are fairly isolated, small, and expected.
132+
- Majority of the regressions come from the introduction of more local variables which LLVM has to work through. This looks like an area we'll want to work through, but we [shouldn't block this PR on addressing this](https://github.com/rust-lang/rust/pull/96116#issuecomment-1109521059).

0 commit comments

Comments
 (0)