eio icon indicating copy to clipboard operation
eio copied to clipboard

Add monotonic clock support

Open bikallem opened this issue 3 years ago • 4 comments

This PR adds support for monotonic clock to eio. The original clock method in Eio.Stdevn.t is now renamed to real_clock since it is in fact an encapsulation of real-time OS clock. The monotonic clock is mono_clock.

The real_clock is implemented using Ptime.t and mono_clock is implemented using Mtime.t. There is an open PR to use CLOCK_BOOTTIME as monotonic clock in Mtime (https://github.com/dbuenzli/mtime/pull/44).

We now use a common Sleep_until effect in Eio_unix.Private for both luv and uring backend.

make bench now uses mono_clock to mark start/end time. I believe this is "more" correct than using real_clock for benchmarking purposes.

In addition to Eio.Time.clock being polymorhphic, it now also features two functions to make working with time in seconds a bit convenient, to_seconds and add_seconds.

Seconds is chosen as default time unit since it seems to be the unit used by OCaml stdlib. Secondly and subjectively it seems to be a convenient unit to be thinking in terms of time when using time related functions in eio.

Fixes https://github.com/ocaml-multicore/eio/issues/229

/cc @haesbaert

bikallem avatar Sep 06 '22 10:09 bikallem

NOTE: This is a non backward compatible change. Specifically, env#clock is no longer there. Additionally, the Eio.Time.clock now also requires 'a generic parameter. Any advice on how to go about maintaining backwards compatibility would be greatly appreciated.

@talex5

bikallem avatar Sep 14 '22 10:09 bikallem

NOTE: This is a non backward compatible change. Specifically, env#clock is no longer there. Additionally, the Eio.Time.clock now also requires 'a generic parameter. Any advice on how to go about maintaining backwards compatibility would be greatly appreciated.

You can keep Stdenv.clock, but mark it deprecated (but it doesn't look like it's possible to get a compile-time warning about env#clock).

As we're not at Eio 1.0 yet, it's OK to break the API if it's difficult to avoid it, like here.

However, if you did want to avoid that it would be possible to add a new Clock module with the new API e.g.

module Clock : sig
  type 'a t = ...
  ...
end

module Time : sig
  type clock = float Clock.t
  ...
  (* deprecated functions here *)
end

talex5 avatar Sep 20 '22 09:09 talex5

I have rebased on master now.

As we're not at Eio 1.0 yet, it's OK to break the API if it's difficult to avoid it, like here.

Thanks. Yes, I would like to keep the current API set since every other option introduces un-necessary cruft.

Could you please approve/merge if there are no further objections.

bikallem avatar Sep 21 '22 09:09 bikallem

@talex5 the PR has been rebased to the latest master.

bikallem avatar Sep 28 '22 10:09 bikallem

Now #338 is merged, the remaining parts of this PR (e.g. the use of Ptime) need rebasing on top of that.

talex5 avatar Nov 15 '22 11:11 talex5

@talex5 It seems this PR has bit rotted quite a bit. Do we still want to use the actual clock to implement timeout functionality, i.e. if mono then we use mono clock, if sys then we use real/sys clock and so forth. If so, I will open another PR rather than continue with this one. Also I believe https://github.com/ocaml-multicore/ocaml-uring/pull/79 in ocaml-uring is required to implement such functionality at least in uring backend.

bikallem avatar Dec 23 '22 16:12 bikallem

Updating the benchmarks to use monotonic times sounds good.

Getting eio_linux to use the realtime clock directly would be good too, fixing this comment:

https://github.com/ocaml-multicore/eio/blob/32c26ab3133b5e51a981533cb4bb201d0eb35148/lib_eio_linux/eio_linux.ml#L1300-L1304

I don't see why that needs https://github.com/ocaml-multicore/ocaml-uring/pull/79. We can just use the new Uring.timeout with Realtime, I think.

Closing this PR and opening new ones is fine with me.

talex5 avatar Jan 06 '23 14:01 talex5

I don't see why that needs https://github.com/ocaml-multicore/ocaml-uring/pull/79. We can just use the new Uring.timeout with Realtime, I think.

My memory is a bit hazy at the moment regarding this. However, the last time I tried this PR, I believe I realized that eventually, timeout value drills down to this bit of code as well. https://github.com/ocaml-multicore/eio/blob/670e656560b1464cdef6e48481b0fd6aa240fbfd/lib_eio_linux/eio_linux.ml#L581

IIRC the clock defaults to CLOCK_MONOTONIC, in order to change/improve on that I think I opened that uring PR.

bikallem avatar Jan 06 '23 16:01 bikallem

You don't need to (and can't) use Uring.wait's timeout for real-time timeouts. You can just submit them as their own separate operations instead. I don't see what completion counts have to do with this either.

  1. Submit a Realtime timeout to the ring, just like any other operation.
  2. When it's done, Uring.wait will return and it will be processed like any other operation.
  3. If you need to cancel, send a cancel message, like cancelling other uring operations.

talex5 avatar Jan 13 '23 10:01 talex5

Closing this since it has bit rotted quite a bit now.

bikallem avatar Feb 28 '23 09:02 bikallem