Exhaustiveness: use an `Option` instead of allocating fictitious patterns #119688

Nadrieril · 2024-01-07T10:22:29Z

In the process of exhaustiveness checking, Matrix stores a 2D array of patterns. Those are subpatterns of the patterns we were provided as input, except sometimes we allocate some extra wildcard patterns to fill a hole during specialization.

Morally though, we could store Option<&'p DeconstructedPat> in the matrix, where None signifies a wildcard. That way we'd only have "real" patterns in the matrix and we wouldn't need the arena to allocate these wildcards. This is what this PR does.

This is part of me splitting up #119581 for ease of review.

r? @compiler-errors

It's incorrect because `CtorSet::split` returns a non-present constructor into `present` in one specific case: variable-length slices of an empty type. That's because empty constructors of arity 0 break the algorithm. This is a tricky corner case that's hard to do cleanly. The assert wasn't adding much anyway.

…trix

Nadrieril · 2024-01-07T18:30:24Z

We should get the same perf issue that #119581 got. It all comes from the commit that introduces the enum but I'm not sure exactly why.

@bors try @rust-timer queue

bors · 2024-01-07T18:31:33Z

⌛ Trying commit 1a3edc1 with merge c27d914...

…s, r=<try> Exhaustiveness: use an `Option` instead of allocating fictitious patterns In the process of exhaustiveness checking, `Matrix` stores a 2D array of patterns. Those are subpatterns of the patterns we were provided as input, _except_ sometimes we allocate some extra wildcard patterns to fill a hole during specialization. Morally though, we could store `Option<&'p DeconstructedPat>` in the matrix, where `None` signifies a wildcard. That way we'd only have "real" patterns in the matrix and we wouldn't need the arena to allocate these wildcards. This is what this PR does. This is part of me splitting up rust-lang#119581 for ease of review. r? `@compiler-errors`

bors · 2024-01-07T19:57:46Z

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

rust-timer · 2024-01-07T21:28:42Z

Finished benchmarking commit (c27d914): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	3.8%	[3.5%, 4.1%]	6
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	3

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)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	3.5%	[2.3%, 4.2%]	3
Improvements ✅ (primary)	-3.7%	[-4.2%, -3.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.8%	[-4.2%, 2.0%]	3

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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 665.982s -> 665.909s (-0.01%)
Artifact size: 308.42 MiB -> 308.39 MiB (-0.01%)

Nadrieril · 2024-01-07T23:05:19Z

Sadly the perf regression isn't quite compensated by the improvement in #119667. Once again wishing match-stress was less sensitive.

compiler-errors

could questions

compiler/rustc_pattern_analysis/src/usefulness.rs

compiler/rustc_pattern_analysis/src/pat.rs

compiler-errors · 2024-01-09T02:04:15Z

compiler/rustc_pattern_analysis/src/lints.rs

-
- fn is_empty(&self) -> bool {
- self.patterns.is_empty()
+ fn expand_and_push(&mut self, pat: PatOrWild<'p, RustcMatchCheckCtxt<'p, 'tcx>>) {


kind of a shame this is duplicated w/ fn expand_and_push on Matrix.

They do have a common shape, but I don't see how to unify them more...

(this one pushes individual patterns and skips wildcards, the Matrix one pushes rows and doesn't skip wildcards)

oh; then it extra sucks that they are named identically. it would be nice if these methods had more doc comments, lol

compiler-errors · 2024-01-09T02:15:55Z

some qeustions

@rustbot author

Nadrieril · 2024-01-09T15:35:32Z

@rustbot ready

compiler-errors

r=me

Nadrieril · 2024-01-09T15:47:24Z

Ty!

@bors r=compiler-errors

bors · 2024-01-09T15:47:26Z

📌 Commit 4a1889e has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-01-11T00:04:12Z

⌛ Testing commit 4a1889e with merge 0a89233...

bors · 2024-01-11T02:09:22Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0a89233 to master...

rust-timer · 2024-01-11T03:22:56Z

Finished benchmarking commit (0a89233): 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)	3.8%	[3.6%, 4.1%]	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)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-1.5%	[-1.5%, -1.5%]	1
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Cycles

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

Binary size

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

Bootstrap: 667.319s -> 667.616s (0.04%)
Artifact size: 308.43 MiB -> 308.45 MiB (0.01%)

…-errors Exhaustiveness: remove the need for arena-allocation within the algorithm After rust-lang#119688, exhaustiveness checking doesn't need access to the arena anymore. This simplifies the lifetime story and makes it compile on stable without the extra dependency. r? `@compiler-errors`

pnkfelix · 2024-01-16T15:57:21Z

This impacted the 6 variants of match-stress {incr-full,full} x {check,debug,opt}
I think the impact on match-stress was probably well-anticipated, and within a reasonable range for a stress-test benchmark.
Note that Exhaustiveness: use an Option instead of allocating fictitious patterns #119688 was a precursor to some further cleanup code (namely to remove the use of a local-arena within exhaustiveness checking).
Marking as triaged.

@rustbot label: +perf-regression-triaged

Exhaustiveness: remove the need for arena-allocation within the algorithm After rust-lang/rust#119688, exhaustiveness checking doesn't need access to the arena anymore. This simplifies the lifetime story and makes it compile on stable without the extra dependency. r? `@compiler-errors`

rustbot assigned compiler-errors Jan 7, 2024

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 Jan 7, 2024

Nadrieril added 2 commits January 7, 2024 16:45

Factor out pushing onto PatternColumn

4c23861

Nadrieril force-pushed the dont-alloc-custom-wildcards branch from 50ff730 to 8ff893f Compare January 7, 2024 15:46

Nadrieril added 2 commits January 7, 2024 19:20

Use special enum to represent algorithm-generated wildcards in the ma…

4ae2840

…trix

We only need the arity of the subtype list now

1a3edc1

Nadrieril force-pushed the dont-alloc-custom-wildcards branch from 8ff893f to 1a3edc1 Compare January 7, 2024 18:26

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 Jan 7, 2024

Nadrieril mentioned this pull request Jan 7, 2024

Exhaustiveness: remove the need for arena-allocation within the algorithm #119581

Merged

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 Jan 7, 2024

compiler-errors reviewed Jan 9, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2024

Nadrieril added 2 commits January 9, 2024 16:22

Avoid PatOrWild glob import

5c65e9f

Document the new expand_and_push method

4a1889e

Nadrieril force-pushed the dont-alloc-custom-wildcards branch from 26eff91 to 4a1889e Compare January 9, 2024 15:32

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 9, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2024

compiler-errors approved these changes Jan 9, 2024

View reviewed changes

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 9, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 11, 2024

bors merged commit 0a89233 into rust-lang:master Jan 11, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 11, 2024

bors mentioned this pull request Jan 11, 2024

Exhaustiveness: track overlapping ranges precisely #119396

Merged

Nadrieril deleted the dont-alloc-custom-wildcards branch January 11, 2024 08:25

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exhaustiveness: use an `Option` instead of allocating fictitious patterns #119688

Exhaustiveness: use an `Option` instead of allocating fictitious patterns #119688

Nadrieril commented Jan 7, 2024 •

edited

Loading

Nadrieril commented Jan 7, 2024

This comment has been minimized.

bors commented Jan 7, 2024

bors commented Jan 7, 2024

This comment has been minimized.

rust-timer commented Jan 7, 2024

Nadrieril commented Jan 7, 2024

compiler-errors left a comment

compiler-errors Jan 9, 2024

Nadrieril Jan 9, 2024

Nadrieril Jan 9, 2024

compiler-errors Jan 9, 2024

Nadrieril Jan 9, 2024

compiler-errors commented Jan 9, 2024

Nadrieril commented Jan 9, 2024

compiler-errors left a comment

Nadrieril commented Jan 9, 2024

bors commented Jan 9, 2024

bors commented Jan 11, 2024

bors commented Jan 11, 2024

rust-timer commented Jan 11, 2024

pnkfelix commented Jan 16, 2024

Exhaustiveness: use an Option instead of allocating fictitious patterns #119688

Exhaustiveness: use an Option instead of allocating fictitious patterns #119688

Conversation

Nadrieril commented Jan 7, 2024 • edited Loading

Nadrieril commented Jan 7, 2024

This comment has been minimized.

bors commented Jan 7, 2024

bors commented Jan 7, 2024

This comment has been minimized.

rust-timer commented Jan 7, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Nadrieril commented Jan 7, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Jan 9, 2024

Nadrieril commented Jan 9, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Nadrieril commented Jan 9, 2024

bors commented Jan 9, 2024

bors commented Jan 11, 2024

bors commented Jan 11, 2024

rust-timer commented Jan 11, 2024

Overall result: ❌ regressions - ACTION NEEDED

pnkfelix commented Jan 16, 2024

Exhaustiveness: use an `Option` instead of allocating fictitious patterns #119688

Exhaustiveness: use an `Option` instead of allocating fictitious patterns #119688

Nadrieril commented Jan 7, 2024 •

edited

Loading