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

Use std:: thread:: available_parallelism() instead of num_cpus dependency #937

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Use std:: thread:: available_parallelism() instead of num_cpus dependency #937

merged 4 commits into from
Sep 18, 2023

Conversation

andrewdavidmackenzie
Copy link
Contributor

From rust 1.59 we have a call for available_parallelism() in the std library and we can remove the dependency on num_cpu.

Open questions for maintainers:

  • what is the minimum version of rust that you want to support
  • what is the best default value when the call to get parallelism fails? I chose "1".

@andrewdavidmackenzie andrewdavidmackenzie changed the title Use std::thread::available_parallelism() instead of num_cpus dependency Use std:\:thread::available_parallelism() instead of num_cpus dependency May 16, 2022
@andrewdavidmackenzie andrewdavidmackenzie changed the title Use std:\:thread::available_parallelism() instead of num_cpus dependency Use std:: thread:: available_parallelism() instead of num_cpus dependency May 16, 2022
@cuviper
Copy link
Member

cuviper commented May 17, 2022

  • what is the minimum version of rust that you want to support

We have an RFC for that, setting policy for at least one year. Right now that would be 1.52, while we're currently still sitting on the last MSRV bump to 1.36.

We could autocfg this change in the build script, but we can't make the num_cpus dependency conditional that way.

  • what is the best default value when the call to get parallelism fails? I chose "1".

Yeah, that's what num_cpus does if it fails internally.

@andrewdavidmackenzie
Copy link
Contributor Author

OK, quite a while to wait then.
Feel free to close the PR, or not, as you see fit.
I had removed num_cpu dependency from my project and then saw it was still there via rayon, so thought I'd take a shot at getting it removed totally.

@adamreichold
Copy link
Collaborator

adamreichold commented May 17, 2022 •
edited
Loading

I just wanted to add that AFAIK, std's implementation does not consider cgroups (as of Rust 1.59) in contrast to num_cpus. So there still is a functional difference between the two options.

This seems especially relevant as Rayon-based programs will often aim to saturate the detected number of hardware threads so this sounds like it could lead to significant oversubscription if cgroups are involved.

@cuviper
Copy link
Member

cuviper commented May 17, 2022

That's true -- cgroup support is coming in 1.61, per rust-lang/rust#92697.

@lukehsiao
Copy link

Just updating here that it has landed now that 1.61 is released :).

@link2xt link2xt mentioned this pull request Jun 2, 2022
Closed
@grego
Copy link

grego commented Jun 7, 2022

What about making the num_cpus dependency optional?

@cuviper
Copy link
Member

cuviper commented Jun 9, 2022

That cgroup support was only for cgroupv2, and this caused a regression for cgroupv1 users when Cargo switched from num_cpus, which they're now reverting. See rust-lang/rust#97549.

What about making the num_cpus dependency optional?

Then we would have one thread if the feature is not enabled? That's not great...
(I mind that less for errors since that's an unlikely runtime condition.)

@grego
Copy link

grego commented Jun 9, 2022 •
edited
Loading

I meant the available_parallelism function would be used with num_cpus disabled.

@cuviper
Copy link
Member

cuviper commented Nov 15, 2022

Just to note, Rust 1.64.0 added cgroupv1 support in rust-lang/rust#97925.

@tatsuya6502 tatsuya6502 mentioned this pull request Apr 23, 2023
Closed
@cuviper
Copy link
Member

cuviper commented Sep 18, 2023

Going by our policy, 1.64.0 will be a year old tomorrow, so we can merge this as long as I don't publish too soon. :)

However, I think I'll leave the strict MSRV at 1.63 (from #948) anyway, because that keeps us compatible with the system rustc on Debian 12 (bookworm), where AFAICS they use cgroups-v2 by default anyway.

bors r+

bors bot added a commit that referenced this pull request Sep 18, 2023
@bors @andrewdavidmackenzie @cuviper
Merge #937
cb848b1 
937: Use std:: thread:: available_parallelism() instead of num_cpus dependency r=cuviper a=andrewdavidmackenzie

From rust 1.59 we have a call for available_parallelism() in the std library and we can remove the dependency on num_cpu.

Open questions for maintainers:
- what is the minimum version of rust that you want to support
- what is the best default value when the call to get parallelism fails? I chose "1".

Co-authored-by: Andrew Mackenzie <andrew@mackenzie-serres.net>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 18, 2023

Canceled.

@cuviper
Copy link
Member

cuviper commented Sep 18, 2023

(fixed a typo in docs)

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 18, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 43868a3 into rayon-rs:master Sep 18, 2023
3 checks passed
@qryxip qryxip mentioned this pull request Mar 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andrewdavidmackenzie @cuviper @adamreichold @lukehsiao @grego