Change LRU cache and other hash tables to use kcas

[clecat]

This PR aims to change the existing hash tables of irmin and use Kcas_data hash tables instead.
This should help maintain good performances when using irmin on several domains, by replacing the existing mutexes.

[clecat]

Here are graphs from the multicore benchmarking:
Before:

After:

[art-w]

Thanks! I refreshed your PR on top of the Eio PR such that we can merge yours asap :) I'm pretty happy with the new benchmarks, as they show that multicore usage isn't slower than singlecore under contention!

Could you clean up the commit history, add an entry to the changelog, ... and take a look at the https://github.com/mirage/irmin/blob/main/src/irmin-pack/unix/lru.ml#L32 to remove the data-race there too?

[lyrm]

This PR is ready to be merged. There are however some follow-up work to do:

There is an lru cache in Irmin-pack that is an upper-layer on the Irmin.Backend.Lru. It adds a more realistic weight to the lru. In irmin, the weight is the number of objects, whereas in irmin-pack it is the actually size of the added objects (it is actually given by the user but it is meant to do that).

However, these changes are not multicore-safe. And it seems no data race are happening. So two questions:

• Should the changes in the lru cache of irmin-pack be moved to irmin ?
• No data race with irmin-pack. Does it mean:
  • we are missing the right tests for it ?
  • it is only used under a mutex and thus does not need to be multicore safe ?

[zshipko]

@lyrm I think it's possible that more testing could expose some data races, using the ocaml tsan enabled compiler to run the irmin-pack GC example shows an error that doesn't exist on the current eio branch, but i'm still trying to understand it

Maybe we should be more conservative in terms of locking/atomics then we can always revert to non-threadsafe types where anppropriate? But overall I agree this should be merged and additional changes can be made in future PRs.

[samoht]

Can you fix the CI issues before merging this?

[lyrm]

Yes, I will check it. In any case, this PR is to merge only after #2280.

[lyrm]

@lyrm I think it's possible that more testing could expose some data races, using the ocaml tsan enabled compiler to run the irmin-pack GC example shows an error that doesn't exist on the current eio branch, but i'm still trying to understand it

Maybe we should be more conservative in terms of locking/atomics then we can always revert to non-threadsafe types where anppropriate? But overall I agree this should be merged and additional changes can be made in future PRs.

Yes, I do think more tests may reveal more data races. I don't necessary agree with using atomics everywhere as they will correct data races but can still produce race conditions if wrongly uses. Mutexes would work. However, in this case, if there is a need to make Irmin_pack lru cache thread safe, the best solution is probably to move the weight management in irmin lru cache so everything is done under the protection of kcas. I will have a look today to see if this is easily feasible.

[zshipko]

I see, that sounds like a good plan! It would be nice to push as much of the synchronization stuff into kcas as possible. If that's not feasible then we can take a look at locking from a more holistic perspective.

[lyrm]

I just pushed a commit to do that. The tests pass but I am not sure they covered everything. Could you review this last commit ?

[zshipko]

The commit looks good, combining those makes a lot of sense! I do think we should to do a pass thinking about locking and synchronization across the entire system since it seems like many of these types of changes are more localized right now.

[clecat]

I have rebased this PR over the current eio branch now that the #2280 was merged.
Is it ready for merging too or do we want to change things ?

[lyrm]

It is ready to merge !