Skip to content

Relax the memory ordering on the implementation of UnsafeArc #12962

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

Closed
wants to merge 1 commit into from
Closed
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
36 changes: 29 additions & 7 deletions src/libstd/sync/arc.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
// Copyright 2013-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
@@ -26,7 +26,7 @@ use clone::Clone;
use kinds::Send;
use ops::Drop;
use ptr::RawPtr;
use sync::atomics::{AtomicUint, SeqCst, Relaxed, Acquire};
use sync::atomics::{fence, AtomicUint, Relaxed, Acquire, Release};
use vec;

/// An atomically reference counted pointer.
@@ -109,8 +109,16 @@ impl<T: Send> UnsafeArc<T> {
impl<T: Send> Clone for UnsafeArc<T> {
fn clone(&self) -> UnsafeArc<T> {
unsafe {
// This barrier might be unnecessary, but I'm not sure...
let old_count = (*self.data).count.fetch_add(1, Acquire);
// Using a relaxed ordering is alright here, as knowledge of the original reference
// prevents other threads from erroneously deleting the object.
//
// As explained in the [Boost documentation][1],
// Increasing the reference counter can always be done with memory_order_relaxed: New
// references to an object can only be formed from an existing reference, and passing
// an existing reference from one thread to another must already provide any required
// synchronization.
// [1]: (www.boost.org/doc/libs/1_55_0/doc/html/atomic/usage_examples.html)
let old_count = (*self.data).count.fetch_add(1, Relaxed);
// FIXME(#12049): this needs some sort of debug assertion
if cfg!(test) { assert!(old_count >= 1); }
return UnsafeArc { data: self.data };
@@ -127,12 +135,26 @@ impl<T> Drop for UnsafeArc<T>{
if self.data.is_null() {
return
}
// Must be acquire+release, not just release, to make sure this
// doesn't get reordered to after the unwrapper pointer load.
let old_count = (*self.data).count.fetch_sub(1, SeqCst);
// Because `fetch_sub` is already atomic, we do not need to synchronize with other
// threads unless we are going to delete the object.
let old_count = (*self.data).count.fetch_sub(1, Release);
// FIXME(#12049): this needs some sort of debug assertion
if cfg!(test) { assert!(old_count >= 1); }
if old_count == 1 {
// 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
// sychronizes with this `Acquire` fence. This means that use of the data happens
// before decreasing the refernce 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.
// [1]: (www.boost.org/doc/libs/1_55_0/doc/html/atomic/usage_examples.html)
fence(Acquire);
let _: ~ArcData<T> = cast::transmute(self.data);
}
}