[RFC]: Convert to direct-style with Eio

[patricoferris]

What's ported?

This draft PR ports parts of Irmin to https://github.com/ocaml-multicore/eio. This includes irmin, irmin-fs, irmin-pack, irmin-containers and irmin-chunk along with the tests. Most of the other libraries are not yet ported because of the effort to try and port the dependencies in particular graphql, ocaml-git and webmachine (although I think irmin-http might be on its way out).

I'm opening this draft PR to let people know of the existence of this work (in case they want to do some hacking ;)) and to also let people comment on it. It might be interesting to see if there are impacts on the benchmarks.

Some issues

One problem that has come up a few times, is Irmin's use of top-level values (even if lazy). In the Eio world this becomes a little tricky because they often need either access to some capability or a Eio.Switch.t. There is a port of irmin-watcher for example that requires the installation of a handler in order to be able to get a switch to the hooks. irmin-fs also requires something similar so that users would have to do this (see the fs unix tests):

let () =
  Eio_main.run @@ fun env ->
  Irmin_fs.run env#fs @@ fun () ->
  Irmin_watcher.run @@ fun () ->
  Irmin_test.Store.run "irmin-fs.unix" ~slow:false ~sleep:Eio_unix.sleep
       ~misc:[]
       [ (`Quick, Test_fs_unix.suite) ]

Which I'm definitely not a fan of! As pointed out in ocaml-multicore/eio#364 (comment) fiber-local storage could be used but comes with it's own problems. The Irmin_fs handler was more about getting the fs capability to the repo constructor that can only take a configuration because I wasn't sure how to do that otherwise. The only way to get a capability is if it is passed to you from Eio's standard environment.

Porting to Eio already is a massive breaking change, so perhaps some of this could be reworked to avoid all of this :)) Would love to know your thoughts?

[art-w]

Thanks a lot @patricoferris, this is fantastic work! We've rebased your branch and completed the missing libraries using lwt_eio here: https://github.com/art-w/irmin/tree/direct-style (... it got a bit messy, but all the tests are working now) Do you mind if I update your PR with the new patches? :)

[patricoferris]

Awesome! Sorry for the slow reply, please do whatever is easiest (pushing the changes here, new PR etc.)

[art-w]

Thanks a lot for your patience! I've updated your PR with all the work done by @ElectreAAS and @clecat to make irmin-pack domain-safe, updates for Eio 1.0, bugfixes etc, and rebased on top of the concurrent changes to irmin-client/server by zshipko and metanivek :)

So this PR now has feature parity with main and is pretty solidly tested... There are still issues that we need to address, but I'm hoping that we could do this in follow-up PRs as this work is getting hard to track:

• We have a couple of Irmin backends in the work that are actually using Eio for I/O (without lwt_eio), which are targetting this branch (and getting rid of the effect handlers to pass the Eio capabilities, as this doesn't really work with fibers)
• Some platform-specific issues reported by the CI on macos/freebsd and ppc64, that deserve special attention (only the OCaml 5.2~alpha1 compilation issue of libirmin has been fixed, thanks to dra27)

WDYT @samoht ? If we can merge this wip, should we add a disclaimer in the readme that the repo is migrating to Eio and that the public API will still change?

[samoht]

Could we add temporary workarounds for the failing tests? I'd be very happy to merge that PR but I also would like to keep the CI green to detect future regressions.

[art-w]

Sure, that makes sense! I made some adjustments to turn the CI green, the only thing left is adding a Codecov upload token but I have neither access to our Codecov account or to the github secrets management. Can someone lend me the accesses or configure the secret such that I'll be able to update the coverage workflow? Thanks!

[clecat]

This should be ready to merge, it will allow to go on with the next PR

[lyrm]

This is a first review to check dependencies. I caught several useless lwt dependencies. I will push a commit very soon to fix some of it.

Some of my comments and some of the following points may be addressed by the following PRs. Please let's me know if this is the case !

Pin-depends

Pin-depends should as much as possible be removed before merging.

• progress and terminal don't require a pin-depend anymore (it will be in my next commit)
• index : pinned to a merge commit. index needs a minor release to remove the pin-depend
• irmin-watcher : Will be done in PR#2336 Some work to do here. The pin is to patricoferris#eio on which is built clecat#eio's branch (and PR). Removing this pin requires merging the PR in irmin-watcher and a release of this lib

Lwt deps

Removing as many Lwt dependencies as possible now would be beneficial, especially when they are unnecessary or can be easily removed without changing a lot of code. Here is a summary of the current status of packages regarding dependency on Lwt.

Package with no more deps to Lwt

Nothing needs to be done for these packages regarding the Lwt dependency in this PR.

• irmin
• irmin-pack: see PR#2280
• irmin-pack-tools: same as irmin-pack
• irmin-chunk
• irmin-fs: see PR#2316
• irmin-tezos

Packages with a dependency on lwt

The following packages still depend on lwt. For some, it is because they use Lwt-eio, but not for all. It would be helpful to do a check-up here to ensure nothing has been forgotten and summarise the plan (or next to come PR) for these libraries.

Core packages:

• irmin-server: the signature of irmin-server still has some Lwt.t in it. Is there a plan to change it later, or does this rely too heavily on its dependencies (conduit, cohttp, etc) to change it?
• irmin-client: same as irmin-server
• irmin-cli: depends on irmin-server
• irmin-graphql: depends on graphql-lwt. No graphql-eio exists.
• irmin-git: used Lwt_eio -> No PR: what is the plan?
• libirmin : used Lwt_eio -> PR#2318
• irmin-mirage: no PR -> Is there a plan?

Facultative (as it is okay to install lwt for tests and benches) :

• irmin-test: only because metrics-unix requires Lwt (used for Metrics_gnuplot). Should it be removed?
• irmin-bench: lwt can be removed in the irmin-bench.opam file (would still depend on lwt because irmin-test depends on it)

Unrelated general comments

I could not write these comments in the review (because the code is unchanged). They require minor changes in this PR.

• odoc should be added to .opam files with {with-doc}
• src/irmin/irmin.mli : examples still use Lwt
• src/irmin/indexable_intf.ml : same (lines 39+)
• src/irmin/node_intf.ml : same (lines 124+)
• test/irmin-pack/data/version_1_large/README.md: same

Not working command. I am unsure whether these require changes or not.

• make bench does not work locally (Is it meant for the CI only ?)
• make fuzzdoes not work

[clecat]

I added your changes @lyrm, keep me in touch if you want to do additional changes

[lyrm]

Thanks! These commits only fix some minor things that I found easier to fix than to comment on. There are still comments that can be addressed now.

In addition, my review's message contains tasks and questions to be answered. I have improved it so that what to do is made clearer. Could you take care of it, please?

[clecat]

I have pushed a new commit addressing some of your feedback (mostly functions names *_lwt.

As for the rest:

• Index is pinned; a small release would help, however I believe it would be useful to fix that later, in the same manner as for irmin-watcher, as an open PR exists for eio in index here.
• For the other Lwt dependencies, I believe getting to merge the second PR changing io to use eio will fix the majority of those issues. Asking this question again once we have merged both seems like a good idea to me. However in the meantime thank you for asking that question, it will allow us to have traces of it.
• As for make bench & make fuzz, I believe they awaits files (release.md mentions examples)

[lyrm]

In brief

This PR represents a significant amount of work accumulated over several years. The CI is green, the tests are passing, and no major issues have been found during the reviews. As the goal is to move forward with the Eio migration, the minor issues identified here will be addressed in a follow-up PR. Thanks @clecat for your help with this review!

Introduction

Context of the review

We are currently working on moving Irmin to Eio by merging the corresponding PRs developed last year. This includes at least the following PRs:

• This PR#2149
• PR#2316, which improves the use of Eio in irmin-fs and introduces EIO capabilities
• PR#2280, targeting irmin-pack

Changes introduced by this PR

The goal of this PR is to make Irmin compatible with multicore OCaml using Eio.

Concretely, it:

• Migrates Irmin to direct-style using Eio
• Adds multicore safety:
  • Introduces multicore tests
  • Replaces non-atomic mutables with atomics to prevent data races, as identified by TSan

What this PR does not do

• It does not completely remove all dependencies on Lwt: some packages (e.g., irmin-git) still rely on libraries that are not Eio-compatible. In such cases, lwt-eio is used to keep the API in direct-style while internally relying on Lwt.
• It does not provide the final Eio-based implementation of Irmin. That work is continued in follow-up PRs.

Implications

• Many of the changes in this PR are mechanical (e.g., removing let*, replacing Lwt_result constructs, etc.).
• This brings Irmin to an intermediate state that will be further refined in subsequent PRs.

Findings

For this second round of review, I went through most commits one by one. Below is a summary of the small changes identified, and follow-up tasks.

Small fixes and cleanups

• src/irmin-pack/unix/dune: progress should be removed from dependencies
• src/irmin/metrics.ml: the Mutate constructor of type update_mode is unused and could pose problems in a multicore context
• src/irmin/irmin.ml: the optional argument ?lock in the function batch appears unused across all backends
• Some examples are not up to date with the new API:
  • src/irmin/irmin.mli
  • src/irmin/indexable_intf.ml (lines 39+)
  • src/irmin/node_intf.ml (lines 124+)
  • test/irmin-pack/data/version_1_large/README.md
• (Opt) Remove instances of | e -> raise e
• (Opt) Replace f () |> function with match f () with ... for uniformisation

Frightening comments

• src/irmin/watch.ml
  • line 46: (* Not sure this is right *)
• src/irmin/watch_intf.mli
  • line 85: (** A terrible hack that will need fixed... *)

Other similar comments might be present and should (probably?) be reviewed and / or removed.

Question: eio_pool

A new file, eio_pool.ml, has been introduced in src/irmin-fs/unix.
Question: Is this file redundant with Eio.Pool ?

Follow-up tasks

The following listing is for tasks that should be

• Multicore safety:
  • Review multicore tests and confirm no data races with TSan
  • Review use of Atomic.set (and make sure a compare_and_set is not needed instead)
  • Add tests for linearizability

• Review LRU changes (note: could be done by reviewing PR#2281 where the lru is fully rewritten)

• Remove pin-depends:
  • index
  • irmin-watcher: pin removal planned in PR#2336

[clecat]

Thank you for your work and the reviews