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 fast path for match checking #76918

Merged
merged 3 commits into from
Sep 24, 2020
Merged

Conversation

ishitatsuyuki
Copy link
Contributor

This adds a fast path that would reduce the complexity to linear on matches consisting of only variant patterns (i.e. enum matches). (Also see: #7462) Unfortunately, I was too lazy to add a similar fast path for constants (mostly for integer matches), ideally that could be added another day.

TBH, I'm not confident with the performance claims due to the fact that enums tends to be small and FxHashMap could add a lot of overhead.

r? @Mark-Simulacrum

needs perf

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 19, 2020

⌛ Trying commit 7c98f6f with merge 52836408ae482f87159a2473c3e5475348b1f255...

@bors
Copy link
Contributor

bors commented Sep 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 52836408ae482f87159a2473c3e5475348b1f255 (52836408ae482f87159a2473c3e5475348b1f255)

@rust-timer
Copy link
Collaborator

Queued 52836408ae482f87159a2473c3e5475348b1f255 with parent fd702d2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (52836408ae482f87159a2473c3e5475348b1f255): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@ishitatsuyuki
Copy link
Contributor Author

Seems that all perf results are nearly identical. I guess that match checking wasn't dominating the compile time then.

It seems that this PR only helps in pathological cases like that generated by the script below. Some crate in the ecosystem might be doing the same kind of code generation, though, and this can be become useful in such cases.

Python script:

print("""#![allow(warnings)]
enum A {""")
for i in range(8192):
    print(f"    Var{i},")
print("""}
fn main() {
    match A::Var0 {""")
for i in range(8192):
    print(f"        A::Var{i} => {{}}")
print("""    }
}""")

Old rustc takes around 2.7s, this build takes around 1.7s.

@panstromek
Copy link
Contributor

panstromek commented Sep 20, 2020

Btw. Addressing your concerns about FxHashMap overhead - #72412 added a MiniMap for similar reasons (it's SSO map backed by ArrayVec). You can try using that here, seems like the map will always hold small number of elements, too, if I understand it correctly.

@ishitatsuyuki
Copy link
Contributor Author

It seems that MiniMap is pretty limited - it doesn't support iteration and entry. So for now I'm not going to make the switch.

@ishitatsuyuki
Copy link
Contributor Author

I added some more comments explaining the code per request.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 21, 2020

Thanks, that helps a lot!

One thing that we could do is to add your pathological case to the perf test suite, then, once that's in, this PR will show an improvement on the perf bot and we thus guard against future regressions.

Another thing is to debug_assert! that the result of the fast path is the same as the slow path. This will require running both paths if debug assertions are activated, but I think it could be reasonably added. What do you think?

@ishitatsuyuki
Copy link
Contributor Author

I added the debug assertions, it has a clone that is not really necessary but I thought the code would be easier to maintain this way.

As for perf, I have mixed feelings since the case I mentioned above is only one kind of match that is slow; I think we could add it once we have support for integer cases like #7462 (comment).

@oli-obk
Copy link
Contributor

oli-obk commented Sep 22, 2020

You can add perf tests for both the integer case and your case within one test, then we are at least tracking them, and any changes to it will show up. If the tests just take a few seconds, that's perfect for perf, as it won't really slow down the perf test suite

@oli-obk
Copy link
Contributor

oli-obk commented Sep 22, 2020

Implementation wise this PR lgtm now, so we could merge it and add the perf test later if you prefer. We'd lose the visible improvement in the perf tests, but I'm not sure how much value that has.

@ishitatsuyuki
Copy link
Contributor Author

Perf PR is up at rust-lang/rustc-perf#769.

@ishitatsuyuki
Copy link
Contributor Author

The match-stress-enum perf benchmark is now fully deployed.

@Mark-Simulacrum
Copy link
Member

r? @oli-obk -- it sounds like you're r+ on this, but not quite sure.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 24, 2020

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 24, 2020

📌 Commit 01a771a 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 24, 2020
@bors
Copy link
Contributor

bors commented Sep 24, 2020

⌛ Testing commit 01a771a with merge e599b53...

@bors
Copy link
Contributor

bors commented Sep 24, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing e599b53 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2020
@bors bors merged commit e599b53 into rust-lang:master Sep 24, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 24, 2020
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Oct 27, 2020
Also removes the ugly caching that was introduced in rust-lang#76918. It was
bolted on without deeper knowledge of the workings of the algorithm.
This commit manages to be more performant without any of the complexity.
It should be better on representative workloads too.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 29, 2020
…ly2, r=varkor

Clarify main code paths in exhaustiveness checking

This PR massively clarifies the main code paths of exhaustiveness checking, by using the `Constructor` enum to a fuller extent. I've been itching to write it for more than a year, but the complexity of matching consts had prevented me. Behold a massive simplification :D.
This in particular removes a fair amount of duplication between various parts, localizes code into methods of relevant types when applicable, makes some implicit assumptions explicit, and overall improves legibility a lot (or so I hope). Additionally, after my changes undoing rust-lang#76918 turned out to be a noticeable perf gain.

As usual I tried my best to make the commits self-contained and easy to follow. I've also tried to keep the code well-commented, but I tend to forget how complex this file is; I'm happy to clarify things as needed.
My measurements show good perf improvements on the two match-heavy benchmarks (-18.0% on `unicode_normalization-check`! :D); I'd like a perf run to check the overall impact.

r? `@varkor`
`@rustbot` modify labels: +A-exhaustiveness-checking
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2023
exhaustiveness: Rework constructor splitting

`SplitWildcard` was pretty opaque. I replaced it with a more legible abstraction: `ConstructorSet` represents the set of constructors for patterns of a given type. This clarifies responsibilities: `ConstructorSet` handles one clear task, and diagnostic-related shenanigans can be done separately.

I'm quite excited, I had has this in mind for years but could never quite introduce it. This opens up possibilities, including type-specific optimisations (like using a `FxHashSet` to collect enum variants, which had been [hackily attempted some years ago](rust-lang#76918)), my one-pass rewrite (rust-lang#116042), and future librarification.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2023
exhaustiveness: Rework constructor splitting

`SplitWildcard` was pretty opaque. I replaced it with a more legible abstraction: `ConstructorSet` represents the set of constructors for patterns of a given type. This clarifies responsibilities: `ConstructorSet` handles one clear task, and diagnostic-related shenanigans can be done separately.

I'm quite excited, I had has this in mind for years but could never quite introduce it. This opens up possibilities, including type-specific optimisations (like using a `FxHashSet` to collect enum variants, which had been [hackily attempted some years ago](rust-lang#76918)), my one-pass rewrite (rust-lang#116042), and future librarification.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 14, 2023
exhaustiveness: Rework constructor splitting

`SplitWildcard` was pretty opaque. I replaced it with a more legible abstraction: `ConstructorSet` represents the set of constructors for patterns of a given type. This clarifies responsibilities: `ConstructorSet` handles one clear task, and diagnostic-related shenanigans can be done separately.

I'm quite excited, I had has this in mind for years but could never quite introduce it. This opens up possibilities, including type-specific optimisations (like using a `FxHashSet` to collect enum variants, which had been [hackily attempted some years ago](rust-lang/rust#76918)), my one-pass rewrite (rust-lang/rust#116042), and future librarification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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