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

Replace deprecated compare_and_swap with compare_exchange #617

Merged
merged 1 commit into from
Dec 24, 2020
Merged
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
4 changes: 2 additions & 2 deletions crossbeam-channel/src/flavors/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,8 @@ impl<T> Channel<T> {
if self
.tail
.block
.compare_and_swap(block, new, Ordering::Release)
== block
.compare_exchange(block, new, Ordering::Release, Ordering::Relaxed)
.is_ok()
{
self.head.block.store(new, Ordering::Release);
block = new;
Expand Down
30 changes: 24 additions & 6 deletions crossbeam-epoch/src/epoch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,31 @@ impl AtomicEpoch {

/// Stores a value into the atomic epoch if the current value is the same as `current`.
///
/// The return value is always the previous value. If it is equal to `current`, then the value
/// is updated.
/// The return value is a result indicating whether the new value was written and containing
/// the previous value. On success this value is guaranteed to be equal to `current`.
///
/// The `Ordering` argument describes the memory ordering of this operation.
/// `compare_exchange` takes two `Ordering` arguments to describe the memory
/// ordering of this operation. `success` describes the required ordering for the
/// read-modify-write operation that takes place if the comparison with `current` succeeds.
/// `failure` describes the required ordering for the load operation that takes place when
/// the comparison fails. Using `Acquire` as success ordering makes the store part
/// of this operation `Relaxed`, and using `Release` makes the successful load
/// `Relaxed`. The failure ordering can only be `SeqCst`, `Acquire` or `Relaxed`
/// and must be equivalent to or weaker than the success ordering.
#[inline]
pub fn compare_and_swap(&self, current: Epoch, new: Epoch, ord: Ordering) -> Epoch {
let data = self.data.compare_and_swap(current.data, new.data, ord);
Epoch { data }
pub fn compare_exchange(
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This is marked as pub, but this is a function used only within the crate and not exposed, so this change does not break the public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • By "exposed", you mean it's not part of the public API?
  • Even if it is public, I vote for chaging like you suggested. We're before 1.0 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

  • By "exposed", you mean it's not part of the public API?

Yes, this function and AtomicEpoch that implements this function is not part of the public API: https://docs.rs/crossbeam-epoch/0.9.1/crossbeam_epoch/#structs

&self,
current: Epoch,
new: Epoch,
success: Ordering,
failure: Ordering,
) -> Result<Epoch, Epoch> {
match self
.data
.compare_exchange(current.data, new.data, success, failure)
{
Ok(data) => Ok(Epoch { data }),
Err(data) => Err(Epoch { data }),
}
}
}
18 changes: 12 additions & 6 deletions crossbeam-epoch/src/internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,10 @@ pub struct Local {
// https://github.com/crossbeam-rs/crossbeam/issues/551
#[test]
fn local_size() {
assert!(core::mem::size_of::<Local>() <= 2048, "An allocation of `Local` should be <= 2048 bytes.");
assert!(
core::mem::size_of::<Local>() <= 2048,
"An allocation of `Local` should be <= 2048 bytes."
);
}

impl Local {
Expand Down Expand Up @@ -468,7 +471,7 @@ impl Local {
// a `SeqCst` fence.
//
// 1. `atomic::fence(SeqCst)`, which compiles into a `mfence` instruction.
// 2. `_.compare_and_swap(_, _, SeqCst)`, which compiles into a `lock cmpxchg`
// 2. `_.compare_exchange(_, _, SeqCst, SeqCst)`, which compiles into a `lock cmpxchg`
// instruction.
//
// Both instructions have the effect of a full barrier, but benchmarks have shown
Expand All @@ -478,10 +481,13 @@ impl Local {
// works fine. Using inline assembly would be a viable (and correct) alternative,
// but alas, that is not possible on stable Rust.
let current = Epoch::starting();
let previous = self
.epoch
.compare_and_swap(current, new_epoch, Ordering::SeqCst);
debug_assert_eq!(current, previous, "participant was expected to be unpinned");
let res = self.epoch.compare_exchange(
current,
new_epoch,
Ordering::SeqCst,
Ordering::SeqCst,
);
debug_assert!(res.is_ok(), "participant was expected to be unpinned");
// We add a compiler fence to make it less likely for LLVM to do something wrong
// here. Formally, this is not enough to get rid of data races; practically,
// it should go a long way.
Expand Down
4 changes: 2 additions & 2 deletions crossbeam-queue/src/seg_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ impl<T> SegQueue<T> {
if self
.tail
.block
.compare_and_swap(block, new, Ordering::Release)
== block
.compare_exchange(block, new, Ordering::Release, Ordering::Relaxed)
.is_ok()
{
self.head.block.store(new, Ordering::Release);
block = new;
Expand Down
4 changes: 2 additions & 2 deletions crossbeam-utils/src/backoff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const YIELD_LIMIT: u32 = 10;
/// let backoff = Backoff::new();
/// loop {
/// let val = a.load(SeqCst);
/// if a.compare_and_swap(val, val.wrapping_mul(b), SeqCst) == val {
/// if a.compare_exchange(val, val.wrapping_mul(b), SeqCst, SeqCst).is_ok() {
/// return val;
/// }
/// backoff.spin();
Expand Down Expand Up @@ -131,7 +131,7 @@ impl Backoff {
/// let backoff = Backoff::new();
/// loop {
/// let val = a.load(SeqCst);
/// if a.compare_and_swap(val, val.wrapping_mul(b), SeqCst) == val {
/// if a.compare_exchange(val, val.wrapping_mul(b), SeqCst, SeqCst).is_ok() {
/// return val;
/// }
/// backoff.spin();
Expand Down