From dec507ea2cd5388888b3e1e34a0b1b5a11e00b04 Mon Sep 17 00:00:00 2001 From: ruriww <215561737+ruriww@users.noreply.github.com> Date: Wed, 2 Sep 2026 09:43:59 +0000 Subject: [PATCH 1/4] arc: improve codegen of drop --- library/alloc/src/sync.rs | 99 ++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 47 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 403a6563492a8..c0ad0dd29f668 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -2238,23 +2238,6 @@ impl Arc { unsafe { self.ptr.as_ref() } } - // Non-inlined part of `drop`. - #[inline(never)] - unsafe fn drop_slow(&mut self) { - // Drop the weak ref collectively held by all strong references when this - // variable goes out of scope. This ensures that the memory is deallocated - // even if the destructor of `T` panics. - // Take a reference to `self.alloc` instead of cloning because 1. it'll last long - // enough, and 2. you should be able to drop `Arc`s with unclonable allocators - let _weak = Weak { ptr: self.ptr, alloc: &self.alloc }; - - // Destroy the data at this time, even though we must not free the box - // allocation itself (there might still be weak pointers lying around). - // We cannot use `get_mut_unchecked` here, because `self.alloc` is borrowed. - // ignore-tidy-undocumented-unsafe - unsafe { ptr::drop_in_place(&mut (*self.ptr.as_ptr()).data) }; - } - /// Returns `true` if the two `Arc`s point to the same allocation in a vein similar to /// [`ptr::eq`]. This function ignores the metadata of `dyn Trait` pointers. /// @@ -2983,6 +2966,55 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { /// ``` #[inline] fn drop(&mut self) { + // Non-inlined part of `drop`. + // This function was moved locally since there is only one caller, + // and makes it easier to reason about the the outlined fence. + #[inline(never)] + unsafe fn drop_slow(&mut self) { + // This fence is needed to prevent reordering of use of the data and + // deletion of the data. Because it is marked `Release`, the decreasing + // of the reference count synchronizes with this `Acquire` fence. This + // means that use of the data happens before decreasing the reference + // count, which happens before this fence, which happens before the + // deletion of the data. + // + // As explained in the [Boost documentation][1], + // + // > It is important to enforce any possible access to the object in one + // > thread (through an existing reference) to *happen before* deleting + // > the object in a different thread. This is achieved by a "release" + // > operation after dropping a reference (any access to the object + // > through this reference must obviously happened before), and an + // > "acquire" operation before deleting the object. + // + // In particular, while the contents of an Arc are usually immutable, it's + // possible to have interior writes to something like a Mutex. Since a + // Mutex is not acquired when it is deleted, we can't rely on its + // synchronization logic to make writes in thread A visible to a destructor + // running in thread B. + // + // Also note that the Acquire fence here could probably be replaced with an + // Acquire load, which could improve performance in highly-contended + // situations. See [2]. + // + // [1]: (www.boost.org/doc/libs/1_55_0/doc/html/atomic/usage_examples.html) + // [2]: (https://github.com/rust-lang/rust/pull/41714) + acquire!(self.inner().strong); + + // Drop the weak ref collectively held by all strong references when this + // variable goes out of scope. This ensures that the memory is deallocated + // even if the destructor of `T` panics. + // Take a reference to `self.alloc` instead of cloning because 1. it'll last long + // enough, and 2. you should be able to drop `Arc`s with unclonable allocators + let _weak = Weak { ptr: self.ptr, alloc: &self.alloc }; + + // Destroy the data at this time, even though we must not free the box + // allocation itself (there might still be weak pointers lying around). + // We cannot use `get_mut_unchecked` here, because `self.alloc` is borrowed. + // ignore-tidy-undocumented-unsafe + unsafe { ptr::drop_in_place(&mut (*self.ptr.as_ptr()).data) }; + } + // Because `fetch_sub` is already atomic, we do not need to synchronize // with other threads unless we are going to delete the object. This // same logic applies to the below `fetch_sub` to the `weak` count. @@ -2990,36 +3022,6 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { return; } - // This fence is needed to prevent reordering of use of the data and - // deletion of the data. Because it is marked `Release`, the decreasing - // of the reference count synchronizes with this `Acquire` fence. This - // means that use of the data happens before decreasing the reference - // count, which happens before this fence, which happens before the - // deletion of the data. - // - // As explained in the [Boost documentation][1], - // - // > It is important to enforce any possible access to the object in one - // > thread (through an existing reference) to *happen before* deleting - // > the object in a different thread. This is achieved by a "release" - // > operation after dropping a reference (any access to the object - // > through this reference must obviously happened before), and an - // > "acquire" operation before deleting the object. - // - // In particular, while the contents of an Arc are usually immutable, it's - // possible to have interior writes to something like a Mutex. Since a - // Mutex is not acquired when it is deleted, we can't rely on its - // synchronization logic to make writes in thread A visible to a destructor - // running in thread B. - // - // Also note that the Acquire fence here could probably be replaced with an - // Acquire load, which could improve performance in highly-contended - // situations. See [2]. - // - // [1]: (www.boost.org/doc/libs/1_55_0/doc/html/atomic/usage_examples.html) - // [2]: (https://github.com/rust-lang/rust/pull/41714) - acquire!(self.inner().strong); - // Make sure we aren't trying to "drop" the shared static for empty slices // used by Default::default. debug_assert!( @@ -3028,6 +3030,9 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { Likely decrement_strong_count or from_raw were called too many times.", ); + // Note: don't mark `drop_slow` as `#[cold]`, that has other side effects + core::hint::cold_path(); + // ignore-tidy-undocumented-unsafe unsafe { self.drop_slow(); From 7cd5eaab285235b1254c35e16165374bd673cd52 Mon Sep 17 00:00:00 2001 From: ruriww <215561737+ruriww@users.noreply.github.com> Date: Wed, 2 Sep 2026 09:51:43 +0000 Subject: [PATCH 2/4] oopsies --- library/alloc/src/sync.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index c0ad0dd29f668..4954fa48329c2 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -2970,7 +2970,7 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { // This function was moved locally since there is only one caller, // and makes it easier to reason about the the outlined fence. #[inline(never)] - unsafe fn drop_slow(&mut self) { + unsafe fn drop_slow(this: &mut Arc) { // This fence is needed to prevent reordering of use of the data and // deletion of the data. Because it is marked `Release`, the decreasing // of the reference count synchronizes with this `Acquire` fence. This @@ -2999,20 +2999,20 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { // // [1]: (www.boost.org/doc/libs/1_55_0/doc/html/atomic/usage_examples.html) // [2]: (https://github.com/rust-lang/rust/pull/41714) - acquire!(self.inner().strong); + acquire!(this.inner().strong); // Drop the weak ref collectively held by all strong references when this // variable goes out of scope. This ensures that the memory is deallocated // even if the destructor of `T` panics. // Take a reference to `self.alloc` instead of cloning because 1. it'll last long // enough, and 2. you should be able to drop `Arc`s with unclonable allocators - let _weak = Weak { ptr: self.ptr, alloc: &self.alloc }; + let _weak = Weak { ptr: this.ptr, alloc: &this.alloc }; // Destroy the data at this time, even though we must not free the box // allocation itself (there might still be weak pointers lying around). // We cannot use `get_mut_unchecked` here, because `self.alloc` is borrowed. // ignore-tidy-undocumented-unsafe - unsafe { ptr::drop_in_place(&mut (*self.ptr.as_ptr()).data) }; + unsafe { ptr::drop_in_place(&mut (*this.ptr.as_ptr()).data) }; } // Because `fetch_sub` is already atomic, we do not need to synchronize @@ -3035,7 +3035,7 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { // ignore-tidy-undocumented-unsafe unsafe { - self.drop_slow(); + drop_slow(self); } } } From 674fde8cd68f0b5fd186df6292a23105700be6b9 Mon Sep 17 00:00:00 2001 From: ruriww <215561737+ruriww@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:19:20 +0000 Subject: [PATCH 3/4] remove the temporal comment --- library/alloc/src/sync.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 4954fa48329c2..816998d9eee2b 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -2967,8 +2967,6 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { #[inline] fn drop(&mut self) { // Non-inlined part of `drop`. - // This function was moved locally since there is only one caller, - // and makes it easier to reason about the the outlined fence. #[inline(never)] unsafe fn drop_slow(this: &mut Arc) { // This fence is needed to prevent reordering of use of the data and From abf388dfb23f0b93c77336417bbfec305d4f3957 Mon Sep 17 00:00:00 2001 From: ruriww <215561737+ruriww@users.noreply.github.com> Date: Fri, 4 Sep 2026 10:26:09 +0000 Subject: [PATCH 4/4] remove the cold hint --- library/alloc/src/sync.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 816998d9eee2b..cdfbfb6ba5bb0 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -3028,9 +3028,6 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { Likely decrement_strong_count or from_raw were called too many times.", ); - // Note: don't mark `drop_slow` as `#[cold]`, that has other side effects - core::hint::cold_path(); - // ignore-tidy-undocumented-unsafe unsafe { drop_slow(self);