ocaml-multicore / eio

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

Eio.Workpool #584

Closed SGrondin closed 10 months ago

SGrondin commented 1 year ago

For your consideration, this PR is an invitation to discuss the user-facing API and the implementation of a Eio.Workpool module.

Let's start with some general "guiding principles" that underpin the current design. I'll gladly make changes to the design if the maintainers invalidate any of these principles.

Principles

P1: A workpool should not pay attention to recommended_domain_count. Users know their own workloads (we don't). The users will tell us how many domains to use.

P2: We need to support m concurrent jobs per domain. An efficient design is one that fully utilizes each core. For CPU-bound workloads, it means 1 job per thread. For IO-bound it's a lot more. There's also hybrid workloads, with CPU-demanding processing interspersed between IO calls. For those workloads, the right number is 1/p where p is the proportion of each job being CPU-bound.

P3: The user knows the right moment to start and shutdown their workpools. They want the threads to be ready to go by the time the jobs come flying in. Having to spawn threads on the fly (lazily) should be avoided.

Tests

We've got them! ~They test durations using monotonic clocks to validate that certain tasks execute concurrently. Despite it being an obvious race condition, the tests are solid and consistent, not flaky at all. I hope they'll behave that way in CI testing too.~ The tests are now fully deterministic using mock clocks, mock domains and the mock backend.

Caveats

This PR uses Fiber.n_any which is only added as part of #587 so it obviously has to wait until the other one is merged.


The code is fairly short and I've added plenty of comments to help the reviewers. I'll trim some comments once it's been reviewed.

Thank you for your time.

SGrondin commented 1 year ago

I just made a few tweaks: fixed a race condition and removed the Core-style ~f named arguments since Eio doesn't normally label lambdas.

SGrondin commented 1 year ago

I'm marking it as Ready For Review since I've fixed the race condition (using Fiber.n_any which is only available as part of #587)

SGrondin commented 1 year ago

Is there a way to test this under dscheck? I looked over in tests/dscheck and I didn't see any use of domains or env, making me think it may not be realistic to do so.

talex5 commented 1 year ago

Instead, you can use the mock domain manager to make the tests deterministic. That currently lives in network.md, but it's generally useful and should be moved to the eio.mock library.

I've moved it out in #610.

SGrondin commented 1 year ago

Thanks for the thorough review. I've either made the requested change or left a comment/question above. I'm now working on using a mock clock in tests.

Edit: The tests are now 100% deterministic! Mock clocks, mock domains, mock backend.

SGrondin commented 10 months ago

Closing in favor of #639