Skip to content

Triage 2024 03 11 #1848

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 4 commits into from
Mar 14, 2024
Merged

Triage 2024 03 11 #1848

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3605985
Initial triage report template.
pnkfelix Mar 11, 2024
2b8ae9c
Triage report template with shortened commit ids
pnkfelix Mar 11, 2024
127cc12
Filled in entries for triage report, apart from summary.
pnkfelix Mar 13, 2024
69fb50d
Add summary for this week's triage.
pnkfelix Mar 13, 2024
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
246 changes: 246 additions & 0 deletions triage/2024-03-11.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,246 @@
# 2024-03-11 Triage Log

A mixed week, with a vast number of improvements (in large part due to PR
#122010, which undoes a prior regression; PR #120985, a host LLVM update).
But also three admittedly small-ish regressions which seemed unanticipated and
were still large enough that I did not feel comfortable rubber-stamping them
with a perf-regression-triaged marking.

Triage done by **@pnkfelix**.
Revision range: [41d97c8a..e919669d](https://perf.rust-lang.org/?start=41d97c8a5dea2731b0e56fe97cd7cb79e21cff79&end=e919669d42dfb8950866d4cb268c5359eb3f7c54&absolute=false&stat=instructions%3Au)

**Summary**:

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | 0.6% | [0.2%, 1.4%] | 38 |
| Regressions ❌ <br /> (secondary) | 1.1% | [0.2%, 4.9%] | 50 |
| Improvements ✅ <br /> (primary) | -1.0% | [-4.8%, -0.2%] | 119 |
| Improvements ✅ <br /> (secondary) | -0.8% | [-2.2%, -0.4%] | 36 |
| All ❌✅ (primary) | -0.6% | [-4.8%, 1.4%] | 157 |


2 Regressions, 5 Improvements, 9 Mixed; 5 of them in rollups
54 artifact comparisons made in total

#### Regressions

interpret: avoid a long-lived PlaceTy in stack frames [#121985](https://github.com/rust-lang/rust/pull/121985) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=8c9a75b3238b66592779d6b240dbf78eacefebb8&end=52f8aec14c616387c5f793687f2d9026de6c78ca&stat=instructions:u)

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

* primary regression was to html5ever doc-full; was anctipated during development and is presumed spurious.
* marking as triaged.

Detect unused struct impls pub trait [#121752](https://github.com/rust-lang/rust/pull/121752) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=cd81f5b27ee00b49d413db50b5e6af871cebcf23&end=c69fda7dc664e62f8920a02a4e55d6207b212c24&stat=instructions:u)

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

* primary regressions are all to serde and cranelift codegen for various profiles of incr-patched:println.
* the cycles measurement didn't observe any change at all, but that could be due to the difference being swamped by overall variance
* the [perf diff](https://perf.rust-lang.org/detailed-query.html?commit=c69fda7dc664e62f8920a02a4e55d6207b212c24&benchmark=serde-1.0.136-check&scenario=incr-patched%3A+println&base_commit=cd81f5b27ee00b49d413db50b5e6af871cebcf23&sort_idx=-11) highlights that the query `live_symbols_and_ignored_derived_traits` is the source of the perf regression, which is consistent with the idea that this lint has become more expensive since that's where we see the call to the newly-added `solve_rest_impl_items` (a worklist algorithm from the PR).
* leaving a note for the author about this; not marking as triaged.

#### Improvements

Rollup of 7 pull requests [#122111](https://github.com/rust-lang/rust/pull/122111) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=bfe762e0ed2e95041cc12c02c5565c4368f2cc9f&end=7d3702e472b99be0f5de6608dd87af1df8f99428&stat=instructions:u)

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

* all 7 primary improvements are to html5ever.
* all 5 secondary improvements are to tt-muncher.

Rollup of 8 pull requests [#122117](https://github.com/rust-lang/rust/pull/122117) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=7d3702e472b99be0f5de6608dd87af1df8f99428&end=d03b986db1f4146b58078c9dde5b0fa6d808f031&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | - | - | 0 |
| Improvements ✅ <br /> (primary) | -2.1% | [-3.9%, -0.4%] | 12 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | -2.1% | [-3.9%, -0.4%] | 12 |

* all 12 primary improvements are to diesel
* this is because of PR #122107, which made the `non_local_definitions` lint allow-by-default
* this effectively had the reverse effect of PR #120393 (which added the aforementioned lint and caused regressions to 12 variations of diesel).

Merge `collect_mod_item_types` query into `check_well_formed` [#121500](https://github.com/rust-lang/rust/pull/121500) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=42825768b103c28b10ce0407749acb21d32abeec&end=74acabe9b042ea8c42862ee29aca2a8b7d333644&stat=instructions:u)

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

* 3 significant primary improvements are to libc incr-patched:clone, and 1 less significant to bitmaps check incr-unchanged.

Avoid invoking the `intrinsic` query for DefKinds other than `Fn` or `AssocFn` [#122010](https://github.com/rust-lang/rust/pull/122010) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=01d73d4041969cde4a79bf9793521ef323248a24&end=4d4bb491b65c300835442f6cb4f34fc9a5685c26&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | - | - | 0 |
| Improvements ✅ <br /> (primary) | -0.5% | [-1.0%, -0.2%] | 74 |
| Improvements ✅ <br /> (secondary) | -0.7% | [-2.1%, -0.2%] | 26 |
| All ❌✅ (primary) | -0.5% | [-1.0%, -0.2%] | 74 |

* undoes the vast bulk of the broad perf regression injected by PR #120675

Dep node encoding cleanups [#122064](https://github.com/rust-lang/rust/pull/122064) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=5bc7b9ac8ace5312e1d2cdc2722715cf58d4f926&end=094a6204f590e6b4770b5f26359dd17a07897adf&stat=instructions:u)

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


#### Mixed

Optimize write with as_const_str for shorter code [#122059](https://github.com/rust-lang/rust/pull/122059) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=79d246112dc95bbd67848f7546f3fd1aca516b82&end=9fb91aa2e70bfcc1c0adaad79711f0321ea81ece&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | - | - | 0 |
| Regressions ❌ <br /> (secondary) | 1.1% | [0.3%, 1.9%] | 2 |
| Improvements ✅ <br /> (primary) | -0.8% | [-1.2%, -0.4%] | 2 |
| Improvements ✅ <br /> (secondary) | -0.3% | [-0.3%, -0.3%] | 1 |
| All ❌✅ (primary) | -0.8% | [-1.2%, -0.4%] | 2 |

* (secondary) regressions are to deep-vector debug-full (which may be spurious based on the graph) and wg-grammar debug-incr-unchanged
* since @nnethercote was already involved in related efforts here (e.g. PR #121001) and this resulting refinement, I'm going to mark this as triaged.

Replace the default branch with an unreachable branch If it is the last variant [#120268](https://github.com/rust-lang/rust/pull/120268) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=9fb91aa2e70bfcc1c0adaad79711f0321ea81ece&end=14fbc3c00525b41a3a3ee2c90e9ab6fd3b05274f&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | 0.7% | [0.2%, 1.8%] | 4 |
| Regressions ❌ <br /> (secondary) | 0.2% | [0.2%, 0.3%] | 7 |
| Improvements ✅ <br /> (primary) | -0.8% | [-1.2%, -0.3%] | 5 |
| Improvements ✅ <br /> (secondary) | -0.9% | [-2.2%, -0.3%] | 3 |
| All ❌✅ (primary) | -0.1% | [-1.2%, 1.8%] | 9 |

* primary regressions are to cargo opt-full (1.76%), cargo debug-incr-patched:println (0.40%), libc doc-full (0.50%), hyper doc-full (0.19%).
* the regression to cargo opt-full [was anticipated](https://github.com/rust-lang/rust/pull/120268#issuecomment-1943896894) by rust-timer runs during development; but other follow-up rust-timer runs did not always include the same regression.
* the PR itself notes that there are known performance problems in LLVM with unreachable branches (see e.g. [llvm#78578](https://github.com/llvm/llvm-project/issues/78578)). It is not clear to me whether the above regressions are related to that.
* posted comment [asking](https://github.com/rust-lang/rust/pull/120268#issuecomment-1992182505) PR author for more info. Not marking as triaged.

Rollup of 8 pull requests [#122182](https://github.com/rust-lang/rust/pull/122182) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=14fbc3c00525b41a3a3ee2c90e9ab6fd3b05274f&end=1b2c53a15dba7962cfc284c3b6d61a0341ffa27a&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | 1.3% | [1.3%, 1.3%] | 1 |
| Regressions ❌ <br /> (secondary) | 0.8% | [0.2%, 1.3%] | 2 |
| Improvements ✅ <br /> (primary) | -0.3% | [-0.7%, -0.2%] | 17 |
| Improvements ✅ <br /> (secondary) | -0.5% | [-0.9%, -0.2%] | 15 |
| All ❌✅ (primary) | -0.3% | [-0.7%, 1.3%] | 18 |

* sole primary regression is to image opt-full
* improvements obviously outweigh the regressions
* ... but oh, there is also a bootstrap regression that may be of concern: "Bootstrap: 648.483s -> 652.903s (0.68%)"
* kobzol has hypothesized that PR #122099 may be the cause of the bootstrap regression.
* after some discussion on the rollup PR, decided to mark as triaged; the author
may well choose to do some followup, but we will not hound them about it. :)

Replace `TypeWalker` usage with `TypeVisitor` in `wf.rs` [#122150](https://github.com/rust-lang/rust/pull/122150) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=9d272a1b0501f96da0ed10caa1b2eb6dbb653686&end=b054da815501bafb24a08284151d32862f7a3a13&stat=instructions:u)

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


* six (secondary) regressions (to variants of unify-linearly and regression-31157) were anticipated during development
* we were also expecting a broader set of 34 primary improvements. But the mean primary improvement we expected was -0.3%, which was unchanged.
* marking as triaged

Rollup of 12 pull requests [#122241](https://github.com/rust-lang/rust/pull/122241) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=b054da815501bafb24a08284151d32862f7a3a13&end=8401645716b26a8b4c6974dc0680e55e81e9e8a1&stat=instructions:u)

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

* sole regressions are to (secondary) regression-31157 debug-incr-full (1.56%), which might be spurious based on the graph, and tt-muncher opt-incr-unchanged (0.19%), which I don't consider worth getting worried about.
* marking as triaged.

Update host LLVM on x64 Linux to LLVM 18 [#120985](https://github.com/rust-lang/rust/pull/120985) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=8401645716b26a8b4c6974dc0680e55e81e9e8a1&end=25ee3c6a2f429a97ff4ad239e3f42409cd71fe0a&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | 0.3% | [0.1%, 1.0%] | 88 |
| Regressions ❌ <br /> (secondary) | 0.4% | [0.1%, 0.5%] | 21 |
| Improvements ✅ <br /> (primary) | -1.0% | [-1.6%, -0.4%] | 54 |
| Improvements ✅ <br /> (secondary) | -0.8% | [-1.2%, -0.6%] | 8 |
| All ❌✅ (primary) | -0.2% | [-1.6%, 1.0%] | 142 |

* These performance changes were anticipated, and the improvements outweigh the regressions.
* Marking as triaged.

Rollup of 8 pull requests [#122256](https://github.com/rust-lang/rust/pull/122256) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=25ee3c6a2f429a97ff4ad239e3f42409cd71fe0a&end=2d24fe591f30386d6d5fc2bb941c78d7266bf10f&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | 0.6% | [0.2%, 1.6%] | 21 |
| Regressions ❌ <br /> (secondary) | 0.6% | [0.3%, 1.6%] | 7 |
| Improvements ✅ <br /> (primary) | -0.7% | [-1.0%, -0.5%] | 3 |
| Improvements ✅ <br /> (secondary) | - | - | 0 |
| All ❌✅ (primary) | 0.4% | [-1.0%, 1.6%] | 24 |

* Nadrieril has isolated this to the rolled up PR #120504 and has reported it on that PR.
* (already marked as triaged)
* It might be addressed by follow-up PR #122396 (which is undergoing evaluation now).

Distinguish between library and lang UB in assert_unsafe_precondition [#121662](https://github.com/rust-lang/rust/pull/121662) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=2d24fe591f30386d6d5fc2bb941c78d7266bf10f&end=768408af123a455fb27ad8af8055becd5b95d36f&stat=instructions:u)

| (instructions:u) | mean | range | count |
|:----------------------------------:|:-----:|:--------------:|:-----:|
| Regressions ❌ <br /> (primary) | 1.1% | [0.3%, 1.7%] | 4 |
| Regressions ❌ <br /> (secondary) | 1.1% | [1.1%, 1.1%] | 1 |
| Improvements ✅ <br /> (primary) | -0.9% | [-1.7%, -0.4%] | 4 |
| Improvements ✅ <br /> (secondary) | -0.2% | [-0.2%, -0.2%] | 1 |
| All ❌✅ (primary) | 0.1% | [-1.7%, 1.7%] | 8 |

* primary regressions are to cranelift-codegen opt-full (1.74%), cargo opt-full (1.33%), clap opt-full (1.18%), and exa debug-incr-unchanged (0.28%).
* the [rust-timer run](https://github.com/rust-lang/rust/pull/121662#issuecomment-1966015546) was "only" expected to regress 5 benchmarks, largely a *different* set, by a mean of 0.5%, not the 1.1% reported above.
* not marking as triaged

Stop using LLVM struct types for byval/sret [#122050](https://github.com/rust-lang/rust/pull/122050) [(Comparison Link)](https://perf.rust-lang.org/compare.html?start=c69fda7dc664e62f8920a02a4e55d6207b212c24&end=a6d93acf5fdeb020ab86cc0d30d5672c23a7dba6&stat=instructions:u)

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

* already marked as triaged as the reported regressions are clearly spurious based on the performance graph