Skip to content

Prefer -1 for None#155473

Open
scottmcm wants to merge 1 commit intorust-lang:mainfrom
scottmcm:tweak_niche_assignment
Open

Prefer -1 for None#155473
scottmcm wants to merge 1 commit intorust-lang:mainfrom
scottmcm:tweak_niche_assignment

Conversation

@scottmcm
Copy link
Copy Markdown
Member

@scottmcm scottmcm commented Apr 18, 2026

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 None::<bool> 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.)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2026
Comment on lines 2091 to 2092
// zero is unavailable because wrapping occurs
move_end(v)
Copy link
Copy Markdown
Member Author

@scottmcm scottmcm Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Apr 18, 2026
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 18, 2026
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the tweak_niche_assignment branch from 302d83d to a146c66 Compare April 18, 2026 06:26
@scottmcm
Copy link
Copy Markdown
Member Author

It helps to remember to git add all the changes 🤦

@rust-bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

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 `None::<bool>` and such.

Any enums with more than one niched value (so not `Result` nor `Option`) remain as they were before.
@scottmcm scottmcm force-pushed the tweak_niche_assignment branch from a146c66 to 09df883 Compare April 18, 2026 07:26
@scottmcm
Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Apr 18, 2026
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 18, 2026

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

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%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 18, 2026
@scottmcm
Copy link
Copy Markdown
Member Author

scottmcm commented Apr 18, 2026

Wow, those results are way better than I expected 😅

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

@scottmcm scottmcm marked this pull request as ready for review April 18, 2026 16:25
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 18, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 18, 2026

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

@chenyukang
Copy link
Copy Markdown
Member

seems a good perf improvement, lgtm, but need other's review.
@rustbot reroll

@rustbot rustbot assigned adwinwhite and unassigned chenyukang Apr 19, 2026
@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants