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.

Sign up for GitHub

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

Jump to bottom

Improve std::thread::available_parallelism docs #89670

Merged
merged 1 commit into from
Oct 14, 2021
Merged

Improve std::thread::available_parallelism docs #89670

merged 1 commit into from
Oct 14, 2021

Conversation

yoshuawuyts
Copy link
Member

Tracking issue: #74479

This PR reworks the documentation of std::thread::available_parallelism, as requested here.

Changes

The following changes are made:

  • We've removed prior mentions of "hardware threads" and instead centers the docs around "parallelism" as a resource available to a program.
  • We now provide examples of when available_parallelism may return numbers that differ from the number of CPU cores in the host machine.
  • We now mention that the amount of available parallelism may change over time.
  • We make note of which platform components we don't take into account which more advanced users may want to take note of.
  • The example has been updated, which should be a bit easier to use.
  • We've added a docs alias to num-cpus which provides similar functionality to available_parallelism, and is one of the most popular crates on crates.io.

Thanks!

r? @BurntSushi

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 8, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
the8472
the8472 reviewed Oct 8, 2021
View reviewed changes
library/std/src/thread/mod.rs Outdated
/// - The amount of parallelism available may be _higher_ than the CPU core
/// count when a CPU exposes multiple "hardware threads" per CPU core.
/// - The reported amount of parallelism available may be _higher_ than the CPU
/// core count if a CPU has "Hyper-Threading" enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to list these first two as separate points? They're more or less the same as far as the amount of threads that'll execute at the same time is concerned.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

I also think we should just de-emphasize "cores" here; these docs are going out of their way to explain how the return value might not be CPU cores, but really, there's no reason it would be "cores". Most commonly, it'll be whatever the OS reports as CPUs.

Copy link
Member

Choose a reason for hiding this comment

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

Is "CPU" even the thing we want to discuss here? What about GPUs and other possible implementations of computers which Rust may target and for which this function would likely return something else. I really like the summary of this function in that context. It doesn't mention threads or hardware anymore and instead turns the explanation around to be all about the program instead.

@shoffmeister
Copy link
Contributor

For the sake of maintainability (technical documentation debt), I'd recommend to simply omit the whole section on The following platform limitations currently apply:.

Instead, I'd suggest wording to the effect of Platform-specific implementations will come with further individual limitations. Please review the respective target platform implementation for details. This is clearly less expressive than the existing text, but also makes less promises.

IMHO, anybody who is interested in the "fully correct picture on parallelism" will have to study the implementation, anyway (or get this from highly specialized implementation)

nagisa
nagisa reviewed Oct 9, 2021
View reviewed changes
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Really nice.

library/std/src/thread/mod.rs Outdated Show resolved Hide resolved
library/std/src/thread/mod.rs Outdated
/// - The amount of parallelism available may be _higher_ than the CPU core
/// count when a CPU exposes multiple "hardware threads" per CPU core.
/// - The reported amount of parallelism available may be _higher_ than the CPU
/// core count if a CPU has "Hyper-Threading" enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Is "CPU" even the thing we want to discuss here? What about GPUs and other possible implementations of computers which Rust may target and for which this function would likely return something else. I really like the summary of this function in that context. It doesn't mention threads or hardware anymore and instead turns the explanation around to be all about the program instead.

@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 11, 2021
@BurntSushi
Copy link
Member

It looks like @joshtriplett is all over this! Thank you. I think my concern is totally addressed here, so I'll hand things over if you don't mind.

r? @joshtriplett

@yoshuawuyts
Copy link
Member Author

Finished merging @joshtriplett's suggestions; believe this should be ready for review again!

@joshtriplett
Copy link
Member

@bors r+

🧵📄👍

@bors
Copy link
Contributor

bors commented Oct 13, 2021

📌 Commit 1c456bc81310fa0cc6641b239a3102adaa49820f has been approved by joshtriplett

@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 Oct 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Oct 13, 2021 •
edited
Loading

The last run contained a typo in the example which caused the test to fail. Fixed it, and tests now pass. Can someone re-run bors again? Thanks!

@the8472
Copy link
Member

the8472 commented Oct 13, 2021

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Oct 13, 2021

📌 Commit 21429ed has been approved by joshtriplett

@matthiaskrgr matthiaskrgr mentioned this pull request Oct 13, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2021
@bors
Auto merge of rust-lang#89858 - matthiaskrgr:rollup-evsnr2e, r=matthi…
8c852bc 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#89347 (suggestion for typoed crate or module)
 - rust-lang#89670 (Improve `std::thread::available_parallelism` docs)
 - rust-lang#89757 (Use shallow clones for submodules)
 - rust-lang#89759 (Assemble the compiler when running `x.py build`)
 - rust-lang#89846 (Add `riscv32imc-esp-espidf` to 1.56 changelog)
 - rust-lang#89853 (Update the 1.56.0 release header for consistency)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 06110c0 into rust-lang:master Oct 14, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 14, 2021
@yoshuawuyts yoshuawuyts deleted the available-parallelism-docs branch October 14, 2021 12:55
@the8472 the8472 mentioned this pull request Oct 23, 2021
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021
@matthiaskrgr
Rollup merge of rust-lang#90198 - the8472:available-parallelism-runti…
f99f296 
…me, r=joshtriplett

Add caveat about changing parallelism and function call overhead

As discussed in rust-lang#89670 (comment)

r? `@joshtriplett`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021
@matthiaskrgr
Rollup merge of rust-lang#90198 - the8472:available-parallelism-runti…
1680359 
…me, r=joshtriplett

Add caveat about changing parallelism and function call overhead

As discussed in rust-lang#89670 (comment)

r? ``@joshtriplett``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the8472 the8472 the8472 left review comments

@nagisa nagisa nagisa left review comments

@joshtriplett joshtriplett joshtriplett left review comments

Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.
10 participants
@yoshuawuyts @rust-log-analyzer @shoffmeister @BurntSushi @joshtriplett @bors @the8472 @nagisa @rust-highfive @rustbot