Skip to content

Triage 2022 08 24 #1422

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.

Sign up for GitHub

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
Aug 24, 2022
Merged

Triage 2022 08 24 #1422

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
702d4cb
added summary.
pnkfelix Aug 24, 2022
2816960
added line about max-rss.
pnkfelix Aug 24, 2022
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
214 changes: 214 additions & 0 deletions triage/2022-08-24.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
# 2022-08-24 Triage Log

Overall some really impressive wins this week. Note in particular
PR [#100209](https://github.com/rust-lang/rust/pull/100209), "Lazily
decode SourceFile from metadata" (which improved 75 primary benchmark
scenarios and 158 secondary scenarios) and
PR [#98655](https://github.com/rust-lang/rust/pull/98655) "Don't derive
`PartialEq::ne`", which improved 65 primary scenarios and 27 secondary
scenarios). There were a few cases that pnkfelix explicitly decided not
to mark as triaged; see report for more details there.
Also pnkfelix wonders if there is a recent slight-upward trend on max-rss
for the past week, see the [summary graph](https://perf.rust-lang.org/?start=&end=&kind=percentfromfirst&stat=max-rss)

Triage done by **@pnkfelix**.
Revision range: [14a459bf..4a24f08b](https://perf.rust-lang.org/?start=14a459bf37bc19476d43e0045d078121c12d3fef&end=4a24f08ba43166cfee86d868b3fe8612aec6faca&absolute=false&stat=instructions%3Au)

**Summary**:

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.6% | [0.4%, 0.8%] | 27 |
| Regressions ❌ <br /> (secondary) | 0.4% | [0.2%, 0.6%] | 9 |
| Improvements ✅ <br /> (primary) | -1.7% | [-20.1%, -0.3%] | 91 |
| Improvements ✅ <br /> (secondary) | -3.6% | [-18.7%, -0.3%] | 160 |
| All ❌✅ (primary) | -1.2% | [-20.1%, 0.8%] | 118 |


3 Regressions, 4 Improvements, 4 Mixed; 3 of them in rollups
43 artifact comparisons made in total

#### Regressions

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.3% | [0.3%, 0.3%] | 2 |
| Regressions ❌ <br /> (secondary) | 1.3% | [0.5%, 1.9%] | 4 |
| Improvements ✅ <br /> (primary) | - | - | 0 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | 0.3% | [0.3%, 0.3%] | 2 |

* lqd hypothesized this was caused by PR #100652 "Remove deferred sized checks (make them eager)"
* regressions for #100652 include most of the rollup regressions, all by similar amounts (only ucd was absent from the narrower view).
* 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.

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.6% | [0.3%, 1.5%] | 15 |
| Regressions ❌ <br /> (secondary) | 1.1% | [0.3%, 1.6%] | 21 |
| Improvements ✅ <br /> (primary) | - | - | 0 |
| Improvements ✅ <br /> (secondary) | -0.4% | [-0.6%, -0.2%] | 2 |
| All ❌✅ (primary) | 0.6% | [0.3%, 1.5%] | 15 |

* the performance of this PR was heavily evaluated as part of its development.
* some regression to instruction counts is compensated for by the improvements file-size and to max-rss.
* the follow-up PR [#100803](https://github.com/rust-lang/rust/issues/100803) is going to more than compensate for the regressions here.
* marked as triaged.


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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.3% | [0.2%, 0.5%] | 22 |
| Regressions ❌ <br /> (secondary) | 0.4% | [0.2%, 0.8%] | 22 |
| Improvements ✅ <br /> (primary) | - | - | 0 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | 0.3% | [0.2%, 0.5%] | 22 |

* 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)).
* The regressions alone are not cause to revert the PR, but the soundness bug pushes me over the line.
* Nominated for discussion (of revert) in Thursday's T-compiler meeting. Not tagging as triaged.

#### Improvements

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | 1.0% | [1.0%, 1.0%] | 1 |
| Improvements ✅ <br /> (primary) | -0.7% | [-1.4%, -0.2%] | 65 |
| Improvements ✅ <br /> (secondary) | -5.2% | [-10.0%, -0.3%] | 27 |
| All ❌✅ (primary) | -0.7% | [-1.4%, -0.2%] | 65 |

* 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.

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.2% | [0.2%, 0.2%] | 2 |
| Regressions ❌ <br /> (secondary) | 0.7% | [0.4%, 0.9%] | 3 |
| Improvements ✅ <br /> (primary) | -1.6% | [-19.6%, -0.2%] | 75 |
| Improvements ✅ <br /> (secondary) | -3.0% | [-18.3%, -0.2%] | 158 |
| All ❌✅ (primary) | -1.6% | [-19.6%, 0.2%] | 77 |

* Don't get too excited y'all, that 19.6% improvement was to helloworld.
* having said that, this does represent a huge win across a broad suite of benchmarks, nearly all in incremental.
* (also, lqd notes that helloworld is a useful proxy for near-trivial build.rs scripts.)

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | - | - | 0 |
| Improvements ✅ <br /> (primary) | -0.6% | [-1.6%, -0.3%] | 13 |
| Improvements ✅ <br /> (secondary) | -1.1% | [-1.5%, -0.3%] | 20 |
| All ❌✅ (primary) | -0.6% | [-1.6%, -0.3%] | 13 |

* As noted by nnethercote, the cycles and max-rss results are neutral or under noise threshold, while instruction counts improved.

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | - | - | 0 |
| Improvements ✅ <br /> (primary) | -0.8% | [-0.9%, -0.6%] | 5 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | -0.8% | [-0.9%, -0.6%] | 5 |

* all five improvements are to instances of regex-opt-incr-{patched,full} benchmark

#### Mixed

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | 0.3% | [0.2%, 0.3%] | 2 |
| Improvements ✅ <br /> (primary) | -0.7% | [-0.9%, -0.3%] | 8 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | -0.7% | [-0.9%, -0.3%] | 8 |

* already triaged: "The small number of small improvements slightly outweighs the small number of small regressions. No further action is needed."

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.8% | [0.5%, 1.0%] | 5 |
| Regressions ❌ <br /> (secondary) | 0.2% | [0.2%, 0.2%] | 1 |
| Improvements ✅ <br /> (primary) | -0.8% | [-0.8%, -0.8%] | 1 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | 0.5% | [-0.8%, 1.0%] | 6 |

* regressions were in regex-opt-incr-patched (and ucd-doc-full, but that was just 0.24%)
* while the regression here is unfortunate, there is not much we can expect to do in the short term to address it
* 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`?
* 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.

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | 0.2% | [0.2%, 0.2%] | 1 |
| Improvements ✅ <br /> (primary) | -0.5% | [-0.5%, -0.5%] | 1 |
| Improvements ✅ <br /> (secondary) | -0.7% | [-1.1%, -0.3%] | 5 |
| All ❌✅ (primary) | -0.5% | [-0.5%, -0.5%] | 1 |

* benefits here heavily outweigh the one minor regression.
* already triaged by nnethercote

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)

| (instructions:u) | mean | range | count |
|:----------------:|:----:|:-----:|:-----:|
| Regressions ❌ <br /> (primary) | 0.6% | [0.2%, 1.0%] | 2 |
| Regressions ❌ <br /> (secondary) | 0.6% | [0.3%, 1.3%] | 8 |
| Improvements ✅ <br /> (primary) | -0.3% | [-0.4%, -0.2%] | 7 |
| Improvements ✅ <br /> (secondary) | -0.5% | [-1.2%, -0.2%] | 15 |
| All ❌✅ (primary) | -0.1% | [-0.4%, 1.0%] | 9 |

* already triaged by nnethercote: "a few small wins and losses here, which balance each other out, and the net effect is perf-neutral."

#### Untriaged Pull Requests

- [#100841 update Miri](https://github.com/rust-lang/rust/pull/100841)
- [#100677 Rollup of 15 pull requests](https://github.com/rust-lang/rust/pull/100677)
- [#100676 implied bounds: explicitly state which types are assumed to be wf](https://github.com/rust-lang/rust/pull/100676)
- [#100429 rustdoc: Merge source code pages HTML elements together](https://github.com/rust-lang/rust/pull/100429)
- [#99792 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/99792)
- [#99520 Rollup of 9 pull requests](https://github.com/rust-lang/rust/pull/99520)
- [#99251 Upgrade indexmap and thorin-dwp to use hashbrown 0.12](https://github.com/rust-lang/rust/pull/99251)
- [#99231 Rollup of 5 pull requests](https://github.com/rust-lang/rust/pull/99231)
- [#99210 Rollup of 5 pull requests](https://github.com/rust-lang/rust/pull/99210)
- [#99126 remove allow(rustc::potential_query_instability) in rustc_span](https://github.com/rust-lang/rust/pull/99126)
- [#99123 proc_macro: use crossbeam channels for the proc_macro cross-thread bridge](https://github.com/rust-lang/rust/pull/99123)
- [#99047 Rollup of 6 pull requests](https://github.com/rust-lang/rust/pull/99047)
- [#99014 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/99014)
- [#98987 Rollup of 7 pull requests](https://github.com/rust-lang/rust/pull/98987)
- [#98957 don't allow ZST in ScalarInt ](https://github.com/rust-lang/rust/pull/98957)
- [#98904 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/98904)
- [#98874 Rollup of 5 pull requests](https://github.com/rust-lang/rust/pull/98874)
- [#98851 rustc_metadata: dedupe strings to prevent multiple copies in rmeta/query cache blow file size](https://github.com/rust-lang/rust/pull/98851)
- [#98612 Rollup of 9 pull requests](https://github.com/rust-lang/rust/pull/98612)
- [#98591 Rollup of 9 pull requests](https://github.com/rust-lang/rust/pull/98591)
- [#98178 btree: avoid forcing the allocator to be a reference](https://github.com/rust-lang/rust/pull/98178)
- [#98145 Pull Derefer before ElaborateDrops](https://github.com/rust-lang/rust/pull/98145)
- [#97786 Account for `-Z simulate-remapped-rust-src-base` when resolving remapped paths](https://github.com/rust-lang/rust/pull/97786)
- [#97019 Transition to valtrees pt1](https://github.com/rust-lang/rust/pull/97019)
- [#96883 Add EarlyBinder](https://github.com/rust-lang/rust/pull/96883)
- [#96825 Retire `ItemLikeVisitor` trait](https://github.com/rust-lang/rust/pull/96825)
- [#96010 Implement `core::ptr::Unique` on top of `NonNull`](https://github.com/rust-lang/rust/pull/96010)
- [#95990 Rollup of 7 pull requests](https://github.com/rust-lang/rust/pull/95990)
- [#95956 Support unstable moves via stable in unstable items](https://github.com/rust-lang/rust/pull/95956)
- [#95899 rustc_metadata: Do not encode unnecessary module children](https://github.com/rust-lang/rust/pull/95899)