supranational
Rust bindings: Remove dependency on `threadpool`
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: https://github.com/supranational/blst/pull/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 https://github.com/rust-lang/rust/pull/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.
As an initial remark. In respect to "potential memory leak." Execute
fn main(){}
under valgrind. To me it says "9 allocs, 8 frees, possibly lost: 0, still reachable: non-zero." Now execute
fn main() {
rayon_core::spawn(|| { println!("hello"); });
}
under valgrind. To me it says "89 allocs, 16 frees, possibly lost: non-zero, still reachable: even-more." After adding call to sleep() the balance between allocs and frees got .... worse. To be fair, "possibly lost" amount remained the same... What I'm trying to say is that the reference to "potential memory leak" is just FUD :-) It's not threadpool, it's the way Rust is. And it's presumably all right, because it's no point in bringing application memory to a specific state prior returning it to the system that inherently doesn't care about it.
Indeed, I did not do enough research into it. I simply came across it and blindly trusted the issue author.
In the mean time I came up with an alternative. To respect the rules around feature unification and to minimize the changes that need to be made, the no-threads feature flag should be replaced with a thread feature flag that is enabled by default. In turn, this would allow threadpool to be an optional dependency.