-
Notifications
You must be signed in to change notification settings - Fork 214
Description
I would like to remove the threadpool dependency. Looking at threadpools repository, it has not seen a single commit in the last 5 years, and blst is the only "big" library (with more than a million downloads) that is still actively using this dependency (can be seen here). All other "big" libraries depending on threadpool are equally unmaintained projects. You could reasonably claim that threadpool is a "done" project, but the repository issues (including a potential memory leak) and the overall project status still worries me from a supply chain perspective.
I saw that there is another PR that attempts a similar thing: #203. I believe I can see both the points for and against raised in the PR. I will start by saying that I am biased here. Not because I would like blst to use rayon, but because I would like this library to remove the threadpool dependency. Let me review the issues raised in the discussion and share my point of view on them.
The first apparent concern is the number of dependencies that rayon brings. I believe we can easily reach a reasonable trade-off here. Instead of using rayon (which mostly implements "nice to use" parallel iterator interfaces) we should really be using rayon-core or crossbeam directly. rayon-core's only dependency is crossbeam, and I'm pretty sure the rayon <-> rayon-core swap can be made in the PR without any other changes (because it already does not rely on rayon features). crossbeam is totally acceptable as a dependency here. This crate already depends on crossbeam through the std library. Indeed, std::mpsc channels (this project uses sync_channel) come directly from crossbeam! See rust-lang/rust#93563.
I understand and share the concerns of having to use a global thread pool that the user can spawn anything on, but it does not have to be that way. One option would be forgoing configurability and use rayon-core::ThreadPool internally, and not spawn anything on the global thread pool. There is most likely a hybrid we could find however, such as also having method that is parameterized over rayon-core::ThreadPoolBuilder, which allows the user to configure the thread pool.
I am willing to open a PR if the maintainers believe this is a reasonable change.