-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Clarify main code paths in exhaustiveness checking #78430
Clarify main code paths in exhaustiveness checking #78430
Conversation
First is checking for constructor overlap, second is extracting the resulting fields.
Since the constructor is recomputed a lot, caching is worth it.
This is even a perf improvement on the match-heavy benchmarks.
This only happens in a slow (diagnostics) path, so the code clarity gain is worth it.
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.
The test change is because we used to treat `&str` like other `&T`s, ie as having a single constructor. That's not quite true though since we consider `&str` constants as atomic instead of refs to `str` constants.
After splitting, subtraction becomes much simpler
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 766ab78 with merge 5fa81289ac2b01e09c5de71dc65a406e9e69d839... |
☀️ Try build successful - checks-actions |
Queued 5fa81289ac2b01e09c5de71dc65a406e9e69d839 with parent 56d288f, future comparison URL. |
Finished benchmarking try commit (5fa81289ac2b01e09c5de71dc65a406e9e69d839): 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 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice series of improvements to the code; it feels a lot simpler now. Some of my comments may no longer make sense (or may apply to a different location), because I reviewed per-commit, but overall I have no issues with the refactoring itself: I just have a few comments about the comments and a couple of nits (I've been a little picky). After that, this should be good to merge. And that speed up is impressive ❤️
9b42c7c
to
41a74ac
Compare
Oh wow, no actual code comments, I must be getting good at this ^^. Thanks for the review! @rustbot modify labels: -S-waiting-on-author +S-waiting-on-review |
This is an amazing PR. Refactoring, more lines removed than added, more comments, massive perf improvement. Great job! |
Thanks, this looks great! @bors r+ rollup=never |
📌 Commit 41a74ac has been approved by |
☀️ Test successful - checks-actions |
Before rust-lang#78430, string literals worked because `specialize_constructor` didn't actually care too much which constructor was passed to it unless needed. Since then, string literals are special cased and a bit hacky. I did not anticipate patterns for the `&str` type other than string literals, hence this bug. This makes string literals less hacky.
Fix rust-lang#78549 Before rust-lang#78430, this worked because `specialize_constructor` didn't actually care too much which constructor was passed to it unless needed. That PR however handles `&str` as a special case, and I did not anticipate patterns for the `&str` type other than string literals. I am not very confident there are not other similar oversights left, but hopefully only `&str` was different enough to break my assumptions. Fixes rust-lang#78549
Excellent performance work, and followed by another PR further improving performance (#78553). Even better a refactoring of one of the most dense pieces of the compiler (at least IMO). |
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 #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