Update docs of 'fence' #41217

topecongiro · 2017-04-11T13:45:16Z

This PR updates the docs for std::sync::atomic::fence with an example and a diagram.
Part of #29377.
r? @steveklabnik

steveklabnik · 2017-04-11T15:58:45Z

@Amanieu, would you mind reviewing this? Or at least, someone other than me should make sure it's accurate. This isn't my area of specialty.

Amanieu · 2017-04-11T18:43:18Z

The example looks correct, and the explanation seems to be correct as well.

Amanieu · 2017-04-11T18:44:02Z

Actually, I think the unsafe impl are not necessary, they are already implemented by AtomicBool.

topecongiro · 2017-04-12T02:41:06Z

Thanks for comments! Removed unsafe impls from the example.

topecongiro · 2017-04-12T14:17:11Z

src/libcore/sync/atomic.rs

 /// A fence 'A' which has [`Release`] ordering semantics, synchronizes with a
 /// fence 'B' with (at least) [`Acquire`] semantics, if and only if there exists
 /// atomic operations X and Y, both operating on some atomic object 'M' such
 /// that A is sequenced before X, Y is synchronized before B and Y observes
 /// the change to M. This provides a happens-before dependence between A and B.
 ///
+///     Thread 1                                          Thread 2


Travis build is failing due to this line. I removed this line on my local environment and ./x.py test src/libcore completed successfully. Is this a bug, or intended?

CommonMark considers indented text to be code:

http://spec.commonmark.org/0.27/#indented-code-blocks

As a result, it attempts to run this as a Rust program:

Thread 1 Thread 2

and that's not a valid Rust program :)

The best idea I have here is to wrap this diagram in three backticks and annotate with 'ignore' as seen here: (ignore the backslashes)

/// \```ignore /// Thread 1 Thread 2 /// /// ... /// ... /// /// \```

@frewsxcv Thank you!

Unfortunately,

/// \```ignore /// ... /// \```

didn't work (the document got corrupted).
Instead, I tried ```text and it worked fine.

ghost

I have just a few minor nits...

The explanation and examples are correct.
Love the ASCII drawing and the example! It'd be good to see more of these. :)

ghost · 2017-04-17T00:29:06Z

src/libcore/sync/atomic.rs

+/// fences when the certain condition is met. Also, a fence prevents reordering
+/// of operations around a fence by CPU or a compiler. Each behavior is
+/// controlled by the value of [`Ordering`].
+///
 /// A fence 'A' which has [`Release`] ordering semantics, synchronizes with a


Perhaps this should say "A fence 'A' which has (at least) [Release]..."? Note the text in parentheses.

ghost · 2017-04-17T00:34:52Z

src/libcore/sync/atomic.rs

@@ -1550,12 +1550,30 @@ unsafe fn atomic_xor<T>(dst: *mut T, val: T, order: Ordering) -> T {

 /// An atomic fence.
 ///
+/// A fence creates happens-before edges between other atomic operations or


s/edges/dependence/ or s/edges/relationships/

A fence creates happens-before relationships between not only atomic operations and fences, but also between normal memory operations.

ghost · 2017-04-17T00:40:05Z

src/libcore/sync/atomic.rs

+/// A fence creates happens-before edges between other atomic operations or
+/// fences when the certain condition is met. Also, a fence prevents reordering
+/// of operations around a fence by CPU or a compiler. Each behavior is
+/// controlled by the value of [`Ordering`].


Nit: If you wish, you may rephrase this along the lines of: "Also, depending on the specified order, a fence prevents certain types of memory operation reorderings around it by CPU or compiler."

arielb1 · 2017-04-18T08:33:24Z

@steveklabnik are you looking at this?

steveklabnik · 2017-04-18T14:13:52Z

I am not sure if @stjepang 's nits should be addressed before merging or not, basically.

topecongiro · 2017-04-19T15:16:54Z

@stjepang Thank you for your feedbacks!

topecongiro · 2017-04-19T15:19:41Z

Is it ok to reflect the nits?

steveklabnik · 2017-04-19T17:27:06Z

Yup!

…

On Wed, Apr 19, 2017 at 11:19 AM, Seiichi Uchida ***@***.***> wrote: Is it ok to reflect the nits? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41217 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABsigtufEOUWQIcKbjf7tHM0XSl1BMiks5rxiYRgaJpZM4M6HmX> .

topecongiro · 2017-04-24T10:35:53Z

Sorry for being lazy... I pushed the changes suggested by @stjepang. Thanks again!

ghost

Looking good - just a few more grammar fixes, and then this is ready to go! :)
Thanks so much for taking an effort to improve the docs!

ghost · 2017-04-24T12:34:50Z

src/libcore/sync/atomic.rs

+/// A fence creates happens-before relationships between other atomic operations or
+/// fences when the certain condition is met. Also, depending of the specified
+/// `order`, a fence prevents reordering certain types of memory operations
+/// around a fence by CPU or a compiler.


s/a fence/it/

@stejpang Thank you for your advices! One question, are you referring to the last a fence, or the second and the last ones?

I just meant "around a fence" -> "around it". :)
But now that I think more about this, how about the following (slightly simplified) paragraph?

"Depending on the specified order, a fence prevents the compiler and CPU from reordering certain types of memory operations around it. That creates synchronizes-with relationships between it and atomic operations or fences in other threads."

I think here it's more correct to talk about synchronizes-with rather than happens-before. Pretty much every two operations within a single thread already have a happens-before relationship.
Fences really just establish synchronizes-with relationships with fences/atomics in other threads.

See the following links for a clearer explanation:
http://preshing.com/20130702/the-happens-before-relation/
http://preshing.com/20130823/the-synchronizes-with-relation/

Sorry for being so pedantic :)

I agree with you, since rust does not have a signal and fence prohibits Relaxed ordering.
Thank you very much for your explanation!

ghost · 2017-04-24T12:35:28Z

src/libcore/sync/atomic.rs

-/// fence 'B' with (at least) [`Acquire`] semantics, if and only if there exists
-/// atomic operations X and Y, both operating on some atomic object 'M' such
+/// A fence creates happens-before relationships between other atomic operations or
+/// fences when the certain condition is met. Also, depending of the specified


ghost · 2017-04-24T12:42:08Z

src/libcore/sync/atomic.rs

+///
+/// A fence 'A' which has (at least) [`Release`] ordering semantics, synchronizes
+/// with a fence 'B' with (at least) [`Acquire`] semantics, if and only if there
+/// exists operations X and Y, both operating on some atomic object 'M' such


s/exists/exist/

arielb1 · 2017-05-02T09:24:27Z

@steveklabnik I think the nits were addressed. Can you look at this?

steveklabnik · 2017-05-02T14:08:10Z

@bors: r+ rollup

thank you so much, and sorry about all of the delays.

bors · 2017-05-02T14:08:10Z

📌 Commit 91a9866 has been approved by steveklabnik

@steveklabnik

…eveklabnik Update docs of 'fence' This PR updates the docs for `std::sync::atomic::fence` with an example and a diagram. Part of rust-lang#29377. r? @steveklabnik

Rollup of 7 pull requests - Successful merges: #41217, #41625, #41640, #41653, #41656, #41657, #41705 - Failed merges:

rust-highfive assigned steveklabnik Apr 11, 2017

alexcrichton added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Apr 11, 2017

topecongiro force-pushed the docs/atomic-fence branch from 7a25dac to ed754cb Compare April 12, 2017 02:40

topecongiro commented Apr 12, 2017

View reviewed changes

frewsxcv added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

topecongiro force-pushed the docs/atomic-fence branch from ed754cb to f5b077a Compare April 14, 2017 13:50

ghost approved these changes Apr 17, 2017

View reviewed changes

topecongiro force-pushed the docs/atomic-fence branch from f5b077a to 3fd8c8e Compare April 24, 2017 10:34

ghost approved these changes Apr 24, 2017

View reviewed changes

Add an example for 'fence'

91a9866

topecongiro force-pushed the docs/atomic-fence branch from 3fd8c8e to 91a9866 Compare May 1, 2017 08:25

arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2017

steveklabnik approved these changes May 2, 2017

View reviewed changes

frewsxcv mentioned this pull request May 3, 2017

Rollup of 7 pull requests #41717

Merged

bors added a commit that referenced this pull request May 3, 2017

Auto merge of #41717 - frewsxcv:rollup, r=frewsxcv

146dc67

Rollup of 7 pull requests - Successful merges: #41217, #41625, #41640, #41653, #41656, #41657, #41705 - Failed merges:

bors merged commit 91a9866 into rust-lang:master May 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docs of 'fence' #41217

Update docs of 'fence' #41217

topecongiro commented Apr 11, 2017

steveklabnik commented Apr 11, 2017

Amanieu commented Apr 11, 2017

Amanieu commented Apr 11, 2017

topecongiro commented Apr 12, 2017

topecongiro Apr 12, 2017

frewsxcv Apr 14, 2017

topecongiro Apr 14, 2017

topecongiro Apr 14, 2017

ghost left a comment •

edited by ghost

Loading

ghost Apr 17, 2017 •

edited by ghost

Loading

ghost Apr 17, 2017 •

edited by ghost

Loading

ghost Apr 17, 2017 •

edited by ghost

Loading

arielb1 commented Apr 18, 2017

steveklabnik commented Apr 18, 2017

topecongiro commented Apr 19, 2017

topecongiro commented Apr 19, 2017

steveklabnik commented Apr 19, 2017 via email

topecongiro commented Apr 24, 2017

ghost left a comment

ghost Apr 24, 2017

topecongiro Apr 25, 2017

ghost Apr 25, 2017

topecongiro May 1, 2017

ghost Apr 24, 2017

ghost Apr 24, 2017

arielb1 commented May 2, 2017

steveklabnik commented May 2, 2017

bors commented May 2, 2017

Update docs of 'fence' #41217

Update docs of 'fence' #41217

Conversation

topecongiro commented Apr 11, 2017

steveklabnik commented Apr 11, 2017

Amanieu commented Apr 11, 2017

Amanieu commented Apr 11, 2017

topecongiro commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 17, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 17, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 17, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

arielb1 commented Apr 18, 2017

steveklabnik commented Apr 18, 2017

topecongiro commented Apr 19, 2017

topecongiro commented Apr 19, 2017

steveklabnik commented Apr 19, 2017 via email

topecongiro commented Apr 24, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented May 2, 2017

steveklabnik commented May 2, 2017

bors commented May 2, 2017

ghost left a comment •

edited by ghost

Loading

ghost Apr 17, 2017 •

edited by ghost

Loading

ghost Apr 17, 2017 •

edited by ghost

Loading

ghost Apr 17, 2017 •

edited by ghost

Loading