ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
540 stars 66 forks source link

Eio.Executor_pool #639

Closed SGrondin closed 9 months ago

SGrondin commented 10 months ago

This is a continuation of #584. After discussing the design, we've agreed to first merge a simple and minimalistic version. I will be adding barriers, job weights, etc., in an upcoming version 2.

talex5 commented 10 months ago

I experimented with some changes here: https://github.com/SGrondin/eio/compare/executorpool...talex5:eio:executorpool?expand=1

Needs a bit more thinking about, but this should ensure that jobs are always rejected if the pool shuts down, though it does add some cost to the fast-path.

SGrondin commented 9 months ago

I like this a lot. Compared to the previous version it guarantees that even egregious misuse never leads to a deadlock at the cost of complexity and a small overhead. The previous version did a "best effort" to avoid deadlocks in such cases. I'll merge it into my branch and play with it a bit.

SGrondin commented 9 months ago

I added more information to the mli, fixed a typo, and removed a few Fiber.yield () from the tests (fewer are needed now that we don't use Fiber.n_any). I then made use of Sync.close and recombined all those commits into one. Sync.close really cleaned everything up. It freed up a lot of "complexity budget" to use for interesting and useful advanced features. 👏 Congrats!

SGrondin commented 9 months ago

@talex5 I've made a small change in the API that I believe makes the Executor_pool more powerful without significantly increasing the complexity (both of the code and to the user). It's getting late to make this change, but I think you'll agree with it. I kept it as a separate commit to highlight it.

talex5 commented 9 months ago

What about fixing the domain capacity at 1 and having users say what fraction of the CPU they expect to use? The capacity seems arbitrary and might be hard to pick if you're sharing one pool with several parts of your program.

SGrondin commented 9 months ago

Yes, I went back and forth on that one. I'll switch it over 👍

SGrondin commented 9 months ago

@talex5 all ready

talex5 commented 9 months ago

Thanks!

(I added a final minor fix to ensure the client's promise is resolved if the worker is cancelled just as it accepts a job.)