Reland debug impl for fieldless enums with concatenated string without using discriminant_value

[panstromek]

View all comments

relands #155452
reverts #162164

This implementation avoids the problem in the original PR by using simple match instead of indexing by discriminant_value. This avoids the cost of many small strings, generates less MIR and reduces borrowck cost, but it doesn't avoid the match checking costs (obviously).

It uses slightly different strategy to avoid more MIR costs - it doesn't generate array of indexes. It maps the enum variant directly to the start index. Names are separated by space, so the outlined function figures out the end index based on that.

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6ff0c29 (6ff0c293700f750741ca037a07d54262de940377)
Base parent: eca445e (eca445e5ae4a6679cc27d3a09106ce245e13a5a6)

[rust-timer]

Finished benchmarking commit (6ff0c29): comparison URL.

Overall result: ❌ regressions - 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

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

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	1

Max RSS (memory usage)

Results (primary 0.8%, 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)	2.2%	[0.5%, 5.2%]	4
Regressions ❌ (secondary)	3.8%	[3.8%, 3.8%]	1
Improvements ✅ (primary)	-2.2%	[-2.4%, -2.0%]	2
Improvements ✅ (secondary)	-1.9%	[-2.4%, -1.5%]	2
All ❌✅ (primary)	0.8%	[-2.4%, 5.2%]	6

Cycles

Results (primary 1.0%, secondary 5.9%)

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

	mean	range	count
Regressions ❌ (primary)	1.5%	[0.4%, 3.5%]	7
Regressions ❌ (secondary)	5.9%	[5.9%, 5.9%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-0.7%, 3.5%]	9

Binary size

Results (primary 0.1%, secondary 4.2%)

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

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 1.7%]	39
Regressions ❌ (secondary)	4.2%	[0.0%, 13.7%]	21
Improvements ✅ (primary)	-0.4%	[-0.7%, -0.0%]	16
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.7%, 1.7%]	55

Bootstrap: 481.923s -> 481.101s (-0.17%)
Artifact size: 403.28 MiB -> 403.16 MiB (-0.03%)

[panstromek]

Replacing discriminant_value with match => variant_index sadly doesn't help much. The generated MIR looks somewhat better than the default match => &str variant (there's less code in each arms basic block), but without the O(n) reduction in basic blocks, it doesn't seem to be worth it.

[panstromek]

@bors try @rust-timer queue

[rust-bors]

💔 Test for 499edf9 failed: CI. Failed jobs:

• try - dist-aarch64-linux-quick (web logs, enhanced plaintext logs)
• try - dist-x86_64-linux-quick (web logs, enhanced plaintext logs)

[panstromek]

@bors try

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: e694fed (e694fede2bc447bdb13d910e69bcefa07ce7599b)
Base parent: 5ec56e3 (5ec56e3902656f2460259fc3ac0f6e2255f94417)

[rust-timer]

Finished benchmarking commit (e694fed): comparison URL.

Overall result: ✅ improvements - no action needed

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

@bors rollup=iffy rustc-perf
@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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.2%]	5
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	4
All ❌✅ (primary)	-0.4%	[-0.4%, -0.2%]	5

Max RSS (memory usage)

Results (primary -0.5%, secondary 4.8%)

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

	mean	range	count
Regressions ❌ (primary)	3.0%	[1.2%, 5.6%]	3
Regressions ❌ (secondary)	4.8%	[2.9%, 6.6%]	3
Improvements ✅ (primary)	-3.1%	[-4.2%, -2.2%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-4.2%, 5.6%]	7

Cycles

Results (primary 2.9%, secondary -1.1%)

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

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.9%, 2.9%]	1
Regressions ❌ (secondary)	2.2%	[2.2%, 2.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-5.8%, -2.8%]	2
All ❌✅ (primary)	2.9%	[2.9%, 2.9%]	1

Binary size

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

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-1.7%, -0.0%]	129
Improvements ✅ (secondary)	-0.1%	[-0.3%, -0.0%]	90
All ❌✅ (primary)	-0.3%	[-1.7%, 0.0%]	133

Bootstrap: 487.044s -> 491.679s (0.95%)
Artifact size: 406.45 MiB -> 406.31 MiB (-0.03%)

[panstromek]

@bors try @rust-timer queue priority=-5

[rust-bors]

☀️ Try build successful (CI)
Build commit: c609802 (c60980230dada66a471d2ad9d2318594583621e0)
Base parent: 6e4cba3 (6e4cba3380a6f9e1e4d7672e11e40372bf04e873)

[rust-timer]

Finished benchmarking commit (c609802): comparison URL.

Overall result: ✅ improvements - no action needed

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

@bors rollup=iffy rustc-perf
@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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	2
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	4
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	2

Max RSS (memory usage)

Results (primary -1.2%, secondary 4.4%)

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

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	5.8%	[2.4%, 10.5%]	6
Improvements ✅ (primary)	-1.8%	[-3.1%, -1.0%]	3
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	-1.2%	[-3.1%, 0.8%]	4

Cycles

Results (primary -2.1%, secondary 5.6%)

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)	8.5%	[3.0%, 11.4%]	4
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-6.2%	[-6.2%, -6.2%]	1
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Binary size

Results (primary -0.1%, secondary 0.2%)

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%, 0.3%]	99
Regressions ❌ (secondary)	0.2%	[0.0%, 0.3%]	149
Improvements ✅ (primary)	-0.6%	[-1.5%, -0.0%]	36
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-0.1%	[-1.5%, 0.3%]	135

Bootstrap: 488.311s -> 487.884s (-0.09%)
Artifact size: 407.22 MiB -> 406.39 MiB (-0.20%)

[panstromek]

This doesn't look as good as the previous run, I think we can keep the threshold on 5