[Experiment] Rewrite exhaustiveness in one pass #116042

Nadrieril · 2023-09-21T22:32:45Z

Arm reachability checking does a quadratic amount of work: for each arm we check if it is reachable given the arms above it. This feels wasteful since we often end up re-exploring the same cases when we check for exhaustiveness.

This PR is an attempt to check reachability at the same time as exhaustiveness. This opens the door to a bunch of code simplifications I'm very excited about. The main question is whether I can get actual performance gains out of this.

I had started the experiment in #111720 but I can't reopen it.

r? @ghost

Nadrieril · 2023-09-21T22:33:19Z

@bors try @rust-timer queue

bors · 2023-09-21T22:33:33Z

⌛ Trying commit b5728f1 with merge aa12554...

[Experiment] Rewrite exhaustiveness in one pass Arm reachability checking does a quadratic amount of work: for each arm we check if it is reachable given the arms above it. This feels wasteful since we often end up re-exploring the same cases when we check for exhaustiveness. This PR is an attempt to check reachability at the same time as exhaustiveness. This opens the door to a bunch of code simplifications I'm very excited about. The main question is whether I can get actual performance gains out of this. I had started the experiment in rust-lang#111720 but I can't reopen it. r? `@ghost`

bors · 2023-09-21T23:44:35Z

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

rust-timer · 2023-09-22T05:32:12Z

Finished benchmarking commit (aa12554): 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.8%	[0.6%, 1.3%]	7
Regressions ❌ (secondary)	6.3%	[6.0%, 6.7%]	6
Improvements ✅ (primary)	-1.1%	[-2.1%, -0.3%]	11
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-2.1%, 1.3%]	18

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.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	1.5%	[0.6%, 2.1%]	5
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-2.3%, 2.2%]	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
Regressions ❌ (secondary)	3.7%	[3.4%, 4.0%]	3
Improvements ✅ (primary)	-1.2%	[-1.4%, -1.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-1.4%, -1.0%]	3

Binary size

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

Bootstrap: 632.84s -> 633.392s (0.09%)
Artifact size: 317.62 MiB -> 317.56 MiB (-0.02%)

Nadrieril · 2023-09-22T18:01:05Z

Is this good enough? Should I clean it up and submit?

cjgillot · 2023-09-22T18:22:31Z

On perf only, (haven't looked at the code yet), the improvements and regressions roughly balance out, that's good.

I'm a bit worried about the 6% regression on the stress test though. Do you have an explanation for this one ? Or should it be balanced with the algorithm improvements?

Kobzol · 2023-09-22T18:31:00Z

The last run of the previous PR had +/- 5% cycle wins on the stress test. Maybe we should start where the last one ended? :)

lqd · 2023-09-22T18:43:32Z

Another prior run also looked great, with no icounts regressions and even better cycles #111720 (comment)

Nadrieril · 2023-09-23T14:24:00Z

Another prior run also looked great, with no icounts regressions and even better cycles #111720 (comment)

Unfortunately that commit was on a WIP that didn't correctly handle the omitted_patterns lint if I recall correctly

I'm a bit worried about the 6% regression on the stress test though. Do you have an explanation for this one ?

I wish I did, locally I measure a 10% improvement :'( This benchmark baffles me

The last run of the previous PR had +/- 5% cycle wins on the stress test. Maybe we should start where the last one ended? :)

Good point, I'll look into it

Nadrieril · 2023-09-29T01:53:32Z

This single commit causes a regression on match-stress-enum locally, which makes no sense at all. Let's check that, at this point I think this benchmark is haunted on my machine

@bors try @rust-timer queue

bors · 2023-09-29T01:54:41Z

⌛ Trying commit 1efc54c with merge dde0381...

[Experiment] Rewrite exhaustiveness in one pass Arm reachability checking does a quadratic amount of work: for each arm we check if it is reachable given the arms above it. This feels wasteful since we often end up re-exploring the same cases when we check for exhaustiveness. This PR is an attempt to check reachability at the same time as exhaustiveness. This opens the door to a bunch of code simplifications I'm very excited about. The main question is whether I can get actual performance gains out of this. I had started the experiment in rust-lang#111720 but I can't reopen it. r? `@ghost`

bors · 2023-09-29T03:06:41Z

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

rust-timer · 2023-09-29T04:52:25Z

Finished benchmarking commit (dde0381): 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)	9.2%	[8.6%, 9.9%]	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)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	2
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
Regressions ❌ (secondary)	4.4%	[4.3%, 4.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 631.219s -> 629.891s (-0.21%)
Artifact size: 317.31 MiB -> 317.21 MiB (-0.03%)

Nadrieril · 2023-09-29T14:41:04Z

Alright well, at least it's consistent with my local measurements, and I've explained the regression I had on the 40-something commits earlier. It's this: 1efc54c (#116042).

This regression however doesn't make any sense to me. This commit reduces the size of IntRange, and does no extra computation on non-error code paths. Plus match-stress-enum won't ever see an IntRange. It must be some layout machinery then? How could a smaller variant generate much worse code on that one benchmark? I really want to ignore it and move on x)

Kobzol · 2023-09-29T14:45:25Z

This is the cachegrind diff of the latest perf. run

604,061,067  ???:rustc_mir_build::thir::pattern::usefulness::is_useful::{closure#0}
 -1,551,859  ???:<rustc_parse::parser::Parser>::bump
  1,441,085  ???:<rustc_parse::parser::Parser>::parse_block_common
  1,061,248  ???:<rustc_query_system::ich::hcx::StableHashingContext as rustc_span::HashStableContext>::def_path_hash
   -893,206  ???:rustc_ast::visit::walk_enum_def::<rustc_lint::early::EarlyContextAndPass<rustc_lint::BuiltinCombinedEarlyLintPass>>
    806,224  ???:<rustc_lint::early::EarlyContextAndPass<rustc_lint::BuiltinCombinedEarlyLintPass> as rustc_ast::visit::Visitor>::visit_variant
    795,623  ???:<rustc_ast::ptr::P<rustc_ast::ast::Expr> as core::clone::Clone>::clone
   -742,328  ???:<rustc_ast::ast::Expr as core::clone::Clone>::clone
    657,211  ???:<rustc_lint::early::EarlyContextAndPass<rustc_lint::BuiltinCombinedEarlyLintPass> as rustc_ast::visit::Visitor>::visit_variant_data

600M more instructions in is_useful.

Nadrieril · 2023-09-29T18:20:06Z

I confirm it's purely a layout issue, and specifically I suspect a padding issue. Constructor is aligned to u128 and this commit makes its size not a multiple of the alignment anymore. Changing the size while staying a multiple of u128 does not make the regression appear.

Turns out for my librarification plans I have other layout changes planned that will make the size work, so I'll do these first

rust-timer · 2023-10-20T17:56:33Z

Finished benchmarking commit (214eab1): 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.3%	[0.3%, 0.4%]	2
Regressions ❌ (secondary)	1.5%	[1.4%, 1.7%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	3
All ❌✅ (primary)	0.3%	[0.3%, 0.4%]	2

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.3%	[1.1%, 3.9%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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: 630.107s -> 630.022s (-0.01%)
Artifact size: 304.01 MiB -> 303.98 MiB (-0.01%)

Nadrieril · 2023-11-04T03:45:09Z

Coming back to this after much distraction from never-patterns and friends.

@bors try @rust-timer queue

bors · 2023-11-04T03:46:22Z

⌛ Trying commit fc12224 with merge 6ce98c7...

[Experiment] Rewrite exhaustiveness in one pass Arm reachability checking does a quadratic amount of work: for each arm we check if it is reachable given the arms above it. This feels wasteful since we often end up re-exploring the same cases when we check for exhaustiveness. This PR is an attempt to check reachability at the same time as exhaustiveness. This opens the door to a bunch of code simplifications I'm very excited about. The main question is whether I can get actual performance gains out of this. I had started the experiment in rust-lang#111720 but I can't reopen it. r? `@ghost`

bors · 2023-11-04T05:09:36Z

☀️ Try build successful - checks-actions
Build commit: 6ce98c7 (6ce98c798ec959cba60aec9d5356cc6b42d767ec)

rust-timer · 2023-11-05T00:30:23Z

Finished benchmarking commit (6ce98c7): 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)	1.3%	[1.3%, 1.4%]	6
Improvements ✅ (primary)	-3.8%	[-23.2%, -0.3%]	13
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-3.8%	[-23.2%, -0.3%]	13

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)	0.9%	[0.6%, 1.1%]	3
Improvements ✅ (primary)	-0.8%	[-1.1%, -0.5%]	3
Improvements ✅ (secondary)	-2.5%	[-5.3%, -0.5%]	7
All ❌✅ (primary)	-0.5%	[-1.1%, 0.5%]	4

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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.3%	[-19.2%, -0.5%]	7
Improvements ✅ (secondary)	-1.9%	[-2.1%, -1.7%]	2
All ❌✅ (primary)	-4.5%	[-19.2%, 0.7%]	8

Binary size

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

Bootstrap: 635.352s -> 635.43s (0.01%)
Artifact size: 304.45 MiB -> 304.62 MiB (0.06%)

Nadrieril · 2023-11-05T00:31:45Z

Pretty great on cycles :3

Nadrieril · 2023-11-05T14:42:46Z

Attempting a for real PR for this here: #117611

Dylan-DPC · 2023-11-09T13:54:36Z

Closing this as #117611 is now created

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 Sep 21, 2023

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 Sep 21, 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 Sep 22, 2023

Nadrieril force-pushed the linear-pass-take-2 branch from b5728f1 to 1efc54c Compare September 29, 2023 01:51

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

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

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 Oct 20, 2023

Nadrieril force-pushed the linear-pass-take-2 branch from 1d158a8 to 5883018 Compare November 4, 2023 03: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 Nov 4, 2023

This comment has been minimized.

Sign in to view

Add some tests

c6e3543

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

Nadrieril force-pushed the linear-pass-take-2 branch from fc12224 to 9bfb7d9 Compare November 5, 2023 00:56

Nadrieril added 4 commits November 5, 2023 02:03

Tweak lower_pattern

97f94a6

Abstract over the list of WitnessStacks

8ad52ed

Respect split invariants for Opaques

c07c143

Keep rows with guards in the matrix

ae77d68

This comment has been minimized.

Sign in to view

Fully rework the algorithm and its explanation

d4f08e7

Nadrieril force-pushed the linear-pass-take-2 branch from 9bfb7d9 to d4f08e7 Compare November 5, 2023 01:48

Dylan-DPC closed this Nov 9, 2023

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

Nadrieril deleted the linear-pass-take-2 branch November 9, 2023 20:09

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Experiment] Rewrite exhaustiveness in one pass #116042

[Experiment] Rewrite exhaustiveness in one pass #116042

Nadrieril commented Sep 21, 2023

Nadrieril commented Sep 21, 2023

This comment has been minimized.

bors commented Sep 21, 2023

bors commented Sep 21, 2023

This comment has been minimized.

rust-timer commented Sep 22, 2023

Nadrieril commented Sep 22, 2023

cjgillot commented Sep 22, 2023

Kobzol commented Sep 22, 2023 •

edited

Loading

lqd commented Sep 22, 2023

Nadrieril commented Sep 23, 2023

Nadrieril commented Sep 29, 2023

This comment has been minimized.

bors commented Sep 29, 2023

bors commented Sep 29, 2023

This comment has been minimized.

rust-timer commented Sep 29, 2023

Nadrieril commented Sep 29, 2023 •

edited

Loading

Kobzol commented Sep 29, 2023 •

edited

Loading

Nadrieril commented Sep 29, 2023

This comment has been minimized.

rust-timer commented Oct 20, 2023

Nadrieril commented Nov 4, 2023

This comment has been minimized.

bors commented Nov 4, 2023

This comment has been minimized.

bors commented Nov 4, 2023

This comment has been minimized.

rust-timer commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

This comment has been minimized.

Nadrieril commented Nov 5, 2023

Dylan-DPC commented Nov 9, 2023

[Experiment] Rewrite exhaustiveness in one pass #116042

[Experiment] Rewrite exhaustiveness in one pass #116042

Conversation

Nadrieril commented Sep 21, 2023

Nadrieril commented Sep 21, 2023

This comment has been minimized.

bors commented Sep 21, 2023

bors commented Sep 21, 2023

This comment has been minimized.

rust-timer commented Sep 22, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Nadrieril commented Sep 22, 2023

cjgillot commented Sep 22, 2023

Kobzol commented Sep 22, 2023 • edited Loading

lqd commented Sep 22, 2023

Nadrieril commented Sep 23, 2023

Nadrieril commented Sep 29, 2023

This comment has been minimized.

bors commented Sep 29, 2023

bors commented Sep 29, 2023

This comment has been minimized.

rust-timer commented Sep 29, 2023

Overall result: ❌ regressions - ACTION NEEDED

Nadrieril commented Sep 29, 2023 • edited Loading

Kobzol commented Sep 29, 2023 • edited Loading

Nadrieril commented Sep 29, 2023

This comment has been minimized.

rust-timer commented Oct 20, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Nadrieril commented Nov 4, 2023

This comment has been minimized.

bors commented Nov 4, 2023

This comment has been minimized.

bors commented Nov 4, 2023

This comment has been minimized.

rust-timer commented Nov 5, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Nadrieril commented Nov 5, 2023

This comment has been minimized.

Nadrieril commented Nov 5, 2023

Dylan-DPC commented Nov 9, 2023

Kobzol commented Sep 22, 2023 •

edited

Loading

Nadrieril commented Sep 29, 2023 •

edited

Loading

Kobzol commented Sep 29, 2023 •

edited

Loading