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.

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

Add monotonic clock support #308

Closed
wants to merge 3 commits into from
Closed

Conversation

bikallem
Copy link
Contributor

@bikallem bikallem commented Sep 6, 2022

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 (dbuenzli/mtime#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 #229

/cc @haesbaert

@bikallem
Copy link
Contributor Author

bikallem commented Sep 14, 2022

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

@talex5
Copy link
Collaborator

talex5 commented Sep 20, 2022

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

@bikallem
Copy link
Contributor Author

bikallem commented Sep 21, 2022

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.

1. Makes Time.clock polymorphic
2. Rename clock to real_clock
3. Add mono_clock
4. Change benchmarks to use monotonic clock from realtime clock
Adds Sleep_until effect to Eio_linux.Private and handles the effect in
both luv and linux backends.
@bikallem
Copy link
Contributor Author

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

talex5 added a commit to talex5/eio that referenced this pull request Oct 4, 2022
talex5 added a commit to talex5/eio that referenced this pull request Oct 4, 2022
talex5 added a commit to talex5/eio that referenced this pull request Oct 12, 2022
talex5 added a commit to talex5/eio that referenced this pull request Oct 20, 2022
talex5 added a commit to talex5/eio that referenced this pull request Nov 7, 2022
talex5 added a commit to talex5/eio that referenced this pull request Nov 15, 2022
@talex5
Copy link
Collaborator

talex5 commented Nov 15, 2022

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

@bikallem
Copy link
Contributor Author

@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 ocaml-multicore/ocaml-uring#79 in ocaml-uring is required to implement such functionality at least in uring backend.

@talex5
Copy link
Collaborator

talex5 commented Jan 6, 2023

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:

eio/lib_eio_linux/eio_linux.ml

Lines 1300 to 1304 in 32c26ab

method sleep_until time =
(* todo: use the realtime clock directly instead of converting to monotonic time.
That is needed to handle adjustments to the system clock correctly. *)
let d = time -. Unix.gettimeofday () in
Eio.Time.Mono.sleep mono_clock d

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

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

@bikallem
Copy link
Contributor Author

bikallem commented Jan 6, 2023

I don't see why that needs ocaml-multicore/ocaml-uring#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.

let result = Uring.wait ?timeout uring in

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

@talex5
Copy link
Collaborator

talex5 commented Jan 13, 2023

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.

@bikallem
Copy link
Contributor Author

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

@bikallem bikallem closed this Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monotonic Support
2 participants