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.

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

Attempt to make Box::default() construct in-place + Box::new_in_place() #64062

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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: 38 additions & 1 deletion src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,43 @@ impl<T> Box<T> {
Box(ptr.cast().into())
}

/// Allocates memory on the heap *then* constructs `T` in-place.
///
/// If the allocation fails, the initialization closure won't be called; because the allocation
/// can fail, the optimizer often couldn't construct `T` in-place even in theory because having
/// the allocation happen first is an observable change if the code that creates the value has
/// side-effects such as being able to panic.
///
/// FIXME: This is intended to work via return-value-optimization, but due to compiler
/// limitations can't reliably do that yet.
#[inline(always)]
fn new_in_place(f: impl FnOnce() -> T) -> Box<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is semantically more like new_please_try_to_do_it_in_place_but_I_am_not_expecting_any_guarantee?
(Such a guarantee is not something we can reliably or would make on a language (not LLVM) level.)

Copy link
Contributor Author

@petertodd petertodd Sep 1, 2019

Choose a reason for hiding this comment

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

Point is, if we had guaranteed copy elision, I think this is as close as you could get with an API that looked something like this given Rust's semantics. Even with out-references it's not clear to me you could get semantics that nicely guarantee no copies in an easy way, as you could still mess up and wind up with a copy prior to writing it to the out-reference.

Like I said above, it's not clear to me this is actually a good idea. But it does seem novel. :)

edit: fixed thinko: we do have return-value-optimization; it's the guaranteed copy elision that we're missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we do not guarantee copy elision we'll need to at minimum call this something other than new_in_place which suggests that there actually is a guarantee. For example: new_hint_in_place.

It might be a good idea to just use this internally for now... let's see how this measures up in terms of perf for fn default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be a good idea to just use this internally for now

Yup, it is private right now, as I didn't want to give people false hope even in nightly. I'd suggest that we leave it that way until RVO is more reliable (specifically named-return-value-optimization if I understand the terminology right?).

let mut r: Box<mem::MaybeUninit<T>> = Box::new_uninit();
let uninit: &mut mem::MaybeUninit<T> = &mut *r;

unsafe {
// So why aren't we using ptr::write() here?
//
// For return-value-optimization to work, the compiler would have to call f with the
// pointer as the return value. But a pointer isn't guaranteed to actually point to
// valid memory: if the pointer was invalid, eg. due to being null, eliding the copy
// would change where the invalid memory access would occur, changing the behavior in
// an observable way.
//
// An invalid reference OTOH is undefined behavior, so the compiler is free to assume
// it is valid.
//
// Unfortunately, this optimization isn't actually implemented yet. Though if
// enough of f() can be inlined the compiler can generally elide the copy anyway.
//
// Finally, this is leak free because MaybeUninit::new() can't panic: either f()
// succesfully creates the value, and assume_init() is called, or f() panics, frees its
// resources, and r is deallocated as a Box<MaybeUninit<T>>
*uninit = mem::MaybeUninit::new(f());
r.assume_init()
}
}

/// Constructs a new `Pin<Box<T>>`. If `T` does not implement `Unpin`, then
/// `x` will be pinned in memory and unable to be moved.
#[stable(feature = "pin", since = "1.33.0")]
Expand Down Expand Up @@ -480,7 +517,7 @@ unsafe impl<#[may_dangle] T: ?Sized> Drop for Box<T> {
impl<T: Default> Default for Box<T> {
/// Creates a `Box<T>`, with the `Default` value for T.
fn default() -> Box<T> {
box Default::default()
Box::new_in_place(T::default)
}
}

Expand Down
14 changes: 13 additions & 1 deletion src/liballoc/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,18 @@ impl<T> Rc<T> {
}
}

#[inline(always)]
fn new_in_place(f: impl FnOnce() -> T) -> Rc<T> {
let mut r: Rc<mem::MaybeUninit<T>> = Rc::new_uninit();

unsafe {
let uninit: &mut mem::MaybeUninit<T> = Rc::get_mut_unchecked(&mut r);

*uninit = mem::MaybeUninit::new(f());
r.assume_init()
}
}

/// Constructs a new `Pin<Rc<T>>`. If `T` does not implement `Unpin`, then
/// `value` will be pinned in memory and unable to be moved.
#[stable(feature = "pin", since = "1.33.0")]
Expand Down Expand Up @@ -1146,7 +1158,7 @@ impl<T: Default> Default for Rc<T> {
/// ```
#[inline]
fn default() -> Rc<T> {
Rc::new(Default::default())
Rc::new_in_place(T::default)
}
}

Expand Down
14 changes: 13 additions & 1 deletion src/liballoc/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,18 @@ impl<T> Arc<T> {
}
}

#[inline(always)]
fn new_in_place(f: impl FnOnce() -> T) -> Arc<T> {
let mut r: Arc<mem::MaybeUninit<T>> = Arc::new_uninit();

unsafe {
let uninit: &mut mem::MaybeUninit<T> = Arc::get_mut_unchecked(&mut r);

*uninit = mem::MaybeUninit::new(f());
r.assume_init()
}
}

/// Constructs a new `Pin<Arc<T>>`. If `T` does not implement `Unpin`, then
/// `data` will be pinned in memory and unable to be moved.
#[stable(feature = "pin", since = "1.33.0")]
Expand Down Expand Up @@ -1933,7 +1945,7 @@ impl<T: Default> Default for Arc<T> {
/// assert_eq!(*x, 0);
/// ```
fn default() -> Arc<T> {
Arc::new(Default::default())
Arc::new_in_place(T::default)
}
}

Expand Down