Skip to content

Add triage for 2022-06-07 #1342

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.

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 1 commit into from
Jun 14, 2022
Merged
Changes from all commits
Commits
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
185 changes: 185 additions & 0 deletions triage/2022-06-07.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
# 2022-06-07 Triage Log

A busy week in compiler performance, but fortunately improvements outweighed regressions. The biggest improvements came from @nnethercote's work on making the decoding of `SourceFile::lines` lazy which significantly cuts the costs of decoding crate metadata. The biggest regressions came from the removal of json handling in `rustc_serialize` which has been a multi-month effort to improve the maintainability of json (de-)serialization in the compiler.

Triage done by **@rylev**.
Revision range: [0a43923a..bb55bd](https://perf.rust-lang.org/?start=0a43923a86c3b8f11d005884871b152f59b746f7&end=bb55bd449e65e611da928560d948982d73e50027&absolute=false&stat=instructions%3Au)

**Summary**:

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | 0.5% | 3.2% | 36 |
| Regressions 😿 <br /> (secondary) | 0.3% | 0.9% | 15 |
| Improvements 🎉 <br /> (primary) | -1.3% | -15.1% | 124 |
| Improvements 🎉 <br /> (secondary) | -2.7% | -13.5% | 182 |
| All 😿🎉 (primary) | -0.9% | -15.1% | 160 |


2 Regression, 6 Improvements, 5 Mixed; 4 of them in rollups
48 artifact comparisons made in total

#### Regressions

rewrite error handling for unresolved inference vars [#89862](https://github.com/rust-lang/rust/pull/89862) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=72f7e3144a386c820c188350092d2d93a74889b8&end=e40d5e83dc133d093c22c7ff016b10daa4f40dcf&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | 0.2% | 0.3% | 7 |
| Regressions 😿 <br /> (secondary) | 0.4% | 1.0% | 23 |
| Improvements 🎉 <br /> (primary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (secondary) | N/A | N/A | 0 |
| All 😿🎉 (primary) | 0.2% | 0.3% | 7 |
- Ran cache grind diff and saw that `ObligationForest::process_obligations` is getting called a lot more.
- I'm very unfamiliar with trait resolution, so I'm unsure if this is a red herring or not.
- In any case, [here](https://gist.github.com/rylev/8e5fd44e2ee0db7f3dd449eb0e88ccbd) is the full diff.
- Left a comment as such [here](https://github.com/rust-lang/rust/pull/89862#issuecomment-1148733903)


Remove all json handling from rustc_serialize [#85993](https://github.com/rust-lang/rust/pull/85993) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=9a74608543d499bcc7dd505e195e8bfab9447315&end=7e9b92cb43a489b34e2bcb8d21f36198e02eedbc&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | 0.5% | 1.3% | 68 |
| Regressions 😿 <br /> (secondary) | 0.9% | 3.5% | 40 |
| Improvements 🎉 <br /> (primary) | -0.4% | -0.6% | 3 |
| Improvements 🎉 <br /> (secondary) | -0.5% | -1.0% | 24 |
| All 😿🎉 (primary) | 0.5% | 1.3% | 71 |
- @nnethercote was not able to reproduce the regressions in a local build, and it seems the consensus is that the regressions are worth the hit.


#### Improvements

Make params be SmallVec as originally was [#97670](https://github.com/rust-lang/rust/pull/97670) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=a6b8c6954829669a5c4fa320c3e6132edf04fcfc&end=f15370b4e44988ad5d228b25e939650c0a6c2ec7&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (primary) | -0.2% | -0.2% | 3 |
| Improvements 🎉 <br /> (secondary) | N/A | N/A | 0 |
| All 😿🎉 (primary) | -0.2% | -0.2% | 3 |


Add PID to LLVM PGO profile path [#97137](https://github.com/rust-lang/rust/pull/97137) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=c7b0452ece11bf714f7cf2003747231931504d59&end=63641795406e1831a822f011242fdfb225fc8fbc&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (primary) | -0.6% | -0.8% | 20 |
| Improvements 🎉 <br /> (secondary) | -0.7% | -1.0% | 8 |
| All 😿🎉 (primary) | -0.6% | -0.8% | 20 |


Rollup of 6 pull requests [#97742](https://github.com/rust-lang/rust/pull/97742) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=4e725bad73747a4c93a3ac53106e4b4006edc665&end=43874a2ee749c2dd9f052172341f2f87fa36cd79&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (primary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (secondary) | -1.9% | -2.6% | 12 |
| All 😿🎉 (primary) | N/A | N/A | 0 |


interpret: better control over whether we read data with provenance [#97684](https://github.com/rust-lang/rust/pull/97684) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=79b6bad406155cad4481150fc5dfa0da5394e3b6&end=9d20fd109809f20c049d6895a5be27a1fbd39daa&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (primary) | -0.7% | -1.9% | 8 |
| Improvements 🎉 <br /> (secondary) | -5.5% | -10.5% | 12 |
| All 😿🎉 (primary) | -0.7% | -1.9% | 8 |


Remove migrate borrowck mode [#95565](https://github.com/rust-lang/rust/pull/95565) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=9f7e997c8bc3cacd2ab4eb75e63cb5fa9279c7b0&end=bb55bd449e65e611da928560d948982d73e50027&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (primary) | -0.5% | -1.9% | 47 |
| Improvements 🎉 <br /> (secondary) | -0.5% | -1.4% | 21 |
| All 😿🎉 (primary) | -0.5% | -1.9% | 47 |


Lazify `SourceFile::lines`. [#97575](https://github.com/rust-lang/rust/pull/97575) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=44e9516c8535c6f41f2d1c444657cd672b4ab81b&end=e71440575c930dcecac288b7c3536410d688b351&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | 0.4% | 0.5% | 6 |
| Improvements 🎉 <br /> (primary) | -1.8% | -15.3% | 52 |
| Improvements 🎉 <br /> (secondary) | -2.9% | -13.8% | 124 |
| All 😿🎉 (primary) | -1.8% | -15.3% | 52 |


#### Mixed

Add `#[inline]` to `Vec`'s `Deref/DerefMut` [#97553](https://github.com/rust-lang/rust/pull/97553) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=02916c4c75912f70b651c0b20b501444ce2ca231&end=395a09c3dafe0c7838c9ca41d2b47bb5e79a5b6d&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | 0.5% | 0.8% | 6 |
| Regressions 😿 <br /> (secondary) | 0.4% | 0.7% | 31 |
| Improvements 🎉 <br /> (primary) | -1.2% | -1.7% | 10 |
| Improvements 🎉 <br /> (secondary) | -1.1% | -1.9% | 10 |
| All 😿🎉 (primary) | -0.5% | -1.7% | 16 |
- As with any chance to inlining, performance is expected to change and to not always have a positive impact.
- The improvements outweigh the regressions (especially in primary benchmarks), and so it doesn't seem worth it to dig too much more into this.


Rollup of 6 pull requests [#97644](https://github.com/rust-lang/rust/pull/97644) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=5e6bb83268518dcd74c96b5504f485b71e604e4c&end=9598b4b594c97dff66feb93522e22db500deea07&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | 0.6% | 0.7% | 5 |
| Improvements 🎉 <br /> (primary) | -0.2% | -0.3% | 2 |
| Improvements 🎉 <br /> (secondary) | -0.3% | -0.5% | 11 |
| All 😿🎉 (primary) | -0.2% | -0.3% | 2 |
- The improvements outweigh the regressions (which are pretty small and contained in secondary benchmarks).
- Given it's in a rollup, it's not worth the effort to investigate.


Rollup of 5 pull requests [#97654](https://github.com/rust-lang/rust/pull/97654) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=20976bae5c426c738262db376eadbd8859aafc08&end=44e9516c8535c6f41f2d1c444657cd672b4ab81b&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | N/A | N/A | 0 |
| Regressions 😿 <br /> (secondary) | 0.6% | 0.6% | 1 |
| Improvements 🎉 <br /> (primary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (secondary) | -0.6% | -0.8% | 8 |
| All 😿🎉 (primary) | N/A | N/A | 0 |
- The improvements outweigh the regressions (which are pretty small and contained in secondary benchmarks).
- Given it's in a rollup, it's not worth the effort to investigate.


Rollup of 3 pull requests [#97694](https://github.com/rust-lang/rust/pull/97694) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=e40d5e83dc133d093c22c7ff016b10daa4f40dcf&end=9a74608543d499bcc7dd505e195e8bfab9447315&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | 0.2% | 0.2% | 1 |
| Regressions 😿 <br /> (secondary) | 0.3% | 0.5% | 5 |
| Improvements 🎉 <br /> (primary) | -0.4% | -0.6% | 12 |
| Improvements 🎉 <br /> (secondary) | -0.3% | -0.5% | 13 |
| All 😿🎉 (primary) | -0.4% | -0.6% | 13 |
- The improvements outweigh the regressions (which are pretty small and almost completely contained in secondary benchmarks).
- Given it's in a rollup, it's not worth the effort to investigate.


Inline `bridge::Buffer` methods. [#97604](https://github.com/rust-lang/rust/pull/97604) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=c3384ea35cafc3a8a6554a2ad524dbf70df4bbcd&end=cb0584f86b8cfa952dffad55f7d83bd90765120f&stat=instructions:u)

| | mean | max | count |
|:----------:|:----:|:---:|:-----:|
| Regressions 😿 <br /> (primary) | 3.8% | 3.8% | 1 |
| Regressions 😿 <br /> (secondary) | N/A | N/A | 0 |
| Improvements 🎉 <br /> (primary) | -0.3% | -0.3% | 1 |
| Improvements 🎉 <br /> (secondary) | -2.1% | -2.1% | 3 |
| All 😿🎉 (primary) | 1.8% | 3.8% | 2 |
- I'm a bit confused as this PR was opened to address a performance regression, but it seems to be itself a partial perf regression (at least in instruction counts).
- This causes a near 4% perf regression in [serde_derive-1.0.136](https://github.com/rust-lang/rustc-perf/tree/master/collector/benchmarks/serde_derive-1.0.136). Granted that particular benchmark has been somewhat noisy but not nearly to the level seen here.
- Add a comment seeking [justification](https://github.com/rust-lang/rust/pull/97604#issuecomment-1148722741)