Turn type inhabitedness into a query to fix `exhaustive_patterns` perf #79670

Nadrieril · 2020-12-03T19:31:04Z

We measured in #79394 that enabling the exhaustive_patterns feature causes significant perf degradation. It was conjectured that the culprit is type inhabitedness checking, and I hypothesized that turning this computation into a query would solve most of the problem.

This PR turns tcx.is_ty_uninhabited_from into a query, and I measured a 25% perf gain on the benchmark that stress-tests exhaustiveness_patterns. This more than compensates for the 30% perf hit I measured when creating it. We'll have to measure enabling the feature again, but I suspect this fixes the perf regression entirely.
I'd like a perf run on this PR obviously.
I made small atomic commits to help reviewing. The first one is just me discovering the "revisions" feature of the testing framework.

I believe there's a push to move things out of rustc_middle because it's huge. I guess inhabitedness/mod.rs could be moved out, but it's quite small. DefIdForest might be movable somewhere too. I don't know what the policy is for that.

Ping @camelid since you were interested in following along
@rustbot modify labels: +A-exhaustiveness-checking

rust-highfive · 2020-12-03T19:31:08Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

jonas-schievink · 2020-12-03T19:40:14Z

@bors try @rust-timer queue

rust-timer · 2020-12-03T19:40:16Z

Awaiting bors try build completion

bors · 2020-12-03T19:40:25Z

⌛ Trying commit e4c3571 with merge eae765f378d0f787f681b2b627b79583c4920d53...

bors · 2020-12-03T20:22:47Z

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

rust-timer · 2020-12-03T20:22:49Z

Queued eae765f378d0f787f681b2b627b79583c4920d53 with parent 1f95c91, future comparison URL.

rust-timer · 2020-12-03T22:03:42Z

Finished benchmarking try commit (eae765f378d0f787f681b2b627b79583c4920d53): 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
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

camelid · 2020-12-03T22:09:17Z

Wow, very impressive @Nadrieril! Very significant reductions in instruction counts on the stress tests, and significant improvements for ripgrep, cargo, and syn!

camelid · 2020-12-03T22:09:29Z

Should this be relnotes-perf?

JohnCSimon · 2021-01-11T06:14:56Z

merge conflict - sent to author

estebank · 2021-01-12T01:00:46Z

Holy hell! Those perf numbers! Sorry I was away for all of December. Reviewing now. I wish I had reviewed before conflicts cropped up.

estebank · 2021-01-12T01:08:34Z

src/test/ui/pattern/usefulness/empty-match.exhaustive_patterns.stderr

+error[E0004]: non-exhaustive patterns: type `u8` is non-empty
+  --> $DIR/empty-match.rs:78:20
   |
-LL |         Some(_) => {}
-   |         ^^^^^^^
-
-error: unreachable pattern
-  --> $DIR/empty-match.rs:87:9
+LL |     match_no_arms!(0u8);
+   |                    ^^^
   |
-LL |         Some(_) => {}
-   |         ^^^^^^^
+   = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
+   = note: the matched value is of type `u8`


This is not introduced by this PR, but we have simultaneously redundant info (we mention the type u8 twice in two different places), and use "weird jargon" (u8 is non-empty). I would like to reword these errors (after landing this PR, probably).

Somehow I felt that there was an issue for that already, but I can't find it. Could have confused it with #78123 .

estebank · 2021-01-12T01:10:20Z

src/test/ui/pattern/usefulness/empty-match.exhaustive_patterns.stderr

+error[E0004]: non-exhaustive patterns: type `NonEmptyStruct1` is non-empty
+  --> $DIR/empty-match.rs:79:20
   |
-LL |     match_empty!(0u8);
-   |                  ^^^
+LL | struct NonEmptyStruct1;
+   | ----------------------- `NonEmptyStruct1` defined here
+...
+LL |     match_no_arms!(NonEmptyStruct1);
+   |                    ^^^^^^^^^^^^^^^
   |
   = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
-   = note: the matched value is of type `u8`
+   = note: the matched value is of type `NonEmptyStruct1`


I would like us to handle this case in particular (matching on an empty struct) explicitly in the diagnostic output as it might not be obvious to a newcomer.

estebank

Ended up spending more time going over the output than the actual code, as it looks good to me (thank you for the easy to follow commits!). r=me after fixing the merge conflict.

camelid · 2021-01-12T01:42:49Z

I feel a bit confused about e4c3571: I thought SmallVec<[T; 1]> only allocated when n > 1?

estebank · 2021-01-12T02:21:38Z

@camelid correct. Given e4c3571#diff-ab398df590486c873907d006df0eec96f9fdeb86131a756f5d25a29401de4dcaR110-R113 I would wonder if fiddling with N could yield even better improvements, but that requires a bit of a trial and error (or to look at granular instrumented perf graphs of the compiler, which we don't have today).

Nadrieril · 2021-01-12T15:35:00Z

I feel a bit confused about e4c3571: I thought SmallVec<[T; 1]> only allocated when n > 1?

Yes, but the smallvec in question needed to be behind an Arc, which always allocates. So the alternative is between Arc<SmallVec<[DefId;1]>> which allocates once most of the time and sometimes twice, and my custom thing that allocates 0 times most of the time, and sometimes once.
And by "most of the time", I mean "really all of the time except arcane cases", namely types that are knowably empty from two disjoint modules. Even for a privately empty field (which is rare enough), the forest would just contain the DefId of the module. To require two DefIds you need weird situations like this one.

Since `DefIdForest` contains 0 or 1 elements the large majority of the time, by allocating only in the >1 case we avoid almost all allocations, compared to `Arc<SmallVec<[DefId;1]>>`. This shaves off 0.2% on the benchmark that stresses uninhabitedness checking.

Nadrieril · 2021-01-12T20:34:35Z

Heh, the conflict was just a typo fix.

@bors r=estebank rollup=never

bors · 2021-01-12T20:34:36Z

📌 Commit e608d8f has been approved by estebank

bors · 2021-01-12T22:58:48Z

⌛ Testing commit e608d8f with merge 058a710...

bors · 2021-01-13T01:40:34Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 058a710 to master...

rustbot added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 3, 2020

rust-highfive assigned estebank Dec 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020

Nadrieril mentioned this pull request Dec 3, 2020

[DO NOT MERGE] Enable exhaustive_patterns by default #79394

Closed

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 3, 2020

Nadrieril mentioned this pull request Dec 12, 2020

Tracking issue for RFC 1872: exhaustive_patterns feature #51085

Open

3 tasks

estebank reviewed Jan 12, 2021

View reviewed changes

estebank approved these changes Jan 12, 2021

View reviewed changes

Nadrieril added 4 commits January 12, 2021 19:59

Deduplicate some tests using revisions

622bfdb

Add tests for uninhabited types

b82f149

Turn type inhabitedness into a query

8598c9f

Nadrieril force-pushed the uninhabited-query branch from e4c3571 to e608d8f Compare January 12, 2021 20:33

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 Jan 12, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2021

bors merged commit 058a710 into rust-lang:master Jan 13, 2021

rustbot added this to the 1.51.0 milestone Jan 13, 2021

Nadrieril deleted the uninhabited-query branch January 13, 2021 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn type inhabitedness into a query to fix `exhaustive_patterns` perf #79670

Turn type inhabitedness into a query to fix `exhaustive_patterns` perf #79670

Nadrieril commented Dec 3, 2020 •

edited

Loading

rust-highfive commented Dec 3, 2020

jonas-schievink commented Dec 3, 2020

rust-timer commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

rust-timer commented Dec 3, 2020

rust-timer commented Dec 3, 2020

camelid commented Dec 3, 2020

camelid commented Dec 3, 2020

JohnCSimon commented Jan 11, 2021

estebank commented Jan 12, 2021

estebank Jan 12, 2021

Nadrieril Jan 12, 2021

estebank Jan 12, 2021

estebank left a comment •

edited

Loading

camelid commented Jan 12, 2021

estebank commented Jan 12, 2021

Nadrieril commented Jan 12, 2021 •

edited

Loading

Nadrieril commented Jan 12, 2021

bors commented Jan 12, 2021

bors commented Jan 12, 2021

bors commented Jan 13, 2021

Turn type inhabitedness into a query to fix exhaustive_patterns perf #79670

Turn type inhabitedness into a query to fix exhaustive_patterns perf #79670

Conversation

Nadrieril commented Dec 3, 2020 • edited Loading

rust-highfive commented Dec 3, 2020

jonas-schievink commented Dec 3, 2020

rust-timer commented Dec 3, 2020

bors commented Dec 3, 2020

bors commented Dec 3, 2020

rust-timer commented Dec 3, 2020

rust-timer commented Dec 3, 2020

camelid commented Dec 3, 2020

camelid commented Dec 3, 2020

JohnCSimon commented Jan 11, 2021

estebank commented Jan 12, 2021

estebank Jan 12, 2021

Choose a reason for hiding this comment

Nadrieril Jan 12, 2021

Choose a reason for hiding this comment

estebank Jan 12, 2021

Choose a reason for hiding this comment

estebank left a comment • edited Loading

Choose a reason for hiding this comment

camelid commented Jan 12, 2021

estebank commented Jan 12, 2021

Nadrieril commented Jan 12, 2021 • edited Loading

Nadrieril commented Jan 12, 2021

bors commented Jan 12, 2021

bors commented Jan 12, 2021

bors commented Jan 13, 2021

Turn type inhabitedness into a query to fix `exhaustive_patterns` perf #79670

Turn type inhabitedness into a query to fix `exhaustive_patterns` perf #79670

Nadrieril commented Dec 3, 2020 •

edited

Loading

estebank left a comment •

edited

Loading

Nadrieril commented Jan 12, 2021 •

edited

Loading