Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Eio.Executor_pool #639

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Eio.Executor_pool #639

merged 2 commits into from
Nov 23, 2023

Conversation

SGrondin
Copy link
Collaborator

@SGrondin SGrondin commented Nov 6, 2023

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.

@SGrondin SGrondin mentioned this pull request Nov 6, 2023
Closed
@SGrondin SGrondin force-pushed the executorpool branch 3 times, most recently from e6e2550 to 9fe1f12 Compare November 6, 2023 17:34
SGrondin
SGrondin commented Nov 6, 2023
View reviewed changes
lib_eio/executor_pool.ml Outdated Show resolved Hide resolved
@talex5
Copy link
Collaborator

talex5 commented Nov 7, 2023

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.

@talex5 talex5 mentioned this pull request Nov 10, 2023
Merged
@SGrondin
Copy link
Collaborator Author

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
Copy link
Collaborator Author

SGrondin commented Nov 11, 2023 •
edited
Loading

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
Copy link
Collaborator Author

SGrondin commented Nov 12, 2023 •
edited
Loading

@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
Copy link
Collaborator

talex5 commented Nov 17, 2023

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
Copy link
Collaborator Author

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

@SGrondin SGrondin force-pushed the executorpool branch 2 times, most recently from 46c7733 to f9f1d93 Compare November 20, 2023 15:30
@SGrondin
Copy link
Collaborator Author

@talex5 all ready

@talex5 talex5 force-pushed the executorpool branch 2 times, most recently from e9b4b11 to d9fa83f Compare November 22, 2023 11:16
talex5
talex5 approved these changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a couple of minor changes (cope with weight:nan and use invalid_arg). Looks good to merge now!

lib_eio/executor_pool.ml Outdated Show resolved Hide resolved
lib_eio/executor_pool.mli Outdated Show resolved Hide resolved
@talex5
Ensure executor pool always resolves promised when cancelled
efad014 
If a worker was cancelled just as it accepted a job, the fork would fail
and the promise would remain unresolved.
@talex5 talex5 merged commit ffe6956 into ocaml-multicore:main Nov 23, 2023
3 of 4 checks passed
@talex5
Copy link
Collaborator

talex5 commented Nov 23, 2023

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.)

@talex5 talex5 mentioned this pull request Nov 23, 2023
Closed
@SGrondin SGrondin deleted the executorpool branch December 3, 2023 15:31
talex5 added a commit to talex5/opam-repository that referenced this pull request Jan 17, 2024
@talex5
[new release] eio (5 packages) (0.14)
5340789 
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
@talex5 talex5 mentioned this pull request Jan 17, 2024
Merged
talex5 added a commit to talex5/opam-repository that referenced this pull request Jan 17, 2024
@talex5
[new release] eio (5 packages) (0.14)
6eb34f3 
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
yawaramin
yawaramin reviewed Feb 22, 2024
View reviewed changes
lib_eio/executor_pool.ml

let enqueue { queue } ~weight fn =
if not (weight >= 0. && weight <= 1.) (* Handles NaN *)
then Fmt.invalid_arg "Executor_pool: weight not >= 0.0 && <= 1.0" weight
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be:

Fmt.invalid_arg "Executor_pool: weight not >= 0.0 && <= 1.0: %f" weight

?

@talex5 talex5 mentioned this pull request Feb 23, 2024
Merged
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@talex5 @nberth
[new release] eio (5 packages) (0.14)
c39875a 
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yawaramin yawaramin yawaramin left review comments

@talex5 talex5 talex5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SGrondin @talex5 @yawaramin