Skip to content

Commit 9ae5b94

Browse files
Mark-SimulacrumMark-Simulacrum
authored
Merge pull request #1422 from pnkfelix/triage-2022-08-24
Triage 2022 08 24
2 parents ebd2d1d + 2816960 commit 9ae5b94

File tree

1 file changed

+214
-0
lines changed

1 file changed

+214
-0
lines changed

‎triage/2022-08-24.md

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
# 2022-08-24 Triage Log
2+
3+
Overall some really impressive wins this week. Note in particular
4+
PR [#100209](https://github.com/rust-lang/rust/pull/100209), "Lazily
5+
decode SourceFile from metadata" (which improved 75 primary benchmark
6+
scenarios and 158 secondary scenarios) and
7+
PR [#98655](https://github.com/rust-lang/rust/pull/98655) "Don't derive
8+
`PartialEq::ne`", which improved 65 primary scenarios and 27 secondary
9+
scenarios). There were a few cases that pnkfelix explicitly decided not
10+
to mark as triaged; see report for more details there.
11+
Also pnkfelix wonders if there is a recent slight-upward trend on max-rss
12+
for the past week, see the [summary graph](https://perf.rust-lang.org/?start=&end=&kind=percentfromfirst&stat=max-rss)
13+
14+
Triage done by **@pnkfelix**.
15+
Revision range: [14a459bf..4a24f08b](https://perf.rust-lang.org/?start=14a459bf37bc19476d43e0045d078121c12d3fef&end=4a24f08ba43166cfee86d868b3fe8612aec6faca&absolute=false&stat=instructions%3Au)
16+
17+
**Summary**:
18+
19+
| (instructions:u) | mean | range | count |
20+
|:----------------:|:----:|:-----:|:-----:|
21+
| Regressions ❌ <br /> (primary) | 0.6% | [0.4%, 0.8%] | 27 |
22+
| Regressions ❌ <br /> (secondary) | 0.4% | [0.2%, 0.6%] | 9 |
23+
| Improvements ✅ <br /> (primary) | -1.7% | [-20.1%, -0.3%] | 91 |
24+
| Improvements ✅ <br /> (secondary) | -3.6% | [-18.7%, -0.3%] | 160 |
25+
| All ❌✅ (primary) | -1.2% | [-20.1%, 0.8%] | 118 |
26+
27+
28+
3 Regressions, 4 Improvements, 4 Mixed; 3 of them in rollups
29+
43 artifact comparisons made in total
30+
31+
#### Regressions
32+
33+
Rollup of 15 pull requests [#100677](https://github.com/rust-lang/rust/pull/100677) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=86c6ebee8fa0a5ad1e18e375113b06bd2849b634&end=9c20b2a8cc7588decb6de25ac6a7912dcef24d65&stat=instructions:u)
34+
35+
| (instructions:u) | mean | range | count |
36+
|:----------------:|:----:|:-----:|:-----:|
37+
| Regressions ❌ <br /> (primary) | 0.3% | [0.3%, 0.3%] | 2 |
38+
| Regressions ❌ <br /> (secondary) | 1.3% | [0.5%, 1.9%] | 4 |
39+
| Improvements ✅ <br /> (primary) | - | - | 0 |
40+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
41+
| All ❌✅ (primary) | 0.3% | [0.3%, 0.3%] | 2 |
42+
43+
* lqd hypothesized this was caused by PR #100652 "Remove deferred sized checks (make them eager)"
44+
* regressions for #100652 include most of the rollup regressions, all by similar amounts (only ucd was absent from the narrower view).
45+
* left a [comment](https://github.com/rust-lang/rust/pull/100652#issuecomment-1225798572) on PR #100652 and marked it as a regression; marked rollup as triaged.
46+
47+
rustc_metadata: dedupe strings to prevent multiple copies in rmeta/query cache blow file size [#98851](https://github.com/rust-lang/rust/pull/98851) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=0b79f758c9aa6646606662a6d623a0752286cd17&end=71ecf5d359bf750cc171e124779a46985633439d&stat=instructions:u)
48+
49+
| (instructions:u) | mean | range | count |
50+
|:----------------:|:----:|:-----:|:-----:|
51+
| Regressions ❌ <br /> (primary) | 0.6% | [0.3%, 1.5%] | 15 |
52+
| Regressions ❌ <br /> (secondary) | 1.1% | [0.3%, 1.6%] | 21 |
53+
| Improvements ✅ <br /> (primary) | - | - | 0 |
54+
| Improvements ✅ <br /> (secondary) | -0.4% | [-0.6%, -0.2%] | 2 |
55+
| All ❌✅ (primary) | 0.6% | [0.3%, 1.5%] | 15 |
56+
57+
* the performance of this PR was heavily evaluated as part of its development.
58+
* some regression to instruction counts is compensated for by the improvements file-size and to max-rss.
59+
* the follow-up PR [#100803](https://github.com/rust-lang/rust/issues/100803) is going to more than compensate for the regressions here.
60+
* marked as triaged.
61+
62+
63+
implied bounds: explicitly state which types are assumed to be wf [#100676](https://github.com/rust-lang/rust/pull/100676) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=d0ea1d767925d53b2230e2ba81197821514781f0&end=a9bb589cd678e034d194193fa892942315b10e2a&stat=instructions:u)
64+
65+
| (instructions:u) | mean | range | count |
66+
|:----------------:|:----:|:-----:|:-----:|
67+
| Regressions ❌ <br /> (primary) | 0.3% | [0.2%, 0.5%] | 22 |
68+
| Regressions ❌ <br /> (secondary) | 0.4% | [0.2%, 0.8%] | 22 |
69+
| Improvements ✅ <br /> (primary) | - | - | 0 |
70+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
71+
| All ❌✅ (primary) | 0.3% | [0.2%, 0.5%] | 22 |
72+
73+
* This PR was intended to be a refactor, but it turns out it has other problems (see [issue 100910](https://github.com/rust-lang/rust/issues/100910)).
74+
* The regressions alone are not cause to revert the PR, but the soundness bug pushes me over the line.
75+
* Nominated for discussion (of revert) in Thursday's T-compiler meeting. Not tagging as triaged.
76+
77+
#### Improvements
78+
79+
Don't derive `PartialEq::ne`. [#98655](https://github.com/rust-lang/rust/pull/98655) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=f241c0c43d71960f078b897e9b8721d4b452ce5e&end=361c599feeefaf6e50efd90658fc9c2222154684&stat=instructions:u)
80+
81+
| (instructions:u) | mean | range | count |
82+
|:----------------:|:----:|:-----:|:-----:|
83+
| Regressions ❌ <br /> (primary) | - | - | 0 |
84+
| Regressions ❌ <br /> (secondary) | 1.0% | [1.0%, 1.0%] | 1 |
85+
| Improvements ✅ <br /> (primary) | -0.7% | [-1.4%, -0.2%] | 65 |
86+
| Improvements ✅ <br /> (secondary) | -5.2% | [-10.0%, -0.3%] | 27 |
87+
| All ❌✅ (primary) | -0.7% | [-1.4%, -0.2%] | 65 |
88+
89+
* This had an interesting discussion thread on it; see [nnethercote's summary comment](https://github.com/rust-lang/rust/pull/98655#issuecomment-1176827089) for more info.
90+
91+
Lazily decode SourceFile from metadata [#100209](https://github.com/rust-lang/rust/pull/100209) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=6c943bad02626dddc5e5135b23c77429b6e4a063&end=468887ef91e46847dff57b6b234cff0fad17cb71&stat=instructions:u)
92+
93+
| (instructions:u) | mean | range | count |
94+
|:----------------:|:----:|:-----:|:-----:|
95+
| Regressions ❌ <br /> (primary) | 0.2% | [0.2%, 0.2%] | 2 |
96+
| Regressions ❌ <br /> (secondary) | 0.7% | [0.4%, 0.9%] | 3 |
97+
| Improvements ✅ <br /> (primary) | -1.6% | [-19.6%, -0.2%] | 75 |
98+
| Improvements ✅ <br /> (secondary) | -3.0% | [-18.3%, -0.2%] | 158 |
99+
| All ❌✅ (primary) | -1.6% | [-19.6%, 0.2%] | 77 |
100+
101+
* Don't get too excited y'all, that 19.6% improvement was to helloworld.
102+
* having said that, this does represent a huge win across a broad suite of benchmarks, nearly all in incremental.
103+
* (also, lqd notes that helloworld is a useful proxy for near-trivial build.rs scripts.)
104+
105+
Update minifier version to 0.2.2 [#100624](https://github.com/rust-lang/rust/pull/100624) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=f2858b5cd32f3689ad83de77cacfa1ea2f533793&end=aa8e761defc245d08d2cf226786def8a8bb56e53&stat=instructions:u)
106+
107+
| (instructions:u) | mean | range | count |
108+
|:----------------:|:----:|:-----:|:-----:|
109+
| Regressions ❌ <br /> (primary) | - | - | 0 |
110+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
111+
| Improvements ✅ <br /> (primary) | -0.6% | [-1.6%, -0.3%] | 13 |
112+
| Improvements ✅ <br /> (secondary) | -1.1% | [-1.5%, -0.3%] | 20 |
113+
| All ❌✅ (primary) | -0.6% | [-1.6%, -0.3%] | 13 |
114+
115+
* As noted by nnethercote, the cycles and max-rss results are neutral or under noise threshold, while instruction counts improved.
116+
117+
Kind-less SessionDiagnostic derive [#100765](https://github.com/rust-lang/rust/pull/100765) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=39a9b88f4e50d4c0204bb12c0821b49a302ab3c5&end=4b695f7c4e1a02d160fe7e159abd0f87027c0fcf&stat=instructions:u)
118+
119+
| (instructions:u) | mean | range | count |
120+
|:----------------:|:----:|:-----:|:-----:|
121+
| Regressions ❌ <br /> (primary) | - | - | 0 |
122+
| Regressions ❌ <br /> (secondary) | - | - | 0 |
123+
| Improvements ✅ <br /> (primary) | -0.8% | [-0.9%, -0.6%] | 5 |
124+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
125+
| All ❌✅ (primary) | -0.8% | [-0.9%, -0.6%] | 5 |
126+
127+
* all five improvements are to instances of regex-opt-incr-{patched,full} benchmark
128+
129+
#### Mixed
130+
131+
Rollup of 9 pull requests [#100810](https://github.com/rust-lang/rust/pull/100810) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=48853a361a5ff0e8215301c62f259a26eed7aa72&end=878aef79dcdf59d19bb8482202dc55e58ceb62ff&stat=instructions:u)
132+
133+
| (instructions:u) | mean | range | count |
134+
|:----------------:|:----:|:-----:|:-----:|
135+
| Regressions ❌ <br /> (primary) | - | - | 0 |
136+
| Regressions ❌ <br /> (secondary) | 0.3% | [0.2%, 0.3%] | 2 |
137+
| Improvements ✅ <br /> (primary) | -0.7% | [-0.9%, -0.3%] | 8 |
138+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
139+
| All ❌✅ (primary) | -0.7% | [-0.9%, -0.3%] | 8 |
140+
141+
* already triaged: "The small number of small improvements slightly outweighs the small number of small regressions. No further action is needed."
142+
143+
update Miri [#100841](https://github.com/rust-lang/rust/pull/100841) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=4b695f7c4e1a02d160fe7e159abd0f87027c0fcf&end=31302033095dc75608675cd6f9b884d1692054f0&stat=instructions:u)
144+
145+
| (instructions:u) | mean | range | count |
146+
|:----------------:|:----:|:-----:|:-----:|
147+
| Regressions ❌ <br /> (primary) | 0.8% | [0.5%, 1.0%] | 5 |
148+
| Regressions ❌ <br /> (secondary) | 0.2% | [0.2%, 0.2%] | 1 |
149+
| Improvements ✅ <br /> (primary) | -0.8% | [-0.8%, -0.8%] | 1 |
150+
| Improvements ✅ <br /> (secondary) | - | - | 0 |
151+
| All ❌✅ (primary) | 0.5% | [-0.8%, 1.0%] | 6 |
152+
153+
* regressions were in regex-opt-incr-patched (and ucd-doc-full, but that was just 0.24%)
154+
* while the regression here is unfortunate, there is not much we can expect to do in the short term to address it
155+
* its not even clear whether miri is really at fault; the [detailed query](https://perf.rust-lang.org/detailed-query.html?commit=31302033095dc75608675cd6f9b884d1692054f0&base_commit=4b695f7c4e1a02d160fe7e159abd0f87027c0fcf&benchmark=regex-1.5.5-opt&scenario=incr-patched:%20reverse) info says that the regression is due to `LLVM_lto_optimize`. Could the miri changes have somehow caused the codegen unit partitioning to change? Why would a miri update affect the time for `LLVM_lto_optimize`?
156+
* not marking as triaged. I'm not sure if anyone can justify spending time to look at this, but I don't want to just let it slide through just yet.
157+
158+
Rollup of 11 pull requests [#100847](https://github.com/rust-lang/rust/pull/100847) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=650bff80a623e17675ac72ae4d62ed200a4a3568&end=c0941dfb5a7d07ef2d70cc54d319669d9d6f6c01&stat=instructions:u)
159+
160+
| (instructions:u) | mean | range | count |
161+
|:----------------:|:----:|:-----:|:-----:|
162+
| Regressions ❌ <br /> (primary) | - | - | 0 |
163+
| Regressions ❌ <br /> (secondary) | 0.2% | [0.2%, 0.2%] | 1 |
164+
| Improvements ✅ <br /> (primary) | -0.5% | [-0.5%, -0.5%] | 1 |
165+
| Improvements ✅ <br /> (secondary) | -0.7% | [-1.1%, -0.3%] | 5 |
166+
| All ❌✅ (primary) | -0.5% | [-0.5%, -0.5%] | 1 |
167+
168+
* benefits here heavily outweigh the one minor regression.
169+
* already triaged by nnethercote
170+
171+
Use `AttrVec` more [#100668](https://github.com/rust-lang/rust/pull/100668) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=0b71ffca18a9f4a9515773b2c23d13f501d1e08f&end=3ce46b74aa3968b459cff3ce5c0d4f13e220b217&stat=instructions:u)
172+
173+
| (instructions:u) | mean | range | count |
174+
|:----------------:|:----:|:-----:|:-----:|
175+
| Regressions ❌ <br /> (primary) | 0.6% | [0.2%, 1.0%] | 2 |
176+
| Regressions ❌ <br /> (secondary) | 0.6% | [0.3%, 1.3%] | 8 |
177+
| Improvements ✅ <br /> (primary) | -0.3% | [-0.4%, -0.2%] | 7 |
178+
| Improvements ✅ <br /> (secondary) | -0.5% | [-1.2%, -0.2%] | 15 |
179+
| All ❌✅ (primary) | -0.1% | [-0.4%, 1.0%] | 9 |
180+
181+
* already triaged by nnethercote: "a few small wins and losses here, which balance each other out, and the net effect is perf-neutral."
182+
183+
#### Untriaged Pull Requests
184+
185+
- [#100841 update Miri](https://github.com/rust-lang/rust/pull/100841)
186+
- [#100677 Rollup of 15 pull requests](https://github.com/rust-lang/rust/pull/100677)
187+
- [#100676 implied bounds: explicitly state which types are assumed to be wf](https://github.com/rust-lang/rust/pull/100676)
188+
- [#100429 rustdoc: Merge source code pages HTML elements together](https://github.com/rust-lang/rust/pull/100429)
189+
- [#99792 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/99792)
190+
- [#99520 Rollup of 9 pull requests](https://github.com/rust-lang/rust/pull/99520)
191+
- [#99251 Upgrade indexmap and thorin-dwp to use hashbrown 0.12](https://github.com/rust-lang/rust/pull/99251)
192+
- [#99231 Rollup of 5 pull requests](https://github.com/rust-lang/rust/pull/99231)
193+
- [#99210 Rollup of 5 pull requests](https://github.com/rust-lang/rust/pull/99210)
194+
- [#99126 remove allow(rustc::potential_query_instability) in rustc_span](https://github.com/rust-lang/rust/pull/99126)
195+
- [#99123 proc_macro: use crossbeam channels for the proc_macro cross-thread bridge](https://github.com/rust-lang/rust/pull/99123)
196+
- [#99047 Rollup of 6 pull requests](https://github.com/rust-lang/rust/pull/99047)
197+
- [#99014 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/99014)
198+
- [#98987 Rollup of 7 pull requests](https://github.com/rust-lang/rust/pull/98987)
199+
- [#98957 don't allow ZST in ScalarInt ](https://github.com/rust-lang/rust/pull/98957)
200+
- [#98904 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/98904)
201+
- [#98874 Rollup of 5 pull requests](https://github.com/rust-lang/rust/pull/98874)
202+
- [#98851 rustc_metadata: dedupe strings to prevent multiple copies in rmeta/query cache blow file size](https://github.com/rust-lang/rust/pull/98851)
203+
- [#98612 Rollup of 9 pull requests](https://github.com/rust-lang/rust/pull/98612)
204+
- [#98591 Rollup of 9 pull requests](https://github.com/rust-lang/rust/pull/98591)
205+
- [#98178 btree: avoid forcing the allocator to be a reference](https://github.com/rust-lang/rust/pull/98178)
206+
- [#98145 Pull Derefer before ElaborateDrops](https://github.com/rust-lang/rust/pull/98145)
207+
- [#97786 Account for `-Z simulate-remapped-rust-src-base` when resolving remapped paths](https://github.com/rust-lang/rust/pull/97786)
208+
- [#97019 Transition to valtrees pt1](https://github.com/rust-lang/rust/pull/97019)
209+
- [#96883 Add EarlyBinder](https://github.com/rust-lang/rust/pull/96883)
210+
- [#96825 Retire `ItemLikeVisitor` trait](https://github.com/rust-lang/rust/pull/96825)
211+
- [#96010 Implement `core::ptr::Unique` on top of `NonNull`](https://github.com/rust-lang/rust/pull/96010)
212+
- [#95990 Rollup of 7 pull requests](https://github.com/rust-lang/rust/pull/95990)
213+
- [#95956 Support unstable moves via stable in unstable items](https://github.com/rust-lang/rust/pull/95956)
214+
- [#95899 rustc_metadata: Do not encode unnecessary module children](https://github.com/rust-lang/rust/pull/95899)

0 commit comments

Comments
 (0)