Exhaustiveness: Improve complexity on some wide matches #118796

Nadrieril · 2023-12-10T08:26:50Z

#118437 revealed an exponential case in exhaustiveness checking. While exponential cases are unavoidable, this one only showed up after my #117611 rewrite of the algorithm. I remember anticipating a case like this and dismissing it as unrealistic, but here we are :').

The tricky match is as follows:

match command {
    BaseCommand { field01: true, .. } => {}
    BaseCommand { field02: true, .. } => {}
    BaseCommand { field03: true, .. } => {}
    BaseCommand { field04: true, .. } => {}
    BaseCommand { field05: true, .. } => {}
    BaseCommand { field06: true, .. } => {}
    BaseCommand { field07: true, .. } => {}
    BaseCommand { field08: true, .. } => {}
    BaseCommand { field09: true, .. } => {}
    BaseCommand { field10: true, .. } => {}
    // ...20 more of the same

    _ => {}
}

To fix this, this PR formalizes a concept of "relevancy" (naming is hard) that was already used to decide what patterns to report. Now we track it for every row, which in wide matches like the above can drastically cut on the number of cases we explore. After this fix, the above match is checked with linear-many cases instead of exponentially-many.

Fixes #118437

r? @cjgillot

lqd · 2023-12-10T11:01:38Z

Out of curiosity, how much does this PR improve things on #118437, and how does that compare to the situation prior to #117611?

Nadrieril · 2023-12-10T11:22:22Z

On #118437, this goes from "takes so long that the playground timeouts" to "compiles instantly".

Compared to before the one-pass rewrite, my guess is that it's comparable in performance. Our benchmarks don't have a lot of variety on matches so it's hard to measure. The rewrite was more for code health than for performance honestly, the difference would only be noticeable for somewhat complex matches. Our benchmarks barely noticed.

Nadrieril · 2023-12-10T11:22:27Z

Let's see how this fares:

@bors try @rust-timer queue

bors · 2023-12-10T11:23:36Z

⌛ Trying commit 13a8e36 with merge 4abe247...

…, r=<try> Exhaustiveness: Improve performance on wide matches rust-lang#118437 revealed an exponential case in exhaustiveness checking. While [exponential cases are unavoidable](https://compilercrim.es/rust-np/), this one only showed up after my rust-lang#117611 rewrite of the algorithm. I remember anticipating a case like this and dismissing it as unrealistic, but here we are :'). The tricky match is as follows: ```rust match command { BaseCommand { field01: true, .. } => {} BaseCommand { field02: true, .. } => {} BaseCommand { field03: true, .. } => {} BaseCommand { field04: true, .. } => {} BaseCommand { field05: true, .. } => {} BaseCommand { field06: true, .. } => {} BaseCommand { field07: true, .. } => {} BaseCommand { field08: true, .. } => {} BaseCommand { field09: true, .. } => {} BaseCommand { field10: true, .. } => {} // ...20 more of the same _ => {} } ``` To fix this, this PR formalizes a concept of "relevancy" (naming is hard) that was already used to decide what patterns to report. Now we track it for every row, which in wide matches like the above can drastically cut on the number of cases we explore. After this fix, the above match is checked with linear-many cases instead of exponentially-many. Fixes rust-lang#118437 r? `@cjgillot`

lqd · 2023-12-10T12:24:48Z

On #118437, this goes from "takes so long that the playground timeouts" to "compiles instantly".

So if I understand correctly, #118437 also compiled instantly prior to #117611.

bors · 2023-12-10T12:51:33Z

☀️ Try build successful - checks-actions
Build commit: 4abe247 (4abe2475bffdcf7fd207c66fbd3525f1443e7fe9)

rust-timer · 2023-12-10T14:12:22Z

Finished benchmarking commit (4abe247): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 may lead 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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.7%, 3.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[0.5%, 4.6%]	3
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.3%	[2.9%, 3.8%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.935s -> 669.415s (-0.08%)
Artifact size: 314.15 MiB -> 314.10 MiB (-0.01%)

Nadrieril · 2023-12-10T17:02:36Z

So if I understand correctly, #118437 also compiled instantly prior to #117611.

Indeed! You can try it here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1f9de10106c7eec6d18773c9804e614f . On stable is the pre-#117611 behavior; on nightly is the current exponential behavior.

Between pre-#117611 and post-this-PR, the tradeoff is roughly: pre-#117611 can check some cases several times, which post-this-PR never does; conversely, there are cases that post-this-PR considers distinct but pre-#117611 does in one go.

apiraino · 2023-12-16T14:30:23Z

I'm going to pre-emptively nominate this for beta backport in case the review and merge make it to Dec, 21st.

Ref. Zulip thread where T-infra announces that the beta cut will happen a bit earlier in this release cycle

@rustbot label +beta-nominated

compiler/rustc_pattern_analysis/src/usefulness.rs

cjgillot · 2023-12-17T19:17:35Z

compiler/rustc_pattern_analysis/src/usefulness.rs

+//! exhaustiveness. This allows us to skip cases.
+//!
+//! When specializing, if there is a `Missing` case we call the other constructors "irrelevant".
+//! When there is no `Missing` case there are no irrelevant constructors.


So in the example, the constructor North is irrelevant?

That was poorly phrased, relevancy really depends on the rows. I reworked the explanation entirely

compiler/rustc_pattern_analysis/src/usefulness.rs

cjgillot · 2023-12-24T13:49:34Z

Thanks!
@bors r+

bors · 2023-12-24T13:49:37Z

📌 Commit efb04e6 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-12-24T14:40:39Z

⌛ Testing commit efb04e6 with merge 1a086e4...

bors · 2023-12-24T16:37:22Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 1a086e4 to master...

bors · 2023-12-24T16:37:23Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 1a086e4 to master...

rust-timer · 2023-12-24T18:33:44Z

Finished benchmarking commit (1a086e4): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.6%, 3.1%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 671.86s -> 670.392s (-0.22%)
Artifact size: 312.67 MiB -> 312.52 MiB (-0.05%)

Nadrieril · 2023-12-24T20:56:52Z

This needs to be backported onto 1.76.0, i.e. the beta that just branched off (or is about to be). So there's no rush, I'll wait for the current stable/beta releases to finish being released so we don't get confused about where to backport it

apiraino · 2023-12-25T12:49:38Z

@Nadrieril thank you 🙂 I see the PR has been merged with some minuses in the perf. report. Do these results need a comment? Will they be triaged during the usual weekly perf. triage?

Nadrieril · 2023-12-25T20:17:47Z

Perf feels ok to me; it's a stress-test and I'm not surprised it was affected by this change

apiraino · 2023-12-25T22:56:57Z

Awesome, then I'll mark this as perf. triaged (I think this is the correct process, in case it's not please anyone feel free to jump in and correct me 🙂 )

@rustbot label -perf-regression +perf-regression-triaged

Kobzol · 2023-12-26T09:43:09Z

@apiraino I think that normally we just apply the perf-regression-triaged label, without removing the perf-regression label (check the example command in the perf comment).

apiraino · 2023-12-28T15:36:30Z

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

Nadrieril · 2024-01-05T10:04:23Z

I seem to understand that someone will swoop in and backport this along with a bunch of other PRs at some point? Ping me if this doesn't backport cleanly.

…mulacrum [beta] backport rollup This PR backports: - rust-lang#119544: Fix: Properly set vendor in i686-win7-windows-msvc target - rust-lang#118796: Exhaustiveness: Improve complexity on some wide matches - rust-lang#119546: Temporarily disable M1 runners on GitHub Actions - rust-lang#119584: [beta] Clippy (early) beta backport - rust-lang#119559: [beta-1.76] Update cargo And also: - Bumps stage0 to released stable. r? `@Mark-Simulacrum`

Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023

rustbot assigned cjgillot Dec 10, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 10, 2023

This comment has been minimized.

Sign in to view

Nadrieril force-pushed the fix-exponential-id-match-2 branch from 41a1bf0 to 13a8e36 Compare December 10, 2023 10:06

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 10, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 10, 2023

Nadrieril changed the title ~~Exhaustiveness: Improve performance on wide matches~~ Exhaustiveness: Improve complexity on some wide matches Dec 10, 2023

This comment was marked as off-topic.

Sign in to view

Nadrieril mentioned this pull request Dec 12, 2023

exhaustiveness checking hangs forever when compiling pulsar-rs #118437

Closed

Nadrieril force-pushed the fix-exponential-id-match-2 branch from 13a8e36 to 9298104 Compare December 15, 2023 16:59

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 16, 2023

cjgillot reviewed Dec 17, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

Nadrieril force-pushed the fix-exponential-id-match-2 branch 2 times, most recently from e62ff58 to 600f3ff Compare December 20, 2023 13:42

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 23, 2023

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 Dec 24, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 24, 2023

bors merged commit 1a086e4 into rust-lang:master Dec 24, 2023
12 checks passed

rustbot added this to the 1.77.0 milestone Dec 24, 2023

Nadrieril deleted the fix-exponential-id-match-2 branch December 24, 2023 18:17

rustbot added perf-regression-triaged The performance regression has been triaged. and removed perf-regression Performance regression. labels Dec 25, 2023

apiraino added the perf-regression Performance regression. label Dec 26, 2023

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 28, 2023

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 7, 2024

Mark-Simulacrum modified the milestones: 1.77.0, 1.76.0 Jan 7, 2024

Mark-Simulacrum mentioned this pull request Jan 7, 2024

[beta] backport rollup #119719

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exhaustiveness: Improve complexity on some wide matches #118796

Exhaustiveness: Improve complexity on some wide matches #118796

Nadrieril commented Dec 10, 2023 •

edited

Loading

This comment has been minimized.

lqd commented Dec 10, 2023

Nadrieril commented Dec 10, 2023

Nadrieril commented Dec 10, 2023

This comment has been minimized.

bors commented Dec 10, 2023

lqd commented Dec 10, 2023

bors commented Dec 10, 2023

This comment has been minimized.

rust-timer commented Dec 10, 2023

Nadrieril commented Dec 10, 2023

This comment was marked as off-topic.

apiraino commented Dec 16, 2023 •

edited

Loading

cjgillot Dec 17, 2023

Nadrieril Dec 23, 2023

This comment was marked as outdated.

cjgillot commented Dec 24, 2023

bors commented Dec 24, 2023

bors commented Dec 24, 2023

bors commented Dec 24, 2023

bors commented Dec 24, 2023

rust-timer commented Dec 24, 2023

Nadrieril commented Dec 24, 2023

apiraino commented Dec 25, 2023

Nadrieril commented Dec 25, 2023

apiraino commented Dec 25, 2023

Kobzol commented Dec 26, 2023

apiraino commented Dec 28, 2023

Nadrieril commented Jan 5, 2024

Exhaustiveness: Improve complexity on some wide matches #118796

Exhaustiveness: Improve complexity on some wide matches #118796

Conversation

Nadrieril commented Dec 10, 2023 • edited Loading

This comment has been minimized.

lqd commented Dec 10, 2023

Nadrieril commented Dec 10, 2023

Nadrieril commented Dec 10, 2023

This comment has been minimized.

bors commented Dec 10, 2023

lqd commented Dec 10, 2023

bors commented Dec 10, 2023

This comment has been minimized.

rust-timer commented Dec 10, 2023

Overall result: ❌ regressions - ACTION NEEDED

Nadrieril commented Dec 10, 2023

This comment was marked as off-topic.

apiraino commented Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

cjgillot commented Dec 24, 2023

bors commented Dec 24, 2023

bors commented Dec 24, 2023

bors commented Dec 24, 2023

bors commented Dec 24, 2023

rust-timer commented Dec 24, 2023

Overall result: ❌ regressions - ACTION NEEDED

Nadrieril commented Dec 24, 2023

apiraino commented Dec 25, 2023

Nadrieril commented Dec 25, 2023

apiraino commented Dec 25, 2023

Kobzol commented Dec 26, 2023

apiraino commented Dec 28, 2023

Nadrieril commented Jan 5, 2024

Nadrieril commented Dec 10, 2023 •

edited

Loading

apiraino commented Dec 16, 2023 •

edited

Loading