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

Exhaustiveness: Improve complexity on some wide matches #118796

Merged
merged 2 commits into from
Dec 24, 2023

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Dec 10, 2023

#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

@Nadrieril Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023
@rustbot 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
@rust-log-analyzer

This comment has been minimized.

@lqd
Copy link
Member

lqd commented Dec 10, 2023

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

@Nadrieril
Copy link
Member Author

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
Copy link
Member Author

Let's see how this fares:

@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 Dec 10, 2023
@bors
Copy link
Contributor

bors commented Dec 10, 2023

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2023
…, 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
Copy link
Member

lqd commented Dec 10, 2023

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
Copy link
Contributor

bors commented Dec 10, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot 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
Copy link
Member Author

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.

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

This comment was marked as off-topic.

@apiraino
Copy link
Contributor

apiraino commented Dec 16, 2023

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

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 16, 2023
compiler/rustc_pattern_analysis/src/usefulness.rs Outdated Show resolved Hide resolved
compiler/rustc_pattern_analysis/src/usefulness.rs Outdated Show resolved Hide resolved
//! 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the example, the constructor North is irrelevant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

compiler/rustc_pattern_analysis/src/usefulness.rs Outdated Show resolved Hide resolved
@bors

This comment was marked as outdated.

@Nadrieril 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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 23, 2023
@cjgillot
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Dec 24, 2023

📌 Commit efb04e6 has been approved by cjgillot

It is now in the queue for this repository.

@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 Dec 24, 2023
@bors
Copy link
Contributor

bors commented Dec 24, 2023

⌛ Testing commit efb04e6 with merge 1a086e4...

@bors
Copy link
Contributor

bors commented Dec 24, 2023

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

1 similar comment
@bors
Copy link
Contributor

bors commented Dec 24, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 24, 2023
@bors bors merged commit 1a086e4 into rust-lang:master Dec 24, 2023
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Dec 24, 2023
@Nadrieril Nadrieril deleted the fix-exponential-id-match-2 branch December 24, 2023 18:17
@rust-timer
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

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

@apiraino
Copy link
Contributor

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

@rustbot rustbot added perf-regression-triaged The performance regression has been triaged. and removed perf-regression Performance regression. labels Dec 25, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Dec 26, 2023

@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 apiraino added the perf-regression Performance regression. label Dec 26, 2023
@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 28, 2023
@Nadrieril
Copy link
Member Author

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.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 7, 2024
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.77.0, 1.76.0 Jan 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2024
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2024
…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`
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 beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exhaustiveness checking hangs forever when compiling pulsar-rs
10 participants