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.

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

Switch to available_parallelism for auto-configuration #212

Closed
ecton opened this issue Feb 24, 2022 · 6 comments
Closed

Switch to available_parallelism for auto-configuration #212

ecton opened this issue Feb 24, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@ecton
Copy link
Member

ecton commented Feb 24, 2022

Via @daxpedda in Discord, a new api has been stabilized in Rust 1.59 that allows querying the "available parallelism":

https://doc.rust-lang.org/stable/std/thread/fn.available_parallelism.html

We could replace the sysinfo dependency with this new API for querying the CPU count. We have to update the MSRV to use this.

@ecton ecton added the enhancement New feature or request label Feb 24, 2022
@ecton ecton added this to the v1.0 milestone Feb 24, 2022
@ecton ecton added this to To do in Khonsu Labs Roadmap via automation Feb 24, 2022
@hamiltop
Copy link

README needs to be updated to reflect 1.59, it still says 1.58 is the minimum rust.

@ecton
Copy link
Member Author

ecton commented Mar 28, 2022

README needs to be updated to reflect 1.59, it still says 1.58 is the minimum rust.

This isn't implemented yet, and the minimum version hasn't officially been updated yet. The currently released version builds on 1.58 (just tested), and the current branch builds with 1.58 unless you enable the test-util feature flag.

@hamiltop
Copy link

Ah, ok. Running the test suite failed on 1.58 because of available_parallelism. I guess I misunderstood the README. Is there a documented minimum version for developing on bonsaidb (including running tests)?

@ecton
Copy link
Member Author

ecton commented Mar 28, 2022

Honestly, because of the lack of CI testing the current MSRV, I hadn't noticed it yet. Thank you for bringing it to my attention. Testing the MSRV is something I have on my list to check before doing a release, but it's also nice to know about the issue ahead of time.

In the process of working on adding the blocking unit test suite, I reached for that out of "not wanting to add another dependency right now", and it just slipped my mind ever since. I most likely will be changing it to num_cpus since its already a dependency in upstream crates, which will revert the requirement change.

I'll try to get MSRV CI hooked up soon. Thank you for checking out BonsaiDb, and apologies for the friction when checking it out!

ecton added a commit that referenced this issue Mar 29, 2022
@ecton
Copy link
Member Author

ecton commented Mar 29, 2022

I've pushed the change to revert the MSRV back to 1.58.

@ecton
Copy link
Member Author

ecton commented Apr 4, 2022

After further investigation, I've decided to not use available_parallelism. num_cpus supports some features that available_parallelism doesn't support, such as cgroups.

@ecton ecton closed this as completed Apr 4, 2022
Khonsu Labs Roadmap automation moved this from To do to Done Apr 4, 2022
@ecton ecton removed this from the v1.0 milestone Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

2 participants