Hmm, I am not entirely sure this is a good idea. In my mental model we want a thread pool for some blocking tasks, which might take "long" and thus block the entire system because of blocking all execution threads - so a separate thread pool for those actually would make sense imho. But I think you are correct that the thread pool usage was bleeding into a few places where it was not needed.

This is up for discussion though, so don't take this as the ground truth, but rather a data point.

In principle I agree, but in this specific case I think it's better to use a single solution rather than trying to combine two.

Most of the intermediate tasks need access to Tokio runtime anyway, because of how pervasive run_input_output helper is. This internally spawns an async-enabled child process, which must be spawned in the Tokio context. This requirement for Tokio child processes is introduced with Tokio 0.2, hence why it pops up now.

Currently, the blocking thread pool is mostly to perform disk I/O or to use blocking HTTP (for simplicity?). In the context of our usage, using Handle::spawn_blocking does not differ much from using a futures::executor::ThreadPool here. As the docs mention, the threads are spawned if necessary and the upper limit is high by default as this API is mostly designed for I/O-bound blocking operations rather than CPU-bound ones. This seem to be our use case; the few CPU-bound tasks we seem to be doing are:

• BLAKE3 digest (currently we do that on a single dedicated thread from the pool and it's intertwined with disk I/O)
• zstd (intertwined with disk I/O; zstd also exposes an async version we could migrate to)
• bincode serde in blocking HTTP requests (if I'm not missing anything it seems the requests/responses are small enough comparing to blocking HTTP time).

The majority of other use cases is just regular std::fs disk I/O. I suspect we could adapt that to use tokio::fs instead and let the runtime schedule disk I/O accordingly (which might be even more performant depending on the backend used).

Until (if?) we use an executor-agnostic child process support, I think simplifying the implementation here gives us more than we'd get by seemingly separating concerns when using futures::executor::ThreadPool that we'd have to combine with Tokio runtime anyway.

An alternative is to create our own executor abstraction that'd wrap Tokio-enabled IO futures that we'd spawn on the separate thread pool but poll inside the Tokio context.

I propose to (in no specific order):

• try to land the first two commits to work towards green CI (and possibly leave remaining work until we have green CI?)
• see if we can use tokio::fs in the places mentioned above to reduce the need for work in an explicit blocking pool
• Keep ThreadPool for now but use it only in the HTTP-related module to help migrate things piecewise and because it seems more cleanly separated from the rest of the code
• at some point try to rewrite HTTP client code to use async

@drahnr WDYT?

In principle I agree, but in this specific case I think it's better to use a single solution rather than trying to combine two.

Most of the intermediate tasks need access to Tokio runtime anyway, because of how pervasive run_input_output helper is. This internally spawns an async-enabled child process, which must be spawned in the Tokio context. This requirement for Tokio child processes is introduced with Tokio 0.2, hence why it pops up now.

A shait, I forgot about this tokio specific thingy.

Currently, the blocking thread pool is mostly to perform disk I/O or to use blocking HTTP (for simplicity?). In the context of our usage, using Handle::spawn_blocking does not differ much from using a futures::executor::ThreadPool here. As the docs mention, the threads are spawned if necessary and the upper limit is high by default as this API is mostly designed for I/O-bound blocking operations rather than CPU-bound ones. This seem to be our use case; the few CPU-bound tasks we seem to be doing are:

* BLAKE3 digest (currently we do that on a single dedicated thread from the pool and it's intertwined with disk I/O)

* zstd (intertwined with disk I/O; zstd also exposes an async version we could migrate to)

* bincode serde in blocking HTTP requests (if I'm not missing anything it seems the requests/responses are small enough comparing to blocking HTTP time).

The majority of other use cases is just regular std::fs disk I/O. I suspect we could adapt that to use tokio::fs instead and let the runtime schedule disk I/O accordingly (which might be even more performant depending on the backend used).

Until (if?) we use an executor-agnostic child process support, I think simplifying the implementation here gives us more than we'd get by seemingly separating concerns when using futures::executor::ThreadPool that we'd have to combine with Tokio runtime anyway.

An alternative is to create our own executor abstraction that'd wrap Tokio-enabled IO futures that we'd spawn on the separate thread pool but poll inside the Tokio context.

That sounds like quite a bit of work for not much gain right now.

* try to land the first two commits to work towards green CI (and possibly leave remaining work until we have green CI?)

Given the above, I think I want to land this as is.

* see if we can use `tokio::fs` in the places mentioned above to reduce the need for work in an explicit blocking pool

I'd defer this one until we see if we practically run into the upper thread count limit.

* Keep `ThreadPool` for now but use it only in the HTTP-related module to help migrate things piecewise and because it seems more cleanly separated from the rest of the code

* at some point try to rewrite HTTP client code to use async

I think this is a cluster, that should be migrated to tokio/hyper in one go and is on the agenda to get rid of rouille.

Fixed up 78fc70f with

diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs
index 93f995d..269ec02 100644
--- a/src/compiler/compiler.rs
+++ b/src/compiler/compiler.rs
@@ -1599,7 +1599,7 @@ LLVM version: 6.0",
                     cwd.to_path_buf(),
                     vec![],
                     CacheControl::Default,
-                    pool.clone(),
+                    pool,
                 ).await
             })
             .unwrap();

to minimize the diff (I added that mistakingly) and added the last expected test change commit 788832b with a more descriptive info as 8fbab2f

I'm merging this as per #34 (comment) and I verified locally that stuff works, as well as in the CI docker container. Left a #23 (comment), I'd be happy to resolve that separately as it doesn't block the future work of splitting the branch into digestible commits in preparation to merge either it to our main branch or upstream in some form.

@drahnr thanks for taking a look!