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

Port from threadpool to rayon #199

Closed
notgull opened this issue Feb 27, 2023 · 3 comments · Fixed by #203
Closed

Port from threadpool to rayon #199

notgull opened this issue Feb 27, 2023 · 3 comments · Fixed by #203
Labels
enhancement New feature or request

Comments

@notgull
Copy link

notgull commented Feb 27, 2023

What can be improved or is missing?

The threadpool crate is no longer maintained. Meanwhile, rayon is not only much better maintained, but also supports using custom functions for spawning threads.

Implementation Approach

We could replace the usage of threadpool with rayon::spawn et al. Since threadpool is in the public API, this is a breaking change.

@notgull notgull added the enhancement New feature or request label Feb 27, 2023
@johannesvollmer
Copy link
Owner

johannesvollmer commented Mar 2, 2023

the initial implementation was using rayon, but it had a few limitations, so we had to roll our own. we tried to use the nice iterator based api. it's been a while, but i think these were the problems:

  • we collect the blocks from the parallel threads into one single image buffer that can can not be shared or sent across threads
  • processing blocks must be streaming, meaning there should not be a huge vector with all the blocks at any time. instead the blocks should be read from the file and written to the image buffer on the fly

of course, you can simply add rayon and not use the iterator based api. but the decision was made to not use rayon because of it's size. we only needed a thread pool, and not all of that other functionality. on the other hand, it's not good to have an unmaintained dependency.

does rayon have a mechanism to detect whether multi threading is available? that would be a feature we need, that thread pool doesn't seem to have. it seems the ThreadpoolBuilder::build() returns a result, this might be useful

@notgull
Copy link
Author

notgull commented Mar 2, 2023

of course, you can simply add rayon and not use the iterator based api. but the decision was made to not use rayon because of it's size.

You can use the rayon-core package instead, which cuts out nearly all of the cruft involved with Rayon and exposes a thread pooling API.

does rayon have a mechanism to detect whether multi threading is available?

The best strategy is probably to have a threading feature that can be disabled if the user is on a platform without multithreading (e.g. WASM). I can implement this if you want.

@johannesvollmer
Copy link
Owner

johannesvollmer commented Mar 2, 2023

Oh, good to know :) Thanks for offering your help! We're actually in the middle of discussing that in another issue, see #200

i think the developer experience is better when the crate will not crash when used out of the box. for that reason, i think it would be great to not fail at runtime, but instead automatically fallback to the single-threaded code, which is already there. as there is an automatic solution, why should we ask the users of the library to manually switch that crate feature :)

if there is another reason to add a threading feature gate, we can do it anyways, i'm just saying the crate could fallback automatically instead of crashing

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
None yet
Development

Successfully merging a pull request may close this issue.

2 participants