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

atomic::Ordering: Get rid of misleading parts of intro #56023

Merged
merged 1 commit into from
Nov 29, 2018
Merged

atomic::Ordering: Get rid of misleading parts of intro #56023

merged 1 commit into from
Nov 29, 2018

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Nov 17, 2018

Remove the parts of atomic::Ordering's intro that wrongly claimed that
SeqCst prevents all reorderings around it.

Closes #55196

This is a (minimal) alternative to #55233.

I also wonder if it would be worth adding at least some warnings that atomics are often a footgun/hard to use correctly, similarly like mem::transmute or other functions have.

@rust-highfive
Copy link
Collaborator

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 17, 2018
@bluss
Copy link
Member

bluss commented Nov 17, 2018

r? @stjepang

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This seems correct - approving.

However, I personally don't find the intro about Relaxed and SeqCst particularly useful. It's full of jargon and lacking details:

  • What does "synchronized" mean?
  • What does "total order" mean?
  • What is a store-load pair? How do operations pair up?
  • What memory do store-load SeqCst operations synchronize?

For people who don't know about memory orderings that might sound like gibberish. :) And for people who do, it's not very informative anyway.

Just saying something equivalent to "use SeqCst if in doubt and read the LLVM docs to learn more" would be enough, I think.

src/libcore/sync/atomic.rs Outdated Show resolved Hide resolved
src/libcore/sync/atomic.rs Outdated Show resolved Hide resolved
Remove the parts of atomic::Ordering's intro that wrongly claimed that
SeqCst prevents all reorderings around it.

Closes #55196
@vorner
Copy link
Contributor Author

vorner commented Nov 18, 2018

However, I personally don't find the intro about Relaxed and SeqCst particularly useful. It's full of jargon and lacking details

Well, I tried to write something more useful in #55233 and the consensus was that such things should probably go to nomicon (and I plan on updating that eventually). So, I don't know how much I'm able to say without giving a full, digestible explanation and still be correct.

I'm open to discussing what level of explanation is appropriate here and in the nomicon. But I'd like to get rid of the wrong parts first.

Just saying something equivalent to "use SeqCst if in doubt and read the LLVM docs to learn more" would be enough, I think.

That's the other quite often given advice about atomics I quite don't like, because it is misleading. If in doubt, read the LLVM docs ‒ sure, but using SeqCst without understanding how the synchronization works is likely to lead to wrong code (eg. SeqCst on compare_and_swap that didn't swap won't publish my changes and it's a non-obvious trap).

That's what I was asking about the warning ‒ should we say something like „Don't mess with these until you read the docs and use Mutex instead“?

@ghost
Copy link

ghost commented Nov 19, 2018

Well, I tried to write something more useful in #55233 and the consensus was that such things should probably go to nomicon (and I plan on updating that eventually).

That'd be fantastic! <3

That's the other quite often given advice about atomics I quite don't like, because it is misleading.

You're right and the pitfall of sequentially consistent CAS operations is a very good point. However, I'll be a little sad if our docs say "if you don't understand memory orderings, use mutexes".

In Java, volatile variables are just sequentially consistent atomics and I think they can be productively used by anyone without a deep understanding of memory orderings. The same applies to e.g. Go and JavaScript. Note that the docs of those languages don't even mention orderings. In C++, std::atomic is designed so that SeqCst is the default ordering for all operations and you can use it without even thinking about memory barriers. I wish atomics in Rust don't get reserved for advanced users only.

Also, I'd point out that mutexes and RW locks come with their own similar pitfalls. For example, they're not guaranteed to be sequentially consistent. Or, read operations in RW locks can elide actual locking if there's no contention. And yet, mutexes and RW locks can be used fearlessly by anyone without even understanding those words.

Anyways, just my two cents. :) This PR is good to go as far as I'm concerned. 👍

@TimNN
Copy link
Contributor

TimNN commented Nov 27, 2018

@bors r=@stjepang

@bors
Copy link
Contributor

bors commented Nov 27, 2018

📌 Commit cc63bd4 has been approved by @stjepang

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2018
@TimNN
Copy link
Contributor

TimNN commented Nov 27, 2018

@bors rollup

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 28, 2018
…stjepang

atomic::Ordering: Get rid of misleading parts of intro

Remove the parts of atomic::Ordering's intro that wrongly claimed that
SeqCst prevents all reorderings around it.

Closes rust-lang#55196

This is a (minimal) alternative to rust-lang#55233.

I also wonder if it would be worth adding at least some warnings that atomics are often a footgun/hard to use correctly, similarly like `mem::transmute` or other functions have.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 29, 2018
…stjepang

atomic::Ordering: Get rid of misleading parts of intro

Remove the parts of atomic::Ordering's intro that wrongly claimed that
SeqCst prevents all reorderings around it.

Closes rust-lang#55196

This is a (minimal) alternative to rust-lang#55233.

I also wonder if it would be worth adding at least some warnings that atomics are often a footgun/hard to use correctly, similarly like `mem::transmute` or other functions have.
bors added a commit that referenced this pull request Nov 29, 2018
Rollup of 22 pull requests

Successful merges:

 - #55391 (bootstrap: clean up a few clippy findings)
 - #56021 (avoid features_untracked)
 - #56023 (atomic::Ordering: Get rid of misleading parts of intro)
 - #56080 (Reduce the amount of bold text at doc.rlo)
 - #56114 (Enclose type in backticks for "non-exhaustive patterns" error)
 - #56124 (Fix small doc mistake on std::io::read::read_to_end)
 - #56127 (Update an outdated comment in mir building)
 - #56148 (Add rustc-guide as a submodule)
 - #56149 (Make std::os::unix/linux::fs::MetadataExt::a/m/ctime* documentation clearer)
 - #56220 (Suggest appropriate place for lifetime when declared after type arguments)
 - #56223 (Make JSON output from -Zprofile-json valid)
 - #56236 (Remove unsafe `unsafe` inner function.)
 - #56255 (Update outdated code comments in StringReader)
 - #56257 (rustc-guide has moved to rust-lang/)
 - #56273 (Add missing doc link)
 - #56289 (Fix small typo in comment of thread::stack_size)
 - #56294 (Fix a typo in the documentation of std::ffi)
 - #56312 (Deduplicate literal -> constant lowering)
 - #56319 (fix futures creating aliasing mutable and shared ref)
 - #56321 (rustdoc: add bottom margin spacing to nested lists)
 - #56322 (resolve: Fix false-positives from lint `absolute_paths_not_starting_with_crate`)
 - #56330 (Clean up span in non-trailing `..` suggestion)

Failed merges:

r? @ghost
@bors bors merged commit cc63bd4 into rust-lang:master Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants