ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
529 stars 67 forks source link

Executor_pool docs #650

Closed SGrondin closed 2 months ago

SGrondin commented 7 months ago

Changes to the README:

Please let me know if any of the above causes issues. Feel free to make changes directly on this branch.

SGrondin commented 7 months ago

Thank you @Sudha247 and @balat for the comments! I've pushed a new commit with a few small changes based on your feedback.

talex5 commented 7 months ago

It's hard to see what changes with sections being moved and edited at the same time. It seems like this is combining three things:

  1. Some obvious fixes, e.g. requiring OCaml 5.1
  2. Moving things around
  3. Documenting the executor pool

It would be helpful to separate these, as 2 needs more thinking about. For example, the section introducing traceln is moved later, but all of the examples use traceln. The part about mocks could perhaps go a bit later, but the reason for having it at the front is to show an immediate benefit of passing the environment to main, as people might wonder why we're doing that.

The tracing section comes right after the fibers section so you can see visually what it looks like to run fibers concurrently. Ideally, we'd also illustrate the other examples, and readers can run the tracing tool to understand any code they write. As the tools aren't ready yet, it currently just shows how to dump the logs, which I agree isn't useful here, but this is just a place-holder while that's being written.

SGrondin commented 7 months ago

Thank you for the details, I really appreciate it because I know how annoying it can be when someone starts changing stuff without understanding why it is the way it is.

I just reverted 2. Moving things around. I'll give it another try in a separate PR that takes everything you just described into account.

Now this PR is only about the Executor_pool and any obvious little fixes I ran into. Please give it another read and let me know what you think! Thank you

talex5 commented 2 months ago

Most of this was merged in #650, so closing.