[Merged by Bors] - Swap out num_cpus for std::thread::available_parallelism #4970

james7132 · 2022-06-09T04:09:34Z

Objective

As of Rust 1.59, std::thread::available_parallelism has been stabilized. As of Rust 1.61, the API matches num_cpus::get by properly handling Linux's cgroups and other sandboxing mechanisms.

As bevy does not have an established MSRV, we can replace num_cpus in bevy_tasks and reduce our dependency tree by one dep.

Solution

Replace num_cpus with std::thread::available_parallelism. Wrap it to have a fallback in the case it errors out and have it operate in the same manner as num_cpus did.

This however removes physical_core_count from the API, though we are currently not using it in any way in first-party crates.

Changelog

Changed: bevy_tasks::logical_core_count -> bevy_tasks::available_parallelism.
Removed: bevy_tasks::physical_core_count.

Migration Guide

bevy_tasks::logical_core_count and bevy_tasks::physical_core_count have been removed. logical_core_count has been replaced with bevy_tasks::available_parallelism, which works identically. If bevy_tasks::physical_core_count is required, the num_cpus crate can be used directly, as these two were just aliases for num_cpus APIs.

bjorn3 · 2022-06-09T10:37:31Z

crates/bevy_tasks/src/lib.rs

+pub fn available_parallelism() -> usize {
+    std::thread::available_parallelism()
+        .map(NonZeroUsize::get)
+        .unwrap_or(1)


I think this should log a warning when it fails.

This review comment isn't resolved yet. I'm fine with either adding the warning if it fails or a reasonable argument why it shouldn't be added.

This was mostly to keep the behavior the same as it was before (as with num_cpus). Also given that this can be called repeatedly, it could easily spam the logs made.

vacuus · 2022-06-09T11:47:17Z

I don't know how important this is for Bevy, but std::thread::available_parallelism is erroneous for "environments where the available parallelism is controlled by cgroupv1" (rust-lang/rust#97911), leading to significantly higher memory usage in such environments. num_cpus doesn't have this issue.

bjorn3 · 2022-06-09T12:20:27Z

Games generally don't run in containers that limit the amount of usable cpu. Also for bevy increasing the amount of threads doesn't increase the memory usage nearly as much as running more rustc processes at the same time.

mockersf · 2022-06-09T12:59:04Z

games don't, but a game server using Bevy could

bjorn3 · 2022-06-09T13:12:10Z

At worst that would reduce efficiency, not cause crashes due to

Also for bevy increasing the amount of threads doesn't increase the memory usage nearly as much as running more rustc processes at the same time.

For maximum efficiency I think you will have to finetune the taskpool configuration anyway including manual determination of the amount of threads of each kind to use.

cart · 2022-06-10T04:51:15Z

Maybe we should just wait to see how rust-lang/rust#97925 plays out?

james7132 · 2022-09-10T19:32:39Z

Seems like rust-lang/rust#97925 has been merged and was released as a part of 1.63, so I think the issues there have been addressed.

bjorn3

Just one comment. Looks fine otherwise.

bjorn3

forgot to push the button to actually comment 😅

bjorn3 · 2022-09-10T19:35:20Z

crates/bevy_tasks/src/lib.rs

+pub fn available_parallelism() -> usize {
+    std::thread::available_parallelism()
+        .map(NonZeroUsize::get)
+        .unwrap_or(1)


This review comment isn't resolved yet. I'm fine with either adding the warning if it fails or a reasonable argument why it shouldn't be added.

alice-i-cecile · 2022-09-19T15:45:46Z

bors r+

# Objective As of Rust 1.59, `std::thread::available_parallelism` has been stabilized. As of Rust 1.61, the API matches `num_cpus::get` by properly handling Linux's cgroups and other sandboxing mechanisms. As bevy does not have an established MSRV, we can replace `num_cpus` in `bevy_tasks` and reduce our dependency tree by one dep. ## Solution Replace `num_cpus` with `std::thread::available_parallelism`. Wrap it to have a fallback in the case it errors out and have it operate in the same manner as `num_cpus` did. This however removes `physical_core_count` from the API, though we are currently not using it in any way in first-party crates. --- ## Changelog Changed: `bevy_tasks::logical_core_count` -> `bevy_tasks::available_parallelism`. Removed: `bevy_tasks::physical_core_count`. ## Migration Guide `bevy_tasks::logical_core_count` and `bevy_tasks::physical_core_count` have been removed. `logical_core_count` has been replaced with `bevy_tasks::available_parallelism`, which works identically. If `bevy_tasks::physical_core_count` is required, the `num_cpus` crate can be used directly, as these two were just aliases for `num_cpus` APIs.

bors · 2022-09-19T16:09:38Z

Pull request successfully merged into main.

Build succeeded:

…4970) # Objective As of Rust 1.59, `std::thread::available_parallelism` has been stabilized. As of Rust 1.61, the API matches `num_cpus::get` by properly handling Linux's cgroups and other sandboxing mechanisms. As bevy does not have an established MSRV, we can replace `num_cpus` in `bevy_tasks` and reduce our dependency tree by one dep. ## Solution Replace `num_cpus` with `std::thread::available_parallelism`. Wrap it to have a fallback in the case it errors out and have it operate in the same manner as `num_cpus` did. This however removes `physical_core_count` from the API, though we are currently not using it in any way in first-party crates. --- ## Changelog Changed: `bevy_tasks::logical_core_count` -> `bevy_tasks::available_parallelism`. Removed: `bevy_tasks::physical_core_count`. ## Migration Guide `bevy_tasks::logical_core_count` and `bevy_tasks::physical_core_count` have been removed. `logical_core_count` has been replaced with `bevy_tasks::available_parallelism`, which works identically. If `bevy_tasks::physical_core_count` is required, the `num_cpus` crate can be used directly, as these two were just aliases for `num_cpus` APIs.

Swap out num_cpus for available_parallelism

a6991ea

james7132 added C-Dependencies A change to the crates that Bevy depends on A-Tasks Tools for parallel and async work labels Jun 9, 2022

james7132 changed the title ~~Swap out num_cpus for std::threadavailable_parallelism~~ Swap out num_cpus for std::thread::available_parallelism Jun 9, 2022

hymm added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 9, 2022

mockersf approved these changes Jun 9, 2022

View reviewed changes

bjorn3 reviewed Jun 9, 2022

View reviewed changes

Weibye added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Aug 10, 2022

james7132 removed the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Sep 10, 2022

james7132 requested a review from bjorn3 September 10, 2022 19:32

bjorn3 approved these changes Sep 10, 2022

View reviewed changes

bjorn3 reviewed Sep 10, 2022

View reviewed changes

hymm approved these changes Sep 14, 2022

View reviewed changes

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 14, 2022

bors bot changed the title ~~Swap out num_cpus for std::thread::available_parallelism~~ [Merged by Bors] - Swap out num_cpus for std::thread::available_parallelism Sep 19, 2022

bors bot closed this Sep 19, 2022

james7132 deleted the available_parallelism branch December 12, 2022 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Swap out num_cpus for std::thread::available_parallelism #4970

[Merged by Bors] - Swap out num_cpus for std::thread::available_parallelism #4970

james7132 commented Jun 9, 2022 •

edited

Loading

bjorn3 Jun 9, 2022

bjorn3 Sep 10, 2022

james7132 Sep 14, 2022

vacuus commented Jun 9, 2022

bjorn3 commented Jun 9, 2022

mockersf commented Jun 9, 2022

bjorn3 commented Jun 9, 2022

cart commented Jun 10, 2022

james7132 commented Sep 10, 2022

bjorn3 left a comment

bjorn3 left a comment

bjorn3 Sep 10, 2022

alice-i-cecile commented Sep 19, 2022

bors bot commented Sep 19, 2022

[Merged by Bors] - Swap out num_cpus for std::thread::available_parallelism #4970

[Merged by Bors] - Swap out num_cpus for std::thread::available_parallelism #4970

Conversation

james7132 commented Jun 9, 2022 • edited Loading

Objective

Solution

Changelog

Migration Guide

bjorn3 Jun 9, 2022

Choose a reason for hiding this comment

bjorn3 Sep 10, 2022

Choose a reason for hiding this comment

james7132 Sep 14, 2022

Choose a reason for hiding this comment

vacuus commented Jun 9, 2022

bjorn3 commented Jun 9, 2022

mockersf commented Jun 9, 2022

bjorn3 commented Jun 9, 2022

cart commented Jun 10, 2022

james7132 commented Sep 10, 2022

bjorn3 left a comment

Choose a reason for hiding this comment

bjorn3 left a comment

Choose a reason for hiding this comment

bjorn3 Sep 10, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Sep 19, 2022

bors bot commented Sep 19, 2022

james7132 commented Jun 9, 2022 •

edited

Loading