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

Arc: remove unused allocation from Weak::new() #50357

Merged
merged 1 commit into from
Jun 29, 2018
Merged
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
56 changes: 36 additions & 20 deletions src/liballoc/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use core::borrow;
use core::fmt;
use core::cmp::Ordering;
use core::intrinsics::abort;
use core::mem::{self, align_of_val, size_of_val, uninitialized};
use core::mem::{self, align_of_val, size_of_val};
use core::ops::Deref;
use core::ops::CoerceUnsized;
use core::ptr::{self, NonNull};
Expand All @@ -43,6 +43,9 @@ use vec::Vec;
/// necessarily) at _exactly_ `MAX_REFCOUNT + 1` references.
const MAX_REFCOUNT: usize = (isize::MAX) as usize;

/// A sentinel value that is used for the pointer of `Weak::new()`.
const WEAK_EMPTY: usize = 1;

/// A thread-safe reference-counting pointer. 'Arc' stands for 'Atomically
/// Reference Counted'.
///
Expand Down Expand Up @@ -235,6 +238,10 @@ impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<Arc<U>> for Arc<T> {}
/// [`None`]: ../../std/option/enum.Option.html#variant.None
#[stable(feature = "arc_weak", since = "1.4.0")]
pub struct Weak<T: ?Sized> {
// This is a `NonNull` to allow optimizing the size of this type in enums,
// but it is actually not truly "non-null". A `Weak::new()` will set this
// to a sentinel value, instead of needing to allocate some space in the
// heap.
ptr: NonNull<ArcInner<T>>,
}

Expand Down Expand Up @@ -1011,8 +1018,8 @@ impl Arc<Any + Send + Sync> {
}

impl<T> Weak<T> {
/// Constructs a new `Weak<T>`, allocating memory for `T` without initializing
/// it. Calling [`upgrade`] on the return value always gives [`None`].
/// Constructs a new `Weak<T>`, without allocating any memory.
/// Calling [`upgrade`] on the return value always gives [`None`].
///
/// [`upgrade`]: struct.Weak.html#method.upgrade
/// [`None`]: ../../std/option/enum.Option.html#variant.None
Expand All @@ -1029,11 +1036,7 @@ impl<T> Weak<T> {
pub fn new() -> Weak<T> {
unsafe {
Weak {
ptr: Box::into_raw_non_null(box ArcInner {
strong: atomic::AtomicUsize::new(0),
weak: atomic::AtomicUsize::new(1),
data: uninitialized(),
}),
ptr: NonNull::new_unchecked(WEAK_EMPTY as *mut _),
Copy link
Member

Choose a reason for hiding this comment

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

😕 Just use NonNull::dangling() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That picks a value based on the alignment size of T. While RawVec can check cap if its pointer is actually null, Weak doesn't have that. So, to check, dangling would need to be called each time.

It's also not clear to me what having a never-used pointer to 8, or 16, etc would provide.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think calling dangling every time is a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be a problem, the value is constant for the input type.

It may avoid memory access errors in debuggers, or avoid tripping unaligned access warnings in tools like Valgrind (if that's a thing).

Copy link
Contributor

Choose a reason for hiding this comment

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

In #41064 we changed all dangling pointers to be aligned rather than 1. There is no reason for this case to be different.

dangling() optimizes to a constant, there is not issue with calling it many times.

}
}
}
Expand Down Expand Up @@ -1070,7 +1073,11 @@ impl<T: ?Sized> Weak<T> {
pub fn upgrade(&self) -> Option<Arc<T>> {
// We use a CAS loop to increment the strong count instead of a
// fetch_add because once the count hits 0 it must never be above 0.
let inner = self.inner();
let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY {
return None;
} else {
unsafe { self.ptr.as_ref() }
};

// Relaxed load because any write of 0 that we can observe
// leaves the field in a permanently zero state (so a
Expand All @@ -1092,17 +1099,15 @@ impl<T: ?Sized> Weak<T> {

// Relaxed is valid for the same reason it is on Arc's Clone impl
match inner.strong.compare_exchange_weak(n, n + 1, Relaxed, Relaxed) {
Ok(_) => return Some(Arc { ptr: self.ptr, phantom: PhantomData }),
Ok(_) => return Some(Arc {
// null checked above
ptr: self.ptr,
phantom: PhantomData,
}),
Err(old) => n = old,
}
}
}

#[inline]
fn inner(&self) -> &ArcInner<T> {
// See comments above for why this is "safe"
unsafe { self.ptr.as_ref() }
}
}

#[stable(feature = "arc_weak", since = "1.4.0")]
Expand All @@ -1120,11 +1125,16 @@ impl<T: ?Sized> Clone for Weak<T> {
/// ```
#[inline]
fn clone(&self) -> Weak<T> {
let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY {
return Weak { ptr: self.ptr };
} else {
unsafe { self.ptr.as_ref() }
};
// See comments in Arc::clone() for why this is relaxed. This can use a
// fetch_add (ignoring the lock) because the weak count is only locked
// where are *no other* weak pointers in existence. (So we can't be
// running this code in that case).
let old_size = self.inner().weak.fetch_add(1, Relaxed);
let old_size = inner.weak.fetch_add(1, Relaxed);

// See comments in Arc::clone() for why we do this (for mem::forget).
if old_size > MAX_REFCOUNT {
Expand All @@ -1139,8 +1149,8 @@ impl<T: ?Sized> Clone for Weak<T> {

#[stable(feature = "downgraded_weak", since = "1.10.0")]
impl<T> Default for Weak<T> {
/// Constructs a new `Weak<T>`, allocating memory for `T` without initializing
/// it. Calling [`upgrade`] on the return value always gives [`None`].
/// Constructs a new `Weak<T>`, without allocating memory.
/// Calling [`upgrade`] on the return value always gives [`None`].
///
/// [`upgrade`]: struct.Weak.html#method.upgrade
/// [`None`]: ../../std/option/enum.Option.html#variant.None
Expand Down Expand Up @@ -1193,7 +1203,13 @@ impl<T: ?Sized> Drop for Weak<T> {
// weak count can only be locked if there was precisely one weak ref,
// meaning that drop could only subsequently run ON that remaining weak
// ref, which can only happen after the lock is released.
if self.inner().weak.fetch_sub(1, Release) == 1 {
let inner = if self.ptr.as_ptr() as *const u8 as usize == WEAK_EMPTY {
return;
} else {
unsafe { self.ptr.as_ref() }
};

if inner.weak.fetch_sub(1, Release) == 1 {
atomic::fence(Acquire);
unsafe {
Global.dealloc(self.ptr.cast(), Layout::for_value(self.ptr.as_ref()))
Expand Down