Skip to content
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

Jump to bottom

perf triage for 2021-07-20. #930

Merged
merged 2 commits into from
Jul 22, 2021
Merged

perf triage for 2021-07-20. #930

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7c03bc
perf triage for 2021-07-20.
pnkfelix Jul 21, 2021
9f1e665
review feedback: added links to my comments, and removed the nags tha…
pnkfelix Jul 22, 2021
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
73 changes: 73 additions & 0 deletions triage/2021-07-20.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# 2021-07-20 Triage Log

A mixed week, with some moderate regressions and moderate improvements. (I am personally wondering whether style-servo-debug needs to have a larger individual threshold for signalling a regression.) There were some notable PR's that were specifically oriented around performance enhancements.

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

3 Regressions, 3 Improvements, 3 Mixed; 1 of them in rollups

#### Regressions

Replace associated item bound vars with placeholders when projecting [#86993](https://github.com/rust-lang/rust/issues/86993)
- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=b1f8e27b74c541d3d555149c8efa4bfe9385cd56&end=27e42058811e448b1a7dd8630d86ab247fbfcb9b&stat=instructions:u) (up to 1.5% on `incr-patched: b9b3e592dd cherry picked` builds of `style-servo-debug`)
- A perf run was [done on the PR originally](https://perf.rust-lang.org/compare.html?start=fdfe819580062a441024d713b49340cd3f7d7efc&end=7baa78ec683fd14db4d4c1869dbef5cbbc5b774d). The regression flagged here seems to be on a different set of benchmarks: `style-servo-debug` and `wf-projection-stress-65510-*`, neither of which was flagged as a regression in the original runs.
pnkfelix marked this conversation as resolved.
Show resolved Hide resolved
- Results overall seem pretty mixed and hard to interpret. Left a [comment](https://github.com/rust-lang/rust/pull/86993#issuecomment-883624762) on the PR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the triage report labeled this as a regression when the results seem so mixed... I will investigate this.


Remove special case for `ExprKind::Paren` in `MutVisitor` [#87284](https://github.com/rust-lang/rust/issues/87284)
- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=014026d1a7ca991f82f12efa95ef4dffb29dc8af&end=6535449a002264ee08dec8e741f1aadd97428fae&stat=instructions:u) (up to 1.0% on `full` builds of `coercions-debug`)
- This is on the borderline for "significant regression", and was the *only* regression associated with this PR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I'd like to make the logic for labeling runs as regressions to be a bit smarter in these cases. We should be able to encode that if there is only a single noisy benchmark which regressed and that regression is barely significant, we shouldn't count it as a regression.

- Also, `coercions-debug` seems like it has a noisy history (that's pnkfelix's opinion from eyeballing it; it has also a single "?" on its comparison line.)

Better diagnostics with mismatched types due to implicit static lifetime [#87244](https://github.com/rust-lang/rust/issues/87244)
- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=718d53b0cb7dde93499cb92950d60b412f5a3d05&end=da7d405357600a76f2b93b8aa41fe5ee5da7885d&stat=instructions:u) (up to 1.2% on `full` builds of `stm32f4-check`)
- Widespread regressions that exceed the 0.2% threshold for non-noisy benchmarks.
- Left a [comment](https://github.com/rust-lang/rust/pull/87244#issuecomment-883635813) asking if this was expected. But I suspect the situation does not warrant additional investment of effort, assuming there's no trivial fix identifiable from the PR author.

#### Improvements

Rollup of 6 pull requests [#87118](https://github.com/rust-lang/rust/issues/87118)
- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=a08f25a7ef2800af5525762e981c24d96c14febe&end=ee5ed4a88d6a869cdb152829ed697d6459650db3&stat=instructions:u) (up to -1.9% on `full` builds of `deeply-nested-async-check`)


Make expansions stable for incr. comp. [#86676](https://github.com/rust-lang/rust/issues/86676)
- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=c78ebb7bdcfc924a20fd069891ffe1364d6814e7&end=68511b574ffe019a5cb3e9fa92605f80d39167bc&stat=instructions:u) (up to -2.4% on `full` builds of `externs-debug`)


Some perf optimizations and logging [#87203](https://github.com/rust-lang/rust/issues/87203)
- Very large improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=68511b574ffe019a5cb3e9fa92605f80d39167bc&end=c7331d65bdbab1187f5a9b8f5b918248678ebdb9&stat=instructions:u) (up to -21.7% on `full` builds of `deeply-nested-async-check`)


#### Mixed

Cache expansion hash globally [#87044](https://github.com/rust-lang/rust/issues/87044)
- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=3e1c75c6e25a4db968066bd2ef2dabc7c504d7ca&end=c7d6bcc788ef6b2293d2d5166a9b0339d5d03b0a&stat=instructions:u) (up to 1.9% on `full` builds of `deeply-nested-async-check`)
- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=3e1c75c6e25a4db968066bd2ef2dabc7c504d7ca&end=c7d6bcc788ef6b2293d2d5166a9b0339d5d03b0a&stat=instructions:u) (up to -1.4% on `incr-unchanged` builds of `inflate-check`)
- This was an improvement for a lot of incremental cases, and a regression for a few full-compilation cases: `deeply-nested-async-{check,debug,opt}`, and `coercions-debug`.
- It seems like these results were [expected](https://github.com/rust-lang/rust/pull/87044#issuecomment-879407381), and they make sense given the nature of the PR.

Update Rust Float-Parsing Algorithms to use the Eisel-Lemire algorithm. [#86761](https://github.com/rust-lang/rust/issues/86761)
- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=64d171b8a419eb6cb872ab579398eff8a741bbc6&end=f502bd3abd12111bbfae0974db018c165a977c0e&stat=instructions:u) (up to 3.2% on `incr-patched: b9b3e592dd cherry picked` builds of `style-servo-debug`)
- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=64d171b8a419eb6cb872ab579398eff8a741bbc6&end=f502bd3abd12111bbfae0974db018c165a977c0e&stat=instructions:u) (up to -1.4% on `full` builds of `piston-image-opt`)
- A perf run was done on the orignal PR, but the run there did not predict the regression that was now observed to `style-servo-debug` when this PR landed on nightly.
- Left a comment, but I am guessing that `style-servo-debug` may need a bigger noise threshold (or rather, sensitivity to perturbations) than what we currently use.
pnkfelix marked this conversation as resolved.
Show resolved Hide resolved

Move OnDiskCache to rustc_query_impl. [#86698](https://github.com/rust-lang/rust/issues/86698)
- Moderate improvement in [instruction counts](https://perf.rust-lang.org/compare.html?start=5a8a44196b3cf099f8c9b0156bd902eaec0b4e5f&end=18073052d8c3544ccb73effd289ed3acda0d66c0&stat=instructions:u) (up to -1.4% on `incr-unchanged` builds of `ctfe-stress-4-check`)
- Moderate regression in [instruction counts](https://perf.rust-lang.org/compare.html?start=5a8a44196b3cf099f8c9b0156bd902eaec0b4e5f&end=18073052d8c3544ccb73effd289ed3acda0d66c0&stat=instructions:u) (up to 1.1% on `incr-unchanged` builds of `deeply-nested-async-opt`)
- A perf run was done on the original PR that predicted a much better outcome here.
- Left a comment asking PR author if they have any idea about the discrepancy.
pnkfelix marked this conversation as resolved.
Show resolved Hide resolved

#### Untriaged Pull Requests

- [#87153 \[debuginfo\] Emit associated type bindings in trait object type names.](https://github.com/rust-lang/rust/pull/87153)
- [#86993 Replace associated item bound vars with placeholders when projecting](https://github.com/rust-lang/rust/pull/86993)
- [#86777 Include terminators in instance size estimate](https://github.com/rust-lang/rust/pull/86777)
- [#86588 Rollup of 8 pull requests](https://github.com/rust-lang/rust/pull/86588)
- [#86034 Change entry point to 🛡️ against 💥 💥-payloads](https://github.com/rust-lang/rust/pull/86034)
- [#84560 Inline Iterator as IntoIterator.](https://github.com/rust-lang/rust/pull/84560)

#### Nags requiring follow up

* [PR #87244 followup](https://github.com/rust-lang/rust/pull/87244#issuecomment-883635813)
* [PR #86698 followup](https://github.com/rust-lang/rust/pull/86698#issuecomment-883647259)
pnkfelix marked this conversation as resolved.
Show resolved Hide resolved