Expand the documentation for the std::sync module
#54078
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/libstd/sync/mod.rs
Outdated
| //! other types of concurrent primitives. | ||
| //! ## The need for synchronization | ||
| //! | ||
| //! On an ideal single-core CPU, the timeline of events happening in a program |
There was a problem hiding this comment.
You can have concurrency on a single core CPU.
There was a problem hiding this comment.
I do mention this possibility when talking about the need for compiler fences with signal handlers / interrupts in single-threaded programs.
I couldn't think of a better way to introduce the idea of sequential consistency / program order, in comparison to actual execution order (which is why I used "ideal", to indicate reality is a bit more complicated).
There was a problem hiding this comment.
So, I get where this paragraph is coming from, but agree it could be worded differently. I'm struggling a bit... I think the issues are:
- it's not clear that this is "ideal".
- This is phrased as being true, when the intention is to show that it's not true.
src/libstd/sync/mod.rs
Outdated
| //! Considering the following code, operating on some global static variables: | ||
| //! | ||
| //! ```rust | ||
| //! # static mut A: u32 = 0; |
There was a problem hiding this comment.
Why hiding them? Also: they're not global since they're inside a function. If you don't declare the main function, then all code is put inside it (except extern crate declarations).
There was a problem hiding this comment.
Why hiding them?
I was trying to keep the example code clean and focus on the executed instructions. But I guess they won't hurt readability.
Also: they're not global since they're inside a function.
Thanks, should be fixed now.
|
cc @rust-lang/docs |
|
Tirage; @GuillaumeGomez What's the current status of the review of this PR? |
|
I'd like english native speakers to review. |
src/libstd/sync/mod.rs
Outdated
| //! other types of concurrent primitives. | ||
| //! ## The need for synchronization | ||
| //! | ||
| //! On an ideal single-core CPU, the timeline of events happening in a program |
There was a problem hiding this comment.
So, I get where this paragraph is coming from, but agree it could be worded differently. I'm struggling a bit... I think the issues are:
- it's not clear that this is "ideal".
- This is phrased as being true, when the intention is to show that it's not true.
src/libstd/sync/mod.rs
Outdated
| //! updated. | ||
| //! | ||
| //! - the final result could be determined just by looking at the code at compile time, | ||
| //! so [constant folding] might turn the whole block into a simple `println!("7 4 4")` |
There was a problem hiding this comment.
These bullets should all be converted to sentences; capital letters and periods please!
Provides an overview on why synchronization is required, as well a short summary of what sync primitives are available.
Because `fn main()` was added automatically, the variables were actually local statics.
Reword the lead paragraph and turn the list items into complete sentences.
|
I've rebased and then addressed the review comments. Thanks! |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Overall i think the docs here are fine for describing why you would need synchronization in the first place, but i don't think it does a good job describing the actual contents of the module. I feel like it spends a lot of time on low-level details like fences but doesn't spend much time on higher-level synchronization structures like Arc/Mutex/RwLock/channels/etc that a user is going to require more of the time. Most of the items in this module aren't meant to deal with single-threaded execution order, so outside of the documentation for compiler_fence it's no more than a curiosity. I'd appreciate it if the commonly-used items in this module (IMO, Arc, Mutex, channel, at least) were given more screen time, so to speak.
src/libstd/sync/mod.rs
Outdated
| //! other types of concurrent primitives. | ||
| //! ## The need for synchronization | ||
| //! | ||
| //! Conceptually, a Rust program is simply a series of operations which will |
There was a problem hiding this comment.
Please remove the word "simply".
src/libstd/sync/mod.rs
Outdated
| //! be executed on a computer. The timeline of events happening in the program | ||
| //! is consistent with the order of the operations in the code. | ||
| //! | ||
| //! Considering the following code, operating on some global static variables: |
There was a problem hiding this comment.
"Consider the following code..." (please change tense of "Considering")
src/libstd/sync/mod.rs
Outdated
| //! } | ||
| //! ``` | ||
| //! | ||
| //! It appears _as if_ some variables stored in memory are changed, an addition |
There was a problem hiding this comment.
This emphasis seems odd; i feel like it would flow better if the emphasis was moved to "appears" or removed entirely.
src/libstd/sync/mod.rs
Outdated
| //! | ||
| //! - **Multiprocessor** system, where multiple hardware threads run at the same time. | ||
| //! In multi-threaded scenarios, you can use two kinds of primitives to deal | ||
| //! with synchronization: |
There was a problem hiding this comment.
The flow between these bullet points seems odd; they're using different structures. Compare the first phrase of each item:
- Compiler reordering instructions:
- Single processor executing instructions out-of-order:
- Multiprocessor system, where multiple hardware threads run at the same time.
The first two items use colons to separate a title from the rest of the segment, but the last one just treats the first sentence as the summary and doesn't have a succinct title like the others.
I feel like this item would work better like this:
- A multiprocessor system executing multiple hardware threads at the same time: In multi-threaded scenarios...
src/libstd/sync/mod.rs
Outdated
| //! Use [compiler fences] to prevent this reordering. | ||
| //! | ||
| //! - **Single processor** executing instructions [out-of-order]: modern CPUs are | ||
| //! capable of [superscalar] execution, i.e. multiple instructions might be |
There was a problem hiding this comment.
- A single processor executing instructions out-of-order: ...
|
I've addressed the review comments and expanded the section on higher-level synchronization primitives provided by the standard library. |
|
@bors: r+ rollup thanks! |
|
📌 Commit 6ba5584 has been approved by |
bors
added
S-waiting-on-bors
…teveklabnik Expand the documentation for the `std::sync` module I've tried to expand the documentation for Rust's synchronization primitives. The module level documentation explains why synchronization is required when working with a multiprocessor system, and then links to the appropiate structure in this module. Fixes rust-lang#29377, since this should be the last item on the checklist (documentation for `Atomic*` was fixed in rust-lang#44854, but not ticked off the checklist).
Rollup of 11 pull requests Successful merges: - #54078 (Expand the documentation for the `std::sync` module) - #54717 (Cleanup rustc/ty part 1) - #54781 (Add examples to `TyKind::FnDef` and `TyKind::FnPtr` docs) - #54787 (Only warn about unused `mut` in user-written code) - #54804 (add suggestion for inverted function parameters) - #54812 (Regression test for #32382.) - #54833 (make `Parser::parse_foreign_item()` return a foreign item or error) - #54834 (rustdoc: overflow:auto doesn't work nicely on small screens) - #54838 (Fix typo in src/libsyntax/parse/parser.rs) - #54851 (Fix a regression in 1.30 by reverting #53564) - #54853 (Remove unneccessary error from test, revealing NLL error.) Failed merges: r? @ghost
I've tried to expand the documentation for Rust's synchronization primitives. The module level documentation explains why synchronization is required when working with a multiprocessor system,
and then links to the appropiate structure in this module.
Fixes #29377, since this should be the last item on the checklist (documentation for
Atomic*was fixed in #44854, but not ticked off the checklist).