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

Extend atomic compare_and_swap #1443

Merged
merged 4 commits into from Feb 18, 2016

Conversation

Projects
None yet
6 participants
@Amanieu
Contributor

Amanieu commented Jan 5, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 22, 2016

Member

For some history, we currently auto-calculate the second ordering when emitting an LLVM instruction for this, which was added in rust-lang/rust@30ff17f when upgrading LLVM and it happened to start requiring two orderings. I don't believe we ever revisited that this until just now! Also for reference, the LLVM documentation for the cmpxchg instruction is here. I do agree that it's kinda unfortunate that compare_and_swap is already taken (without the ability to define a failure ordering), but alas!

Alright, so with all that out of the way, here are some thoughts of mine:

  • We may want to decide whether to consider the existing compare_and_swap method deprecated or not. This may involve a literal #[rustc_deprecated], but it may not. If we do consider it deprecated, then we could avoid adding compare_and_swap_weak_explicit (wow that's a mouthful), and we could also consider renaming compare_and_swap entirely (for example compare_exchange like C++ does). The downside of this, however, is that compare_and_swap is clearly more ergonomic as it only requires one ordering and is perhaps easier to think about as well.
  • Could you add documentation to the RFC to indicate what happens if the two orderings are invalid? For example if the failure ordering is stronger than the success one that should be considered invalid. I suspect that a panic is sufficient here (as the orderings are almost always statically known) and this mirrors the fence function as well.
  • I agree that adding new functions is superior to adding more options to the Ordering enum which only make sense for compare_and_swap. This also has backwards-compatibility issues with adding variants to a stable enum
  • One other possibility for an alternative would be to have a totally separate enum for the "double ordering" methods. This enum could then have all possible combinations.
  • One further possibility would be to make the method generic over a trait to get the orderings, for example:
pub trait IntoTwoOrderings {
    pub fn into_two_orderings(self) -> (Ordering, Ordering);
}

impl IntoTwoOrderings for Ordering {
    pub fn into_two_orderings(self) -> (Ordering, Ordering) { (self, self) }
}

impl IntoTwoOrderings for (Ordering, Ordering) {
    pub fn into_two_orderings(self) -> (Ordering, Ordering) { self }
}

pub fn compare_and_swap<O: IntoTwoOrderings>(&self, expected: T, ordering: O) -> T;

Alright, now with all that out of the way, my personal opinion is that we should stick with this RFC basically as-is. I think that there's utility in keeping the one-ordering methods as they're somewhat easier to understand, and adding lots of methods won't really hurt the types.

That being said I'm not super certain about one alternative over the other, I'm just swaying a bit in that direction.

Member

alexcrichton commented Jan 22, 2016

For some history, we currently auto-calculate the second ordering when emitting an LLVM instruction for this, which was added in rust-lang/rust@30ff17f when upgrading LLVM and it happened to start requiring two orderings. I don't believe we ever revisited that this until just now! Also for reference, the LLVM documentation for the cmpxchg instruction is here. I do agree that it's kinda unfortunate that compare_and_swap is already taken (without the ability to define a failure ordering), but alas!

Alright, so with all that out of the way, here are some thoughts of mine:

  • We may want to decide whether to consider the existing compare_and_swap method deprecated or not. This may involve a literal #[rustc_deprecated], but it may not. If we do consider it deprecated, then we could avoid adding compare_and_swap_weak_explicit (wow that's a mouthful), and we could also consider renaming compare_and_swap entirely (for example compare_exchange like C++ does). The downside of this, however, is that compare_and_swap is clearly more ergonomic as it only requires one ordering and is perhaps easier to think about as well.
  • Could you add documentation to the RFC to indicate what happens if the two orderings are invalid? For example if the failure ordering is stronger than the success one that should be considered invalid. I suspect that a panic is sufficient here (as the orderings are almost always statically known) and this mirrors the fence function as well.
  • I agree that adding new functions is superior to adding more options to the Ordering enum which only make sense for compare_and_swap. This also has backwards-compatibility issues with adding variants to a stable enum
  • One other possibility for an alternative would be to have a totally separate enum for the "double ordering" methods. This enum could then have all possible combinations.
  • One further possibility would be to make the method generic over a trait to get the orderings, for example:
pub trait IntoTwoOrderings {
    pub fn into_two_orderings(self) -> (Ordering, Ordering);
}

impl IntoTwoOrderings for Ordering {
    pub fn into_two_orderings(self) -> (Ordering, Ordering) { (self, self) }
}

impl IntoTwoOrderings for (Ordering, Ordering) {
    pub fn into_two_orderings(self) -> (Ordering, Ordering) { self }
}

pub fn compare_and_swap<O: IntoTwoOrderings>(&self, expected: T, ordering: O) -> T;

Alright, now with all that out of the way, my personal opinion is that we should stick with this RFC basically as-is. I think that there's utility in keeping the one-ordering methods as they're somewhat easier to understand, and adding lots of methods won't really hurt the types.

That being said I'm not super certain about one alternative over the other, I'm just swaying a bit in that direction.

@Amanieu

This comment has been minimized.

Show comment
Hide comment
@Amanieu

Amanieu Jan 22, 2016

Contributor

I don't think adding hacks to extend the Ordering enum is very viable, especially since a separate function is going to be required anyways to preserve backward compatibility. We might as well just use two Ordering arguments in that case.

I disagree that a single Ordering parameter is more ergonomic: either you know what you are doing, in which case you know which memory orderings to use, or you just use SeqCst everywhere. Most C++ code uses both ordering parameters on compare_exchange, so lock-free code ported from C++ will most likely specify both ordering.

I'm actually starting to prefer the idea of simply deprecating compare_and_swap and replacing it with a compare_exchange that allows specifying both ordering parameters.

Contributor

Amanieu commented Jan 22, 2016

I don't think adding hacks to extend the Ordering enum is very viable, especially since a separate function is going to be required anyways to preserve backward compatibility. We might as well just use two Ordering arguments in that case.

I disagree that a single Ordering parameter is more ergonomic: either you know what you are doing, in which case you know which memory orderings to use, or you just use SeqCst everywhere. Most C++ code uses both ordering parameters on compare_exchange, so lock-free code ported from C++ will most likely specify both ordering.

I'm actually starting to prefer the idea of simply deprecating compare_and_swap and replacing it with a compare_exchange that allows specifying both ordering parameters.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 22, 2016

Member

In C++ though it looks like you've at least got the option of not specifying a second ordering? (I may be misunderstanding overloading...)

If it's idiomatic in C++, however, to always have two orderings then I'd be pretty ok pushing on deprecation + new names for the new functions

Member

alexcrichton commented Jan 22, 2016

In C++ though it looks like you've at least got the option of not specifying a second ordering? (I may be misunderstanding overloading...)

If it's idiomatic in C++, however, to always have two orderings then I'd be pretty ok pushing on deprecation + new names for the new functions

@Amanieu

This comment has been minimized.

Show comment
Hide comment
@Amanieu

Amanieu Jan 23, 2016

Contributor

In C++ though it looks like you've at least got the option of not specifying a second ordering? (I may be misunderstanding overloading...)

That is correct, C++ gives you 3 options: no ordering (defaults to seqcst), single ordering (failure defaults to the strongest possible) or both orderings.

If it's idiomatic in C++, however, to always have two orderings then I'd be pretty ok pushing on deprecation + new names for the new functions

To be precise, in C++ it is idiomatic to either specify both orderings or none at all. But I've never really seen any code that just specifies a single ordering. If you look at C11, it only provides forms that accept no orderings or both.

I've updated the RFC to propose deprecating compare_and_swap and replacing it with compare_exchange_strong and compare_exchange_weak.

Contributor

Amanieu commented Jan 23, 2016

In C++ though it looks like you've at least got the option of not specifying a second ordering? (I may be misunderstanding overloading...)

That is correct, C++ gives you 3 options: no ordering (defaults to seqcst), single ordering (failure defaults to the strongest possible) or both orderings.

If it's idiomatic in C++, however, to always have two orderings then I'd be pretty ok pushing on deprecation + new names for the new functions

To be precise, in C++ it is idiomatic to either specify both orderings or none at all. But I've never really seen any code that just specifies a single ordering. If you look at C11, it only provides forms that accept no orderings or both.

I've updated the RFC to propose deprecating compare_and_swap and replacing it with compare_exchange_strong and compare_exchange_weak.

@ranma42 ranma42 referenced this pull request Jan 23, 2016

Closed

Generic atomic #1474

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 24, 2016

Member

Thanks for the update! This is looking pretty good to me. I might slightly prefer compare_exchange to compare_exchange_strong, but that's just a minor bikeshed.

Member

alexcrichton commented Jan 24, 2016

Thanks for the update! This is looking pretty good to me. I might slightly prefer compare_exchange to compare_exchange_strong, but that's just a minor bikeshed.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 11, 2016

Member

🔔 This RFC is now entering its week-long final comment period 🔔

Note that this is deprecating the compare_and_swap function, so others may indeed have some opinions!


My own personal opinion here is that I would prefer the name compare_exchange over the name compare_exchange_strong, but otherwise looks good to me!

Member

alexcrichton commented Feb 11, 2016

🔔 This RFC is now entering its week-long final comment period 🔔

Note that this is deprecating the compare_and_swap function, so others may indeed have some opinions!


My own personal opinion here is that I would prefer the name compare_exchange over the name compare_exchange_strong, but otherwise looks good to me!

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Feb 11, 2016

Member

My own personal opinion here is that I would prefer the name compare_exchange over the name compare_exchange_strong, but otherwise looks good to me!

Same here.

Member

aturon commented Feb 11, 2016

My own personal opinion here is that I would prefer the name compare_exchange over the name compare_exchange_strong, but otherwise looks good to me!

Same here.

@briansmith

This comment has been minimized.

Show comment
Hide comment
@briansmith

briansmith Feb 13, 2016

Please use the ISO C11 and/or ISO C++ names and semantics whenever possible. Otherwise, it would be a complete mess to understand code where more than one language (Rust, C, C++) is being used to modify the same value using atomic primitives. The ISO C and C++ stuff isn't perfect, but people are working hard to get them to be closer to perfect and there's no need to reinvent the wheel or to generate confusion with different names or slightly different semantics for similarly-named things.

briansmith commented Feb 13, 2016

Please use the ISO C11 and/or ISO C++ names and semantics whenever possible. Otherwise, it would be a complete mess to understand code where more than one language (Rust, C, C++) is being used to modify the same value using atomic primitives. The ISO C and C++ stuff isn't perfect, but people are working hard to get them to be closer to perfect and there's no need to reinvent the wheel or to generate confusion with different names or slightly different semantics for similarly-named things.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Feb 13, 2016

Contributor

Otherwise, it would be a complete mess to understand code where more than one language (Rust, C, C++) is being used to modify the same value using atomic primitives.

I do not find this argument persuading. __sync_*_compare_and_swap (C), atomic::compare_exchange_stronk (c++), Atomic*::compareAndSet (java) and Atomic*::{compare_and_swap,compare_exchange} (rust, this rfc) are all pretty different from the naming standpoint already (don’t even match between C and C++!). They all intuitively seem to mean the same thing (with the exception of whatever the meaning strong in the c++ API is supposed to carry).

For the weak CAS, I’d rather use java style naming (appending/prepending weak only to the weak variant) and leaving off strong on the regular version.

Contributor

nagisa commented Feb 13, 2016

Otherwise, it would be a complete mess to understand code where more than one language (Rust, C, C++) is being used to modify the same value using atomic primitives.

I do not find this argument persuading. __sync_*_compare_and_swap (C), atomic::compare_exchange_stronk (c++), Atomic*::compareAndSet (java) and Atomic*::{compare_and_swap,compare_exchange} (rust, this rfc) are all pretty different from the naming standpoint already (don’t even match between C and C++!). They all intuitively seem to mean the same thing (with the exception of whatever the meaning strong in the c++ API is supposed to carry).

For the weak CAS, I’d rather use java style naming (appending/prepending weak only to the weak variant) and leaving off strong on the regular version.

@briansmith

This comment has been minimized.

Show comment
Hide comment
@briansmith

briansmith Feb 14, 2016

I do not find this argument persuading. _sync*_compare_and_swap (C), atomic::compare_exchange_stronk (c++),

C11 uses the same names as C++11 for these, except they prefixed atomic_ to them due to C's poor namespacing issues. See http://en.cppreference.com/w/c/atomic/atomic_compare_exchange.

briansmith commented Feb 14, 2016

I do not find this argument persuading. _sync*_compare_and_swap (C), atomic::compare_exchange_stronk (c++),

C11 uses the same names as C++11 for these, except they prefixed atomic_ to them due to C's poor namespacing issues. See http://en.cppreference.com/w/c/atomic/atomic_compare_exchange.

@Amanieu

This comment has been minimized.

Show comment
Hide comment
@Amanieu

Amanieu Feb 15, 2016

Contributor

I would prefer atomic_compare_exchange_strong for consistency with C++, but I don't feel too strongly either way.

Contributor

Amanieu commented Feb 15, 2016

I would prefer atomic_compare_exchange_strong for consistency with C++, but I don't feel too strongly either way.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 18, 2016

Member

The libs team discussed this RFC during triage yesterday and the conclusion was that we'd like to merge! We felt, however, that the name compare_exchange was more "rustic" over compare_exchange_strong. @Amanieu would you be ok updating this to mention that name instead? After that I can merge.

Member

alexcrichton commented Feb 18, 2016

The libs team discussed this RFC during triage yesterday and the conclusion was that we'd like to merge! We felt, however, that the name compare_exchange was more "rustic" over compare_exchange_strong. @Amanieu would you be ok updating this to mention that name instead? After that I can merge.

@Amanieu

This comment has been minimized.

Show comment
Hide comment
@Amanieu

Amanieu Feb 18, 2016

Contributor

Done

Contributor

Amanieu commented Feb 18, 2016

Done

@alexcrichton alexcrichton merged commit 9638c35 into rust-lang:master Feb 18, 2016

bors added a commit to rust-lang/rust that referenced this pull request Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment