Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify SyncOnceCell's take and drop. #76640

Merged
merged 2 commits into from
Sep 14, 2020
Merged

Simplify SyncOnceCell's take and drop. #76640

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a261a2
Simplify SyncOnceCell's `take` and `drop`.
m-ou-se Sep 12, 2020
aa68aaa
Mark Once::new as #[inline].
m-ou-se Sep 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 13 additions & 26 deletions library/std/src/lazy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
cell::{Cell, UnsafeCell},
fmt,
marker::PhantomData,
mem::{self, MaybeUninit},
mem::MaybeUninit,
ops::{Deref, Drop},
panic::{RefUnwindSafe, UnwindSafe},
sync::Once,
Expand Down Expand Up @@ -316,13 +316,7 @@ impl<T> SyncOnceCell<T> {
/// ```
#[unstable(feature = "once_cell", issue = "74465")]
pub fn into_inner(mut self) -> Option<T> {
// SAFETY: Safe because we immediately free `self` without dropping
let inner = unsafe { self.take_inner() };

// Don't drop this `SyncOnceCell`. We just moved out one of the fields, but didn't set
// the state to uninitialized.
mem::forget(self);
inner
self.take()
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
}

/// Takes the value out of this `SyncOnceCell`, moving it back to an uninitialized state.
Expand All @@ -348,22 +342,12 @@ impl<T> SyncOnceCell<T> {
/// ```
#[unstable(feature = "once_cell", issue = "74465")]
pub fn take(&mut self) -> Option<T> {
mem::take(self).into_inner()
}

/// Takes the wrapped value out of a `SyncOnceCell`.
/// Afterwards the cell is no longer initialized.
///
/// Safety: The cell must now be free'd WITHOUT dropping. No other usages of the cell
/// are valid. Only used by `into_inner` and `drop`.
unsafe fn take_inner(&mut self) -> Option<T> {
Copy link
Member

Choose a reason for hiding this comment

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

👍 I was thinking about removing take_inner as well (its contract is just weird), but I didn't know about those useful MaybeUninit helpers!

Copy link
Member Author

Choose a reason for hiding this comment

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

That's because they didn't exist yet. :) I added assume_init_drop in #76484 just a few days ago.

// The mutable reference guarantees there are no other threads that can observe us
// taking out the wrapped value.
// Right after this function `self` is supposed to be freed, so it makes little sense
// to atomically set the state to uninitialized.
if self.is_initialized() {
let value = mem::replace(&mut self.value, UnsafeCell::new(MaybeUninit::uninit()));
Some(value.into_inner().assume_init())
self.once = Once::new();
// SAFETY: `self.value` is initialized and contains a valid `T`.
// `self.once` is reset, so `is_initialized()` will be false again
// which prevents the value from being read twice.
unsafe { Some((&mut *self.value.get()).assume_init_read()) }
Copy link
Member

Choose a reason for hiding this comment

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

So we leave some garbage bytes inside, but they are guarded by reseted Once? Nifty!

Copy link
Member

@RalfJung RalfJung Sep 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, previously this wrote MaybeUninit::uninit() which is equivalent to not writing anything, so we already left behind garbage before. (Just the compiler might find it easier this way as there are no uninit writes to eliminate.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The disassembly of this new version is slightly shorter. It seems to make one less copy. (Specifically for SyncOnceCell<String> at least.) But that's probably just the copy out of self.value into value.

Copy link
Member

Choose a reason for hiding this comment

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

It's also possible that the compiler just did not realize before that it was needlessly copying uninitialized memory.

} else {
None
}
Expand Down Expand Up @@ -416,9 +400,12 @@ impl<T> SyncOnceCell<T> {

unsafe impl<#[may_dangle] T> Drop for SyncOnceCell<T> {
fn drop(&mut self) {
// SAFETY: The cell is being dropped, so it can't be accessed again.
// We also don't touch the `T`, which validates our usage of #[may_dangle].
unsafe { self.take_inner() };
if self.is_initialized() {
// Safety: The cell is initialized and being dropped, so it can't
// be accessed again. We also don't touch the `T` other than
// dropping it, which validates our usage of #[may_dangle].
unsafe { (&mut *self.value.get()).assume_init_drop() };
}
}
}

Expand Down