Exhaustiveness: track overlapping ranges precisely #119396

Nadrieril · 2023-12-28T21:53:41Z

The overlapping_range_endpoints lint has false positives, e.g. #117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable.

r? @ghost

Nadrieril · 2023-12-28T21:54:13Z

@bors try @rust-timer try

bors · 2023-12-28T21:55:22Z

⌛ Trying commit 3330ee9 with merge cb98877...

…try> Experiment: track overlapping ranges precisely The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable. r? `@ghost`

bors · 2023-12-28T23:21:26Z

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

Nadrieril · 2023-12-29T07:10:00Z

@rust-timer queue

Nadrieril · 2023-12-29T07:30:35Z

@bors try @rust-timer queue

bors · 2023-12-29T07:31:45Z

⌛ Trying commit 80df6f9 with merge b58be7c...

…try> Experiment: track overlapping ranges precisely The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable. r? `@ghost`

bors · 2023-12-29T08:57:38Z

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

rust-timer · 2023-12-29T10:18:06Z

Finished benchmarking commit (b58be7c): comparison URL.

Overall result: ✅ improvements - no 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.

@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)	-	-	0
Improvements ✅ (primary)	-0.6%	[-1.5%, -0.2%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-1.5%, -0.2%]	9

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

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.5%	[0.4%, 0.5%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.7%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.7%, 0.5%]	4

Binary size

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

Bootstrap: 672.269s -> 668.673s (-0.53%)
Artifact size: 312.29 MiB -> 312.29 MiB (-0.00%)

Kobzol · 2023-12-29T10:20:26Z

This actually looks like a very nice perf. win, -25% spent in the check_match query, and bootstrap got improved by almost 4s (which is outside of noise threshold, usually noise is +/- 2s)!

Nadrieril · 2023-12-29T12:11:54Z

I was not expecting a perf win wtf :D It seems running the lint separately is worse than this extra tracking I have to do

Nadrieril · 2023-12-30T23:28:16Z

Let's check I didn't break perf while cleaning up, and then this will be ready!

@bors try @rust-timer queue

bors · 2023-12-30T23:29:26Z

⌛ Trying commit 2327f2e with merge 7ff489d...

…try> Experiment: track overlapping ranges precisely The `overlapping_range_endpoints` lint has false positives, e.g. rust-lang#117648. I expected that removing these false positives would have too much of a perf impact but never measured it. This PR is an experiment to see if the perf loss is manageable. r? `@ghost`

bors · 2023-12-31T00:56:51Z

☀️ Try build successful - checks-actions
Build commit: 7ff489d (7ff489d25b4a33a6727f602007a2871ae65682ec)

rust-timer · 2023-12-31T04:12:38Z

Finished benchmarking commit (7ff489d): comparison URL.

Overall result: ✅ improvements - no 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.

@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)	-	-	0
Improvements ✅ (primary)	-0.5%	[-1.5%, -0.2%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-1.5%, -0.2%]	10

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)	3.9%	[3.4%, 5.0%]	3
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-1.4%	[-2.3%, -1.0%]	4
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	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.7%	[3.1%, 4.3%]	4
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.562s -> 667.015s (-0.38%)
Artifact size: 311.76 MiB -> 311.71 MiB (-0.01%)

Nadrieril · 2023-12-31T09:50:57Z

r? compiler

@rustbot ready

bors · 2024-01-11T03:07:01Z

☔ The latest upstream changes (presumably #119688) made this pull request unmergeable. Please resolve the merge conflicts.

WaffleLapkin

Not gonna lie, I only understand like a half of this 😅 I wish I had time to dive into pattern analysis more, but alas.

r=me, with/out nits

WaffleLapkin · 2024-01-11T22:43:37Z

compiler/rustc_pattern_analysis/src/usefulness.rs

+ for mut new_row in row.expand_or_pat() {
+ new_row.intersects = BitSet::new_empty(self.rows.len());
 self.rows.push(new_row);
 }


IMO would be easier to read if it was written like

self .rows .extend( row .expand_or_pat() .zip(self.rows.len()..) .map(|(new_row, len)| Row { intersects: BitSet::new_empty(len), ..new_row }) );

(okay, not sure about the struct update syntax, but mutating the field here is slightly confusing) (also extend ftw)

I like that the two calls to push are copies of each other, makes it easier to track the invariant that the bitset always has the same length as the number of rows above this one.

WaffleLapkin · 2024-01-12T00:44:56Z

compiler/rustc_pattern_analysis/src/lints.rs

 cx: MatchCtxt<'a, 'p, 'tcx>,
 column: &PatternColumn<'p, 'tcx>,
+ overlapping_range_endpoints: &mut Vec<rustc::OverlappingRanges<'p, 'tcx>>,


Do you think it would be a good idea to use a impl FnMut or &mut dyn FnMut here? I don't think lint_overlapping_range_endpoints needs the whole collection allocated...

Performance is irrelevant for this so it's a stylistic choice. I personally prefer to be computing data than calling callbacks in terms of code legibility. Otherwise I'd have made that a method on TypeCx I guess

I think it'll depend a lot of what's possible on the rust-analyzer side as well. If it fits in TypeCx on their side too I might change to that

I don't know what I was thinking, you're quite right actually

WaffleLapkin · 2024-01-12T00:56:34Z

compiler/rustc_pattern_analysis/src/lib.rs

@@ -125,7 +125,9 @@ pub fn analyze_match<'p, 'tcx>(
 let pat_column = PatternColumn::new(arms);

 // Lint ranges that overlap on their endpoints, which is likely a mistake.
- lint_overlapping_range_endpoints(cx, &pat_column)?;
+ if !report.overlapping_range_endpoints.is_empty() {


Is this if important? lint_overlapping_range_endpoints is just a for loop, so for empty overlapping_range_endpoints it will do nothing anyway...

True but I have a general policy of helping both the reader and LLVM figure out that they can skip this most of the time

Agreed that it's logically redundant

Nadrieril · 2024-01-12T08:20:23Z

Thanks for the review!

@bors r=WaffleLapkin

bors · 2024-01-12T08:20:26Z

📌 Commit a24f4db has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2024-01-12T11:29:09Z

⌛ Testing commit a24f4db with merge 174e73a...

bors · 2024-01-12T13:26:42Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 174e73a to master...

rust-timer · 2024-01-12T15:20:28Z

Finished benchmarking commit (174e73a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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)	-	-	0
Improvements ✅ (primary)	-0.4%	[-1.7%, -0.2%]	32
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-1.7%, -0.2%]	32

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	2.4%	[0.8%, 5.5%]	6
Improvements ✅ (primary)	-5.5%	[-5.5%, -5.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-5.5%, 0.5%]	2

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.5%	[0.4%, 0.6%]	4
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-0.7%, 0.6%]	5

Binary size

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

Bootstrap: 665.086s -> 669.852s (0.72%)
Artifact size: 308.40 MiB -> 308.36 MiB (-0.01%)

…-errors Lint `overlapping_ranges_endpoints` directly instead of collecting into a Vec In rust-lang#119396 I was a bit silly: I was trying to avoid any lints being fired from within the exhaustiveness algorithm for some vague aesthetic/reusability reason that doesn't really hold. This PR fixes that: instead of passing a `&mut Vec` around I just added a method to the `TypeCx` trait. r? `@compiler-errors`

Rollup merge of rust-lang#120002 - Nadrieril:dont-collect, r=compiler-errors Lint `overlapping_ranges_endpoints` directly instead of collecting into a Vec In rust-lang#119396 I was a bit silly: I was trying to avoid any lints being fired from within the exhaustiveness algorithm for some vague aesthetic/reusability reason that doesn't really hold. This PR fixes that: instead of passing a `&mut Vec` around I just added a method to the `TypeCx` trait. r? `@compiler-errors`

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 28, 2023

Nadrieril mentioned this pull request Dec 28, 2023

overlapping_range_endpoints false positive #117648

Closed

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 29, 2023

Nadrieril force-pushed the intersection-tracking branch from 3330ee9 to 80df6f9 Compare December 29, 2023 07:28

This comment has been minimized.

Sign in to view

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

Nadrieril force-pushed the intersection-tracking branch from 80df6f9 to 2327f2e Compare December 30, 2023 22:43

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 30, 2023

This comment has been minimized.

Sign in to view

Nadrieril marked this pull request as ready for review December 31, 2023 09:50

rustbot assigned WaffleLapkin Dec 31, 2023

Nadrieril changed the title ~~Experiment: track overlapping ranges precisely~~ Exhaustiveness: track overlapping ranges precisely Dec 31, 2023

This comment was marked as off-topic.

Sign in to view

Nadrieril mentioned this pull request Jan 4, 2024

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

Merged

Nadrieril force-pushed the intersection-tracking branch from 2327f2e to 7931ee6 Compare January 7, 2024 19:26

Nadrieril mentioned this pull request Jan 7, 2024

Lint singleton gaps after exclusive ranges #118879

Merged

Nadrieril added 3 commits January 11, 2024 13:56

Track row intersections

89d01ba

Factor out collection of overlapping ranges

6f6ba25

Only lint ranges that really overlap

a24f4db

Nadrieril force-pushed the intersection-tracking branch from 7931ee6 to a24f4db Compare January 11, 2024 13:06

WaffleLapkin approved these changes Jan 12, 2024

View reviewed changes

WaffleLapkin 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 12, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2024

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

bors merged commit 174e73a into rust-lang:master Jan 12, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 12, 2024

Nadrieril deleted the intersection-tracking branch January 12, 2024 14:28

Nadrieril mentioned this pull request Jan 15, 2024

Lint overlapping_ranges_endpoints directly instead of collecting into a Vec #120002

Merged

Exhaustiveness: track overlapping ranges precisely #119396

Exhaustiveness: track overlapping ranges precisely #119396

Conversation

Nadrieril commented Dec 28, 2023

Nadrieril commented Dec 28, 2023

bors commented Dec 28, 2023

bors commented Dec 28, 2023

Nadrieril commented Dec 29, 2023

This comment has been minimized.

Nadrieril commented Dec 29, 2023

This comment has been minimized.

bors commented Dec 29, 2023

bors commented Dec 29, 2023

This comment has been minimized.

rust-timer commented Dec 29, 2023

Overall result: ✅ improvements - no action needed

Kobzol commented Dec 29, 2023

Nadrieril commented Dec 29, 2023

Nadrieril commented Dec 30, 2023

This comment has been minimized.

bors commented Dec 30, 2023

bors commented Dec 31, 2023

This comment has been minimized.

rust-timer commented Dec 31, 2023

Overall result: ✅ improvements - no action needed

Nadrieril commented Dec 31, 2023

This comment was marked as off-topic.

bors commented Jan 11, 2024

WaffleLapkin 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

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

Nadrieril commented Jan 12, 2024

bors commented Jan 12, 2024

bors commented Jan 12, 2024

bors commented Jan 12, 2024

rust-timer commented Jan 12, 2024

Overall result: ✅ improvements - no action needed