[EXPERIMENT] Check if isolating Matrix::unspecialize dodges regressions

[Zalathar]

View all comments

• Successor to [EXPERIMENT] Check if isolating Matrix::unspecialize dodges regressions #161632
• See also [EXPERIMENT] RawBitSet experiments #153589

[Zalathar]

@bors try @rust-timer queue

[panstromek]

Maybe I was wrong in the original assesment and the culprit is a different insert call in match checking - there's another one in ConstructorSet::split:

rust/compiler/rustc_pattern_analysis/src/constructor.rs

Lines 1106 to 1108 in 05320a2

	for idx in seen.iter().filter_map(|c| c.as_variant()) {
	seen_set.insert(idx);
	}

This was pretty cold in cachegrind, so I didn't pay attention to it very much, but at this point it feels like whatever you do there's some spooky regression at a distance, so maybe it's not as innocent as it looks.

(and there's no other insert call in match checking as far as I know, including transitively called ones)

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0fd30f7 (0fd30f71aa360d59aa9283318317b761e764e41c)
Base parent: da51146 (da5114692c9ebe46b869488c5f34f92eb10b98c1)

[rust-timer]

Finished benchmarking commit (0fd30f7): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

Results (primary -0.3%, secondary -4.6%)

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

	mean	range	count
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-4.6%	[-4.6%, -4.6%]	1
All ❌✅ (primary)	-0.3%	[-2.6%, 2.0%]	2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 469.244s -> 469.33s (0.02%)
Artifact size: 400.19 MiB -> 400.28 MiB (0.02%)

[Zalathar]

Let's see if only perturbing ConstructorSet::split causes regressions:

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 362939d (362939d9b1e7906cedbecdee745c0ed9eec3a41d)
Base parent: 347a1db (347a1dbf428ab92f284e3afafacfa7eb46341a5d)

[rust-timer]

Finished benchmarking commit (362939d): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 7.2%)

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

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

Cycles

Results (primary -2.0%)

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

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

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 468.927s -> 469.784s (0.18%)
Artifact size: 400.24 MiB -> 400.28 MiB (0.01%)

[panstromek]

All these results are quite puzzling. So I guess the only remaining explanation is that the regression is not in match checking after all?

[panstromek]

btw. The commit benchmarked here #161647 (comment) does nothing (insert2 is just the exact same function as insert). Was that intentional?

[panstromek]

Given the above comment, I think the only combination wee haven't tried is this one: 7f86311 (It seems to me that this was the original intent of this PR)

I kicked off a perf run in #161496 (comment). If this one doesn't regress, it (most likely means) that the regression is not in match checking code.

[Zalathar]

btw. The commit benchmarked here #161647 (comment) does nothing (insert2 is just the exact same function as insert). Was that intentional?

Yes, one of my perf runs was trying to check whether even an identical duplicate of insert was enough to trigger regressions.

(This made more sense when we thought that particular call site was the culprit. Either way, it didn’t produce an interesting result.)

[Zalathar]

Here's another experiment: What happens if we switch to the simpler insert2 only in other data structures that use DenseBitSet internally? (patch)

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: bf63c84 (bf63c84c7d546f09b4132f98db0a82632642f994)
Base parent: 9bb55c8 (9bb55c8c865411b7d9dea6ff743e583d510d89f5)

[rust-timer]

Finished benchmarking commit (bf63c84): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -6.0%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-9.5%, -2.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-6.0%	[-9.5%, -2.5%]	2

Cycles

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

Binary size

Results (primary -0.0%, 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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	8
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	6
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	8

Bootstrap: 476.471s -> 471.361s (-1.07%)
Artifact size: 400.96 MiB -> 400.28 MiB (-0.17%)

[Zalathar]

This benchmark result is evidence that the sensitive call site is calling DenseBitSet::insert directly, not via some other bitset-based data structure.

[panstromek]

I kicked off a perf run in #161496 (comment). If this one doesn't regress, it (most likely means) that the regression is not in match checking code.

Also, this run came out clean so the call in unspecialize is not it.