Add `Option::as_`(`mut_`)`slice` by llogiq · Pull Request #105871 · rust-lang/rust

llogiq · 2022-12-18T14:57:41Z

This adds the following functions:

Option<T>::as_slice(&self) -> &[T]
Option<T>::as_mut_slice(&mut self) -> &[T]

The as_slice and as_mut_slice_mut functions benefit from an optimization that makes them completely branch-free. ~~Unfortunately, this optimization is not available on by-value Options, therefore the into_slice implementations use the plain match + slice::from_ref approach.~~

Note that the optimization's soundness hinges on the fact that either the niche optimization makes the offset of the Some(_) contents zero or the mempory layout of Option<T> is equal to that of Option<MaybeUninit<T>>.

The idea has been discussed on Zulip. Notably the idea for the as_slice_mut and `into_slice´ methods came from @cuviper and @Sp00ph hardened the optimization against niche-optimized Options.

The rust playground shows that the generated assembly of the optimized method is basically only a copy while the naive method generates code containing a test dx, dx on x86_64.

EDIT from reviewer: ACP is rust-lang/libs-team#150

rustbot · 2022-12-18T14:57:47Z

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2022-12-18T14:57:49Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

vacuus · 2022-12-18T16:37:48Z

Perhaps use the byte_offset_from and cast and whatnot methods on raw pointers (not familiar with the API myself, but I think it's the preferred style now).

llogiq · 2022-12-18T18:35:48Z

I tried that, but it didn't work; the compiler complained that *const Option<T> is not *const T.

llogiq · 2022-12-18T22:36:24Z

Given the problem of inference errors, I wonder if we should rename the mutable version of into_slice to into_mut_slice?

llogiq · 2022-12-19T08:43:30Z

I had to add #![feature(const_pointer_byte_offsets)] for using byte_offset in a const fn.

vacuus · 2022-12-19T14:28:33Z

You could also use wrapping_byte_offset in as_mut_slice

llogiq · 2022-12-19T15:00:47Z

Thank you, I had missed that one.

scottmcm

I'd really rather not take a linkchecker change here (unless there's a bug it can link to saying this is the best option for now), but other than that it looks good!

llogiq · 2023-02-28T20:27:47Z

@scottmcm I have reverted the linkchecker change and changed the offending links as per your suggestion, let's hope they work this time.

scottmcm · 2023-02-28T20:29:33Z

Thanks! I don't really know the exact intradoc link rules nor the markdown parsing ones, so I'm also just 🤞

This adds the following functions: * `Option<T>::as_slice(&self) -> &[T]` * `Option<T>::as_slice_mut(&mut self) -> &[T]` The `as_slice` and `as_slice_mut` functions benefit from an optimization that makes them completely branch-free. Note that the optimization's soundness hinges on the fact that either the niche optimization makes the offset of the `Some(_)` contents zero or the mempory layout of `Option<T>` is equal to that of `Option<MaybeUninit<T>>`.

scottmcm · 2023-03-01T06:17:34Z

Ah, glad that one worked!

@bors r+

bors · 2023-03-01T06:17:36Z

📌 Commit 41da875 has been approved by scottmcm

It is now in the queue for this repository.

Add `Option::as_`(`mut_`)`slice` This adds the following functions: * `Option<T>::as_slice(&self) -> &[T]` * `Option<T>::as_mut_slice(&mut self) -> &[T]` The `as_slice` and `as_mut_slice_mut` functions benefit from an optimization that makes them completely branch-free. ~~Unfortunately, this optimization is not available on by-value Options, therefore the `into_slice` implementations use the plain `match` + `slice::from_ref` approach.~~ Note that the optimization's soundness hinges on the fact that either the niche optimization makes the offset of the `Some(_)` contents zero or the mempory layout of `Option<T>` is equal to that of `Option<MaybeUninit<T>>`. The idea has been discussed on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Option.3A.3Aas_slice). Notably the idea for the `as_slice_mut` and `into_slice´ methods came from `@cuviper` and `@Sp00ph` hardened the optimization against niche-optimized Options. The [rust playground](https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=74f8e4239a19f454c183aaf7b4a969e0) shows that the generated assembly of the optimized method is basically only a copy while the naive method generates code containing a `test dx, dx` on x86_64. --- EDIT from reviewer: ACP is rust-lang/libs-team#150

Add `Option::as_`(`mut_`)`slice` This adds the following functions: * `Option<T>::as_slice(&self) -> &[T]` * `Option<T>::as_mut_slice(&mut self) -> &[T]` The `as_slice` and `as_mut_slice_mut` functions benefit from an optimization that makes them completely branch-free. ~~Unfortunately, this optimization is not available on by-value Options, therefore the `into_slice` implementations use the plain `match` + `slice::from_ref` approach.~~ Note that the optimization's soundness hinges on the fact that either the niche optimization makes the offset of the `Some(_)` contents zero or the mempory layout of `Option<T>` is equal to that of `Option<MaybeUninit<T>>`. The idea has been discussed on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Option.3A.3Aas_slice). Notably the idea for the `as_slice_mut` and `into_slice´ methods came from ``@cuviper`` and ``@Sp00ph`` hardened the optimization against niche-optimized Options. The [rust playground](https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=74f8e4239a19f454c183aaf7b4a969e0) shows that the generated assembly of the optimized method is basically only a copy while the naive method generates code containing a `test dx, dx` on x86_64. --- EDIT from reviewer: ACP is rust-lang/libs-team#150

bors · 2023-03-01T12:33:00Z

⌛ Testing commit 41da875 with merge 5423745...

bors · 2023-03-01T15:36:08Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 5423745 to master...

rust-timer · 2023-03-01T16:53:45Z

Finished benchmarking commit (5423745): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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)	3.5%	[3.5%, 3.5%]	1
Regressions ❌ (secondary)	2.0%	[1.9%, 2.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-3.4%, -1.7%]	3
All ❌✅ (primary)	3.5%	[3.5%, 3.5%]	1

Cycles

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

…trieb Use `Option::as_slice` where applicable After rust-lang#105871 introduced `Option::as_slice`, this PR uses it within the compiler. I found it interesting that all cases where `as_slice` could be used were done with different code before; so it seems the new API also has the benefit of being "the obvious solution" where before there was a mix of options, none clearly better than the rest.

llogiq added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 18, 2022

rustbot assigned m-ou-se Dec 18, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2022

ChayimFriedman2 reviewed Dec 18, 2022
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

llogiq force-pushed the option-as-slice branch 2 times, most recently from 37b5b52 to 4a52066Compare December 18, 2022 15:54

jyn514 reviewed Dec 18, 2022
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

llogiq force-pushed the option-as-slice branch from 4a52066 to 74b3edcCompare December 18, 2022 16:24

llogiq force-pushed the option-as-slice branch 2 times, most recently from 99cbfee to 9c6a7d5Compare December 18, 2022 21:53

scottmcm reviewed Dec 19, 2022
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

scottmcm reviewed Dec 19, 2022
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

scottmcm reviewed Dec 19, 2022
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

scottmcm added the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Dec 19, 2022

scottmcm assigned scottmcm and unassigned m-ou-se Dec 19, 2022

llogiq force-pushed the option-as-slice branch from 9c6a7d5 to bb671b8Compare December 19, 2022 08:00

This comment has been minimized.
Sign in to view

llogiq force-pushed the option-as-slice branch from bb671b8 to f6e39e2Compare December 19, 2022 08:42

llogiq force-pushed the option-as-slice branch from f6e39e2 to fbfb3e7Compare December 19, 2022 09:41

llogiq force-pushed the option-as-slice branch from fbfb3e7 to daff81aCompare December 19, 2022 14:37

scottmcm removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2023

llogiq changed the title ~~Add Option::as_slice(_mut) and ::into_slice~~Add Option::as_slice(_mut)Feb 27, 2023

llogiq changed the title ~~Add Option::as_slice(_mut)~~Add Option::as_(mut_)sliceFeb 28, 2023

scottmcm reviewed Feb 28, 2023
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

scottmcm reviewed Feb 28, 2023
View reviewed changes

Comment threadlibrary/core/src/option.rs Outdated

scottmcm requested changes Feb 28, 2023
View reviewed changes

This comment has been minimized.
Sign in to view

llogiq force-pushed the option-as-slice branch from d47b530 to 9f25458Compare February 28, 2023 20:23

llogiq force-pushed the option-as-slice branch from 9f25458 to 41da875Compare February 28, 2023 23:09

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2023

matthiaskrgr mentioned this pull request Mar 1, 2023
Rollup of 7 pull requests #108600
Closed

matthiaskrgr mentioned this pull request Mar 1, 2023
Rollup of 6 pull requests #108602
Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 1, 2023

bors merged commit 5423745 into rust-lang:masterMar 1, 2023

rustbot added this to the 1.69.0 milestone Mar 1, 2023

llogiq mentioned this pull request Mar 2, 2023
Use Option::as_slice where applicable #108678
Merged

llogiq deleted the option-as-slice branch March 10, 2023 13:33

ChaiTRex mentioned this pull request Feb 25, 2024
Add as_slice and as_mut_slice to Option #92411
Closed

Uh oh!

Uh oh!

Conversation

llogiq commented Dec 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 18, 2022

Uh oh!

rustbot commented Dec 18, 2022

Uh oh!

Uh oh!

Uh oh!

vacuus commented Dec 18, 2022

Uh oh!

llogiq commented Dec 18, 2022

Uh oh!

llogiq commented Dec 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

llogiq commented Dec 19, 2022

Uh oh!

vacuus commented Dec 19, 2022

Uh oh!

llogiq commented Dec 19, 2022

Uh oh!

Uh oh!

Uh oh!

scottmcm left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

llogiq commented Feb 28, 2023

Uh oh!

scottmcm commented Feb 28, 2023

Uh oh!

scottmcm commented Mar 1, 2023

Uh oh!

bors commented Mar 1, 2023

Uh oh!

bors commented Mar 1, 2023

Uh oh!

bors commented Mar 1, 2023

Uh oh!

rust-timer commented Mar 1, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

llogiq commented Dec 18, 2022•
edited
Loading

llogiq commented Dec 18, 2022•
edited
Loading