johannesvollmer / exrs

100% Safe Rust OpenEXR file library
Other
149 stars 22 forks source link

Port from threadpool to rayon #199

Closed notgull closed 1 year ago

notgull commented 1 year ago

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.

johannesvollmer commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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