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

25% regression in compile time of issue-20936-deep-vector benchmark #43511

Closed
alexcrichton opened this issue Jul 27, 2017 · 3 comments · Fixed by #43576
Closed

25% regression in compile time of issue-20936-deep-vector benchmark #43511

alexcrichton opened this issue Jul 27, 2017 · 3 comments · Fixed by #43576
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

According to perf.rust-lang.org this benchmarked experienced a >25% regressoin in compile time due to #39409

Seems bad!

cc @pnkfelix, @rust-lang/compiler
@alexcrichton alexcrichton added I-nominated regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 27, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

We got a lot of stuff on our plate, and lacking evidence that this regression is more widespread, we'll consider this p-medium for now. The cause is certainly that EndRegion added a bunch of MIR statements. We can perhaps fix by adding some filtering?

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Jul 27, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 28, 2017
@alexcrichton
Copy link
Member Author

Looking at some data again, it's worth nothing that #39409 regressed almost all benchmarks, much more so than #42808 which is also perceived as a relatively high-priority regression.

@alexcrichton alexcrichton mentioned this issue Jul 31, 2017
Closed
@arielb1 arielb1 mentioned this issue Jul 31, 2017
Merged
bors added a commit that referenced this issue Aug 1, 2017
@bors
Auto merge of #43576 - arielb1:no-unneeded-unwind, r=eddyb
640cfc8 
rustc_mir: don't build unused unwind cleanup blocks

When building a scope exit, don't build unwind cleanup blocks unless they will actually be used by the unwind path of a drop - the unused blocks are removed by SimplifyCfg, but they can cause a significant performance slowdown before they are removed. That fixes #43511.

Also a few other small MIR cleanups & optimizations.

r? @eddyb
@alexcrichton
Copy link
Member Author

FWIW the perf graph here shows that we recovered almost all of the performance here, but there's still a 10% regression from before #39409. @arielb1 do you think it's worth opening another issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

rustc_mir: don't build unused unwind cleanup blocks arielb1/rust
4 participants
@alexcrichton @nikomatsakis @Mark-Simulacrum @rust-highfive