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

Rewrite atomics section #378

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

SabrinaJewson
Copy link

@SabrinaJewson SabrinaJewson commented Aug 4, 2022

Rendered

This PR is for my attempted rewrite of the atomics section of the Nomicon, to give it a more spec-focused explanation and avoid misconceptions like time and reordering. Currently it’s far from finished, containing only an explanation of multi-threaded execution and the start of the “relaxed” section, but I’m opening this PR for early review and feedback on the explanation method.

Reading order:

  1. atomics.md
  2. multithread.md
  3. relaxed.md
  4. acquire-release.md
  5. seqcst.md
  6. fences.md
Edit: I now see that copyright means this won’t work, so I’ve removed it.

I also copy-pasted in the C++ spec, but changed a couple things, due things like the lack of consume and sig_atomic_t:

I’m not certain about the copyright of this — ISO says that they only hold copyright over published versions, but I couldn’t find information about copyright of drafts.

src/atomics/relaxed.md Outdated Show resolved Hide resolved
src/atomics/relaxed.md Outdated Show resolved Hide resolved
@jwakely
Copy link

jwakely commented Aug 4, 2022

I’m not certain about the copyright of this — ISO says that they only hold copyright over published versions, but I couldn’t find information about copyright of drafts.

https://en.cppreference.com/w/cpp/language/memory_model and https://en.cppreference.com/w/cpp/atomic/memory_order#Formal_description paraphrase the relevant parts of the C++ spec, and are CC-BY-SA and GFDL dual-licensed. See What can I do with the material on this site?.

src/atomics/relaxed.md Outdated Show resolved Hide resolved
src/atomics/relaxed.md Outdated Show resolved Hide resolved
src/atomics/relaxed.md Outdated Show resolved Hide resolved
@SabrinaJewson SabrinaJewson marked this pull request as ready for review October 1, 2022 08:34
@SabrinaJewson
Copy link
Author

I have now rendered this PR for easier review.

@cbeuw
Copy link

cbeuw commented Oct 1, 2022

This looks absolutely amazing! The Unicode diagrams are nicely rendered on both my Mac and Android phone, and are very pretty. This may well be the best tutorial on C++ memory model out there, including all the ones I've seen targeting C++ exclusively.

I definitely wouldn't call myself a weak memory expert, considering whenever I work with it I always come up with new questions instead of being able to tell the definite behaviours (but this may be a good thing sometimes). Unfortunately for me, the next 3 months will be extremely busy. I won't be able to go through it in detail, but I'll have my eye on it from time to time, both now and after it's been merged. There will almost certainly be extremely subtle errors given the topic's nature, but that shouldn't prevent this from going live.

Comment on lines +87 to +88
order. However, some algorithms will require a globally agreed-upon ordering,
and this is where `SeqCst` can come in useful.
Copy link

Choose a reason for hiding this comment

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

I'd be interested to know which algorithms actually require SeqCst. The leading motivation for proposing algorithms under SeqCst is probably that it's easier to reason about, not that a Single Total Order is necessary (read-latest may be needed but this can be done with idempotent RMW).

src/atomics/seqcst.md Outdated Show resolved Hide resolved
src/atomics/atomics.md Outdated Show resolved Hide resolved
@newpavlov
Copy link

newpavlov commented Nov 25, 2022

I have minor rendering issues with the diagrams on Linux (in both Firefox and Chromium):

I suggest replacing them with inline SVG or separate image files.

@JanBeh
Copy link

JanBeh commented Nov 25, 2022

From Issue #104814 on Rust:

[…] it should be clarified whether the difference between the Rust documentation and the C++20 reference exists intentionally, i.e. is Rust deliberately giving less guarantees to the programmer than the C++20 standard does? (Even though still following the C++20 memory model in practice "as of now".)

I have been told that the C++20 reference takes precedence over Rust's documentation, but wanted to raise this issue nonetheless as it may be relevant for future backward compatibility.

@WaffleLapkin
Copy link
Member

What is the status here? 👀

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2023

This is in my queue to review, but I have 100+ PRs in that queue, and this one is quite large and a challenging topic. Realistically I probably won't get to it soon.

In the meantime, I recommend people check out Mara's book at https://marabos.nl/atomics/ which is now free.

EDIT to add: To be clear, I very much want to see this land. I just may not be able to make time for it for a while.

src/atomics/atomics.md Outdated Show resolved Hide resolved
src/atomics/multithread.md Outdated Show resolved Hide resolved
Copy link

@chorman0773 chorman0773 left a comment

Choose a reason for hiding this comment

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

All of the information here looks to be correct.

@WaffleLapkin
Copy link
Member

@ehuss do you still have too much in the review queue?

Comment on lines +14 to +16
for everyone. This enables you to write code without having to think about
what the underlying system does or how it does it, as long as you obey the
Abstract Machine’s rules you know you’ll be fine.

Choose a reason for hiding this comment

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

This feels like a good place to point out that arguments of the form "it works on the physical hardware, so it's fine" are not okay?

@SabrinaJewson
Copy link
Author

I just pushed a commit to fix the incorrect claim that one needs 4 threads to observe the difference between SeqCst and AcqRel, which was a misunderstanding I had. I also changed the example that shows the need for SeqCst to a simpler one that only uses two threads.

The changed sections include the SeqCst chapter and the part on SeqCst fences. Thanks @ibraheemdev for pointing this out!

@Noratrieb
Copy link
Member

@ehuss it seems like people with expertise in atomics have given this a look by now. so I think it would just need an editorial review from you, which is hopefully quicker than a technical one (it takes some time still of course, and I don't expect you to take a look at this very soon, but just FYI)

@traviscross
Copy link

We discussed this in the lang-docs call today. Thanks to everyone here for the reviews and the feedback, and to @SabrinaJewson for this work. We're trying to have a look at this. Even just editorially, it's an impressive body of work and may take some time. Thanks for bearing with us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.