Prefer -1 for None

[scottmcm]

View all comments

Currently we pick "weird" numbers like 1114112 for None::<char>. While that's not wrong, it's kinda unnatural -- a human wouldn't make that choice.

This PR instead picks -1 for thinge like None::<char> -- like clang's WEOF -- and such.

Any enums with more than one niched value (so not Result nor Option) remain as they were before. Also we continue to use 0 when that's possible -- -1 is only preferred when zero isn't possible.

---

Inspired when someone in discord posted an example like this https://rust.godbolt.org/z/W94s9qdYW and I thought it was odd that we're currently picking -9223372036854775808 to be the value to store to mark an Option<Vec<_>> as None. (Especially since that needs an 8-byte immediate on x64, and writing -1 is only a 4-byte immediate.)

[scottmcm]

Note that this arm just kinda gave up before; it never even considered whether putting it next to start might be better.

View changes since the review

[scottmcm]

@bors try @rust-timer queue

[scottmcm]

It helps to remember to git add all the changes 🤦

[scottmcm]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: ed76e05 (ed76e056cbb36453c64da4dd8ffccac4e093f819, parent: 2f201bccb3a7fb5a85b0fcfcc0a020a946d6d58a)

[rust-timer]

Finished benchmarking commit (ed76e05): 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)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	0.5%	[0.0%, 1.5%]	9
Improvements ✅ (primary)	-0.3%	[-2.9%, -0.2%]	86
Improvements ✅ (secondary)	-0.3%	[-1.8%, -0.0%]	62
All ❌✅ (primary)	-0.3%	[-2.9%, 0.3%]	87

Max RSS (memory usage)

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

Cycles

Results (primary -2.9%, 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
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-2.9%	[-3.0%, -2.9%]	2
Improvements ✅ (secondary)	-6.2%	[-7.6%, -3.4%]	3
All ❌✅ (primary)	-2.9%	[-3.0%, -2.9%]	2

Binary size

Results (primary -0.2%, secondary -0.4%)

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.3%	[0.0%, 0.6%]	2
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.1%]	5
Improvements ✅ (secondary)	-0.9%	[-2.1%, -0.3%]	3
All ❌✅ (primary)	-0.2%	[-0.6%, -0.1%]	5

Bootstrap: 491.618s -> 493.312s (0.34%)
Artifact size: 394.25 MiB -> 396.31 MiB (0.52%)

[scottmcm]

Wow, those results are way better than I expected 😅

Seems pretty clearly a net win to me.
@rustbot label: +perf-regression-triaged

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
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
• compiler expanded to 72 candidates
• Random selection from 18 candidates

[apiraino]

We are observing some breakage in a number of crates (see linked issues).
There is no layout guarantees so this is technically not a breaking change. This said, should we give some visibility about this in the release notes?

[scottmcm]

@apiraino Agreed, a compatibility note sounds like a good idea.

[BrianIAm]

Just a small note: 1114112 isn't an arbitrary "weird" number, it's 0x110000, the first value above the Unicode codespace (valid scalar values are 0x0000..=0x10FFFF), which makes it a natural niche for char specifically. It's the tightest possible niche that still lies outside the valid range.

That said, -1 is arguably more ergonomic in practice (smaller immediate on x86-64, matches WEOF convention, easier to spot in a debugger), so the change may still be the right call.

If the value does stay as -1, it might be worth a comment explaining why 0x110000 was the previous choice, so the next reader doesn't have to work it out. And if it stays as 0x110000, a comment like // 0x110000 - one past the end of valid Unicode scalar values would prevent the "weird number" confusion from recurring.

[scottmcm]

@BrianIAm To anyone editing the layout code, that's already pretty clear. The code is choosing between "pick next available value after or before the current range". If the enum is using -A..=B, we're either going to use -A-1 or B+1.

I had "weird" in scare-quotes because it was never an arbitrary choice (and it's "not wrong" as I also put there), just one where when you look at in in godbolt you kinda wish it was different, since it'd be easier to read and could pick better instructions.