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

alloc: avoid creating references in implementation of Rc/Arc #119691

Closed
wants to merge 1 commit into from
Closed

alloc: avoid creating references in implementation of Rc/Arc #119691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 23 additions & 14 deletions library/alloc/src/rc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1628,7 +1628,7 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {
// We are careful to *not* create a reference covering the "count" fields, as
// this would conflict with accesses to the reference counts (e.g. by `Weak`).
unsafe { &mut (*this.ptr.as_ptr()).value }
unsafe { &mut *ptr::addr_of_mut!((*this.ptr.as_ptr()).value) }
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 this created any unnecessary references, the addr_of_mut roundtrip seems pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you're right, but this does result in different MIR output.
I thought that they were different because the previous line created a reference to the entire struct, not just that one field

Copy link
Member

Choose a reason for hiding this comment

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

I thought that they were different because the previous line created a reference to the entire struct, not just that one field

IIUC, &mut (*ptr).field does not create a reference to the entire struct. See also the following lines in Weak::inner.

rust/library/alloc/src/sync.rs

Lines 2849 to 2852 in 87e1430

// We are careful to *not* create a reference covering the "data" field, as
// the field may be mutated concurrently (for example, if the last `Arc`
// is dropped, the data field will be dropped in-place).
Some(unsafe { WeakInner { strong: &(*ptr).strong, weak: &(*ptr).weak } })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that comment but I didn't quite trust it because it's apparently part of faulty code :) I guess I can close this tho

}

#[inline]
Expand Down Expand Up @@ -1885,10 +1885,10 @@ impl<T: ?Sized> Rc<T> {
// Initialize the RcBox
let inner = mem_to_rcbox(ptr.as_non_null_ptr().as_ptr());
unsafe {
debug_assert_eq!(Layout::for_value(&*inner), layout);
debug_assert_eq!(Layout::for_value_raw(inner), layout);

ptr::write(&mut (*inner).strong, Cell::new(1));
ptr::write(&mut (*inner).weak, Cell::new(1));
ptr::write(ptr::addr_of_mut!((*inner).strong), Cell::new(1));
ptr::write(ptr::addr_of_mut!((*inner).weak), Cell::new(1));
}

Ok(inner)
Expand All @@ -1902,7 +1902,7 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
// Allocate for the `RcBox<T>` using the given value.
unsafe {
Rc::<T>::allocate_for_layout(
Layout::for_value(&*ptr),
Layout::for_value_raw(ptr),
|layout| alloc.allocate(layout),
|mem| mem.with_metadata_of(ptr as *const RcBox<T>),
)
Expand All @@ -1918,7 +1918,7 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
// Copy value as bytes
ptr::copy_nonoverlapping(
&*src as *const T as *const u8,
&mut (*ptr).value as *mut _ as *mut u8,
ptr::addr_of_mut!((*ptr).value) as *mut u8,
value_size,
);

Expand Down Expand Up @@ -1952,7 +1952,11 @@ impl<T> Rc<[T]> {
unsafe fn copy_from_slice(v: &[T]) -> Rc<[T]> {
unsafe {
let ptr = Self::allocate_for_slice(v.len());
ptr::copy_nonoverlapping(v.as_ptr(), &mut (*ptr).value as *mut [T] as *mut T, v.len());
ptr::copy_nonoverlapping(
v.as_ptr(),
ptr::addr_of_mut!((*ptr).value) as *mut T,
v.len(),
);
Self::from_ptr(ptr)
}
}
Expand Down Expand Up @@ -1987,15 +1991,15 @@ impl<T> Rc<[T]> {
let ptr = Self::allocate_for_slice(len);

let mem = ptr as *mut _ as *mut u8;
let layout = Layout::for_value(&*ptr);
let layout = Layout::for_value_raw(ptr);

// Pointer to first element
let elems = &mut (*ptr).value as *mut [T] as *mut T;
let elems = ptr::addr_of_mut!((*ptr).value) as *mut T;

let mut guard = Guard { mem: NonNull::new_unchecked(mem), elems, layout, n_elems: 0 };

for (i, item) in iter.enumerate() {
ptr::write(elems.add(i), item);
for item in iter {
ptr::write(elems.add(guard.n_elems), item);
Comment on lines +2001 to +2002
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by simplification

guard.n_elems += 1;
}

Expand Down Expand Up @@ -2524,7 +2528,7 @@ impl<T, A: Allocator> From<Vec<T, A>> for Rc<[T], A> {
let (vec_ptr, len, cap, alloc) = v.into_raw_parts_with_alloc();

let rc_ptr = Self::allocate_for_slice_in(len, &alloc);
ptr::copy_nonoverlapping(vec_ptr, &mut (*rc_ptr).value as *mut [T] as *mut T, len);
ptr::copy_nonoverlapping(vec_ptr, ptr::addr_of_mut!((*rc_ptr).value) as *mut T, len);

// Create a `Vec<T, &A>` with length 0, to deallocate the buffer
// without dropping its contents or the allocator
Expand Down Expand Up @@ -3099,7 +3103,10 @@ impl<T: ?Sized, A: Allocator> Weak<T, A> {
// is dropped, the data field will be dropped in-place).
Some(unsafe {
let ptr = self.ptr.as_ptr();
WeakInner { strong: &(*ptr).strong, weak: &(*ptr).weak }
WeakInner {
strong: &*ptr::addr_of!((*ptr).strong),
weak: &*ptr::addr_of!((*ptr).weak),
}
})
}
}
Expand Down Expand Up @@ -3499,7 +3506,9 @@ impl<T> DerefMut for UniqueRc<T> {
// SAFETY: This pointer was allocated at creation time so we know it is valid. We know we
// have unique ownership and therefore it's safe to make a mutable reference because
// `UniqueRc` owns the only strong reference to itself.
unsafe { &mut (*self.ptr.as_ptr()).value }
// We are careful to *not* create a reference covering the "count" fields, as
// this would conflict with accesses to the weak reference count.
unsafe { &mut *ptr::addr_of_mut!((*self.ptr.as_ptr()).value) }
}
}

Expand Down
31 changes: 18 additions & 13 deletions library/alloc/src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,11 +1828,11 @@ impl<T: ?Sized> Arc<T> {
mem_to_arcinner: impl FnOnce(*mut u8) -> *mut ArcInner<T>,
) -> *mut ArcInner<T> {
let inner = mem_to_arcinner(ptr.as_non_null_ptr().as_ptr());
debug_assert_eq!(unsafe { Layout::for_value(&*inner) }, layout);
debug_assert_eq!(unsafe { Layout::for_value_raw(inner) }, layout);

unsafe {
ptr::write(&mut (*inner).strong, atomic::AtomicUsize::new(1));
ptr::write(&mut (*inner).weak, atomic::AtomicUsize::new(1));
ptr::write(ptr::addr_of_mut!((*inner).strong), atomic::AtomicUsize::new(1));
ptr::write(ptr::addr_of_mut!((*inner).weak), atomic::AtomicUsize::new(1));
}

inner
Expand All @@ -1847,7 +1847,7 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
// Allocate for the `ArcInner<T>` using the given value.
unsafe {
Arc::allocate_for_layout(
Layout::for_value(&*ptr),
Layout::for_value_raw(ptr),
|layout| alloc.allocate(layout),
|mem| mem.with_metadata_of(ptr as *const ArcInner<T>),
)
Expand All @@ -1863,7 +1863,7 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
// Copy value as bytes
ptr::copy_nonoverlapping(
&*src as *const T as *const u8,
&mut (*ptr).data as *mut _ as *mut u8,
ptr::addr_of_mut!((*ptr).data) as *mut u8,
value_size,
);

Expand Down Expand Up @@ -1898,7 +1898,7 @@ impl<T> Arc<[T]> {
unsafe {
let ptr = Self::allocate_for_slice(v.len());

ptr::copy_nonoverlapping(v.as_ptr(), &mut (*ptr).data as *mut [T] as *mut T, v.len());
ptr::copy_nonoverlapping(v.as_ptr(), ptr::addr_of_mut!((*ptr).data) as *mut T, v.len());

Self::from_ptr(ptr)
}
Expand Down Expand Up @@ -1934,15 +1934,15 @@ impl<T> Arc<[T]> {
let ptr = Self::allocate_for_slice(len);

let mem = ptr as *mut _ as *mut u8;
let layout = Layout::for_value(&*ptr);
let layout = Layout::for_value_raw(ptr);

// Pointer to first element
let elems = &mut (*ptr).data as *mut [T] as *mut T;
let elems = ptr::addr_of_mut!((*ptr).data) as *mut T;

let mut guard = Guard { mem: NonNull::new_unchecked(mem), elems, layout, n_elems: 0 };

for (i, item) in iter.enumerate() {
ptr::write(elems.add(i), item);
for item in iter {
ptr::write(elems.add(guard.n_elems), item);
guard.n_elems += 1;
}

Expand Down Expand Up @@ -2305,7 +2305,7 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {
// We are careful to *not* create a reference covering the "count" fields, as
// this would alias with concurrent access to the reference counts (e.g. by `Weak`).
unsafe { &mut (*this.ptr.as_ptr()).data }
unsafe { &mut *ptr::addr_of_mut!((*this.ptr.as_ptr()).data) }
}

/// Determine whether this is the unique reference (including weak refs) to
Expand Down Expand Up @@ -2849,7 +2849,12 @@ impl<T: ?Sized, A: Allocator> Weak<T, A> {
// We are careful to *not* create a reference covering the "data" field, as
// the field may be mutated concurrently (for example, if the last `Arc`
// is dropped, the data field will be dropped in-place).
Some(unsafe { WeakInner { strong: &(*ptr).strong, weak: &(*ptr).weak } })
Some(unsafe {
WeakInner {
strong: &*ptr::addr_of!((*ptr).strong),
weak: &*ptr::addr_of!((*ptr).weak),
}
})
}
}

Expand Down Expand Up @@ -3380,7 +3385,7 @@ impl<T, A: Allocator + Clone> From<Vec<T, A>> for Arc<[T], A> {
let (vec_ptr, len, cap, alloc) = v.into_raw_parts_with_alloc();

let rc_ptr = Self::allocate_for_slice_in(len, &alloc);
ptr::copy_nonoverlapping(vec_ptr, &mut (*rc_ptr).data as *mut [T] as *mut T, len);
ptr::copy_nonoverlapping(vec_ptr, ptr::addr_of_mut!((*rc_ptr).data) as *mut T, len);

// Create a `Vec<T, &A>` with length 0, to deallocate the buffer
// without dropping its contents or the allocator
Expand Down
Loading