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.

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

Add an intermediate representation to exhaustiveness checking #88950

Merged
merged 13 commits into from
Sep 29, 2021

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Sep 15, 2021

The exhaustiveness checking algorithm keeps deconstructing patterns into a Constructor and some Fields, but does so a bit all over the place. This PR introduces a new representation for patterns that already has that information, so we only compute it once at the start.
I find this makes code easier to follow. In particular DeconstructedPat::specialize is a lot simpler than what happened before, and more closely matches the description of the algorithm. I'm also hoping this could help for the project of librarifying exhaustiveness for rust_analyzer since it decouples the algorithm from rustc_middle::Pat.

@camelid camelid added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns labels Sep 15, 2021
@Nadrieril

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 15, 2021
@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rustbot rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 15, 2021
@camelid
Copy link
Member

camelid commented Sep 15, 2021

Interesting, issue-58319 doesn't seem to have a pattern match. I guess it regressed because it has a large struct definition that is being lowered?

@Nadrieril
Copy link
Member Author

It's weird though, because patterns used to be lowered already via a PatternFolder, which I imagine traverses everythings and reallocates. So I didn't expect that my new lowering would be much slower

@Nadrieril
Copy link
Member Author

Ok, some progress. My local measurements differ quite a bit from perf here, so I want to see what it says now.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 17, 2021
@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@Nadrieril
Copy link
Member Author

Huh ok I found the main issue; commit 180204f (#88950) fixes it. Apparently changing from an explicit for loop to the equivalent iterator expression killed perf specifically for match-stress-enum. The rest of the regression was because cloning my new struct is slower than copying a ref, so I arena-allocated my struct too.
I don't know how to stop the timer, let's hope rescheduling does it.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@Nadrieril
Copy link
Member Author

Ohh I can trivialize the terrible mess that was SubPatSet now! Biig simplification, this was the most arcane part of the algorithm. Help I can't stop simplifying things 🙃 . I'll have to stop when a reviewer comes by

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 25, 2021
@bors
Copy link
Contributor

bors commented Sep 25, 2021

⌛ Trying commit b7e358e with merge 86bbc6573b2248ce60968ba6b29dd56dc4ac80c8...

@Nadrieril
Copy link
Member Author

@varkor remember the mess I introduced to solve #80632? It's all in this cute function now https://github.com/Nadrieril/rust/blob/b7e358ee17a5794603b2324858de078c4586acfc/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs#L1577-L1586 ! 7 lines of this do the job of about 250 lines of convoluted SubPatSet logic x)

@bors
Copy link
Contributor

bors commented Sep 26, 2021

☀️ Try build successful - checks-actions
Build commit: 86bbc6573b2248ce60968ba6b29dd56dc4ac80c8 (86bbc6573b2248ce60968ba6b29dd56dc4ac80c8)

@rust-timer
Copy link
Collaborator

Queued 86bbc6573b2248ce60968ba6b29dd56dc4ac80c8 with parent addb4da, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (86bbc6573b2248ce60968ba6b29dd56dc4ac80c8): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -2.8% on full builds of unicode_normalization)
  • Large regression in instruction counts (up to 1.4% on full builds of match-stress-enum)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 26, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2021

😆 I'm getting to it right now

@oli-obk
Copy link
Contributor

oli-obk commented Sep 28, 2021

r=me unless you want to keep investigating the stress test perf regression

@Nadrieril
Copy link
Member Author

Yay thanks!

Most (1%) of the regression comes from the last commit, presumably the part where we retraverse all the patterns. Since that didn't affect the more realistic benchmarks, and no real match has 8000 arms, I believe that retraversing the patterns takes negligible time in real cases. So I'll ignore that 1%.
Imo it's plenty worth it in code clarity anyways.
@rustbot label: +perf-regression-triaged

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 28, 2021

📌 Commit b7e358e has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2021
@bors
Copy link
Contributor

bors commented Sep 29, 2021

⌛ Testing commit b7e358e with merge 6df1d82...

@DevinR528
Copy link
Contributor

Just want to second how much more readable the code is without SubPatSet (after b7e358e) everything looks better look forward to having to rebase 😜!

@BubbaSheen

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 29, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6df1d82 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2021
@bors bors merged commit 6df1d82 into rust-lang:master Sep 29, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 29, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6df1d82): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

  • Very large improvement in instruction counts (up to -2.6% on full builds of unicode_normalization)
  • Very large regression in instruction counts (up to 2.9% on full builds of match-stress-enum)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants