Get rid of niche selection's dependence on fields's order

[adwinwhite]

Fixes #125630.
Use the optimal niche selection decided in univariant() rather than picking niche field manually.

r? @the8472

[the8472]

I don't think largest niche in the largest variant is necessarily optimal. There may be multiple largest variants. And niches in a smaller variant can still be useful, e.g. when used by an outer enum when nesting multiple into each other.

The linked issue could be solved by sorting the enum variant fields differently, without changing the niche-picking code.

[adwinwhite]

My wording is not precise.
The "optimal" actually means the better strategy in your PR.

I don't quite understand the relevance of niches in smaller variant. My commit still uses the same largest variant as before.
If my understanding is correct, niche_filling_layout picks a different field than the better strategy thus it doesn't have enough space to fit other variants.

[adwinwhite]

The field sorting is quite naive compared to what's implemented in univariant_biased, as it doesn't make use of the knowledge of the variant's layout to allow more space for fitting. I don't think simply sorting differently can change much unless we make it as complicated as univariant. Maybe there's something I missed?

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[RalfJung]

Thanks for adjusting the Miri test! The intention in that Miri test was that the add(0) would be changed to add(size-of-ptr) if the padding moves elsewhere. Maybe it would make sense to add a "let pad_offset = ...;" and "add(pad_offset)" to make that more clear.

[the8472]

I don't quite understand the relevance of niches in smaller variant. My commit still uses the same largest variant as before.
If my understanding is correct, niche_filling_layout picks a different field than the better strategy thus it doesn't have enough space to fit other variants.

I was thinking of different logic, not niche_filling_layout. Now that I have looked at the context you're correct.

The field sorting is quite naive compared to what's implemented in univariant_biased, as it doesn't make use of the knowledge of the variant's layout to allow more space for fitting.

For the non-prefixed enum layout it should actually be going through the same logic as univariant structs. It's calling self.univariant(v, repr, StructKind::AlwaysSized) a few lines up and the AlwaysSized goes through the elaborate sorting.

So yeah, the sorting was already fine. As you spotted, it wasn't making use of the already-optimized niche choice when multiple of the same size were available.

---

I think there would be ways to make the miri test more robust by inferring the position of the padding by enumerating all fields and the struct size. But that's probably overkill for an easily adjusted test. And the layout logic only changes every now and then.

---

LGTM. @bors r+

[bors]

📌 Commit 937b09b has been approved by the8472

It is now in the queue for this repository.

[the8472]

and niches can affect perf

@bors rollup=never

[bors]

⌛ Testing commit 937b09b with merge 902f295...

[bors]

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 902f295 to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

[adwinwhite]

Bors met race condition. It seems that a retry is needed.
@the8472

[rust-timer]

Finished benchmarking commit (902f295): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.5%	[0.2%, 0.9%]	3
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.6%, 0.2%]	2

Max RSS (memory usage)

Results (primary -0.6%, secondary -0.9%)

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.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	3
Improvements ✅ (primary)	-1.3%	[-1.4%, -1.2%]	2
Improvements ✅ (secondary)	-2.4%	[-3.0%, -1.5%]	3
All ❌✅ (primary)	-0.6%	[-1.4%, 0.8%]	3

Cycles

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

Binary size

Results (secondary 0.0%)

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.0%	[0.0%, 0.0%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 769.206s -> 769.235s (0.00%)
Artifact size: 341.23 MiB -> 341.28 MiB (0.01%)

[the8472]

@bors r+

[bors]

⌛ Testing commit 937b09b with merge 2b11f26...

[bors]

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 2b11f26 to master...