experiment: match lowering: Change the column selection strategy in pick_test() to potentially reduce redundant tests

[jakubadamw]

At the moment, pick_test() always selects the first match pair of the first candidate. This could lead to suboptimal match trees when many arms share a value for a field that is not listed first in the pattern. For example, in match p { P { x: 1, y: 0 }, P { x: 2, y: 0 }, .. }, the compiler would switch on x first and then redundantly check y == 0 in every branch.

In this PR, pick_test() will iterate over the first candidate's match pairs and picks the one whose testable value is shared across the largest number of consecutive candidates. In the example above, y: 0 appears in all arms (consecutive count = 3), so we test y first, check it once, and then switch on x – producing a smaller match tree.

This is just an experiment, I expect the extra scan to have at least some impact on compile time for most programs. I suppose there’s a tension here between letting the user write match expressions with more control over what order of condition branches they compile to versus treating them as declarative queries where it’s the compiler’s job to find the best ordering of branches to fulfil it.

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir
• compiler, mir expanded to 69 candidates
• Random selection from 11 candidates

[Nadrieril]

r? me

curious to see the results!

[jakubadamw]

curious to see the results!

@Nadrieril, thanks! So I’ve just fixed the results by having the selection prefer left-most indices when there’s a tie around how many consecutive rows can be branched into at once, which means the behaviour is unchanged for probably most of patterns. Would you be able to initiate a perf run on this?

[jakubadamw]

@bors try @rust-timer queue

[rust-bors]

@jakubadamw: 🔑 Insufficient privileges: not in try users

[jakubadamw]

@Nadrieril, are you in power to initiate the perf run by any chance? 🙂 That’d be much appreciated.

[Nadrieril]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2d09f56 (2d09f56789a3419fa208a9080b5a5d87e7c96287, parent: 30d0309fa821f7a0984a9629e0d227ca3c0d2eda)

[rust-timer]

Finished benchmarking commit (2d09f56): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	1.3%	[0.3%, 2.3%]	2
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-0.4%, 2.5%]	2

Max RSS (memory usage)

Results (primary 2.8%, secondary 1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (primary 1.8%, secondary 2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

Results (primary 0.1%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 1.8%]	40
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	24
Improvements ✅ (primary)	-0.1%	[-0.5%, -0.0%]	9
Improvements ✅ (secondary)	-0.2%	[-0.5%, -0.0%]	2
All ❌✅ (primary)	0.1%	[-0.5%, 1.8%]	49

Bootstrap: 489.264s -> 489.811s (0.11%)
Artifact size: 395.30 MiB -> 395.36 MiB (0.01%)

[jakubadamw]

Oh well, at least we know. 🙂

@Nadrieril, thanks for your help, closing this.

[Nadrieril]

Wait, these are compile-time benches. It's not the worst diff I've seen, it could be worth it on runtime perf. I thoughts there were some runtime benches in rustc-perf but it seems they didn't run? Or are never ran? I'm not sure.

[fmease]

I think they ran. Go to tab Runtime and tick Show non-relevant results. I don't know the accuracy of these benches tho, nor their coverage.

[jakubadamw]

Thank you both. Looking at the description of the runtime benchmarks, it doesn't seem like many of them would even be stressing the match code en to the extend that this change would influence. The bootstrap results are more peculiar, but also rather noisy. regex_syntax and rustc_hir seem to be significant outliers for reasons unknown. The ideal benchmark would be a crater run on a bunch of third-party crates out in the wild, I suppose, but I am not yet familiar with how easy it would be to carry it out. I'll do some more local experiments, whilst perhaps the PR could be reopened for the time being as I had clearly hastened to close it.