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

PCLOCK / MCLOCK - implementations of POSIX and monotonic clocks #548

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mattgray
Contributor

mattgray commented Jun 17, 2016

see #442

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 18, 2016

highly appreciated! PCLOCK should be renamed to CLOCK to replace the existing module type (this will break some dependencies)...

(** {1 Posix clock}
Clock returning time since the unix epoch. Subject to adjustment by eg. NTP.
Compatible with the Ptime library. *)

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

I don't think this line is relevant.

This comment has been minimized.

@mattgray

mattgray Jun 19, 2016

Contributor

Removed the line mentioning Ptime, however I think we should highlight the existence of the Ptime library as a convenient means of working with the timestamp representation that PCLOCK provides.

end
(** {1 Posix clock}

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

POSIX

(** {1 Posix clock}
Clock returning time since the unix epoch. Subject to adjustment by eg. NTP.

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

returning -> counting

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

eg. -> e.g.

(** {1 Monotonic clock}
Clock returning monotonic time since an arbitrary point. To be used for eg.

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

returning -> counting

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

eg. -> e.g.

module type MCLOCK = sig
val elapsed_ns : unit -> int64
(** [elapsed_ns ()] gives a monotonically increasing count in nanoseconds from

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

Not "gives", use "is", describe the value of the expression you just mentioned not the fact that a function is being called (encourages equational reasoning and thinking). See here for example.

@dbuenzli

This comment has been minimized.

Contributor

dbuenzli commented Jun 18, 2016

highly appreciated! PCLOCK should be renamed to CLOCK to replace the existing module type (this will break some dependencies)...

I think PCLOCK is a better name than CLOCK given the existence of MCLOCK.

(** Abstract type for posix clocks. *)
val pclock: pclock typ
(** Implementations of the [V1.PCLOCK] signature. *)

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

Hyperlink, this should be {!V1.PCLOCK}

(** Abstract type for monotonic clocks *)
val mclock: mclock typ
(** Implementations of the [V1.MCLOCK] signature. *)

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

Hyperlink again {!V1.MCLOCK}.

* some arbitrary point *)
val period_ns : unit -> int64 option
(** [period_ns ()] is if available [Some ns] representing the clock's

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

I think I wrote this... but the sentence is difficult to parse. Commas are missing is, if available, or maybe the whole sentence is broken.

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

As mentioned above I'd remove the if available and add a , if known. at the end of the sentence.

val current_tz_offset_s : unit -> int option
(** [current_tz_offset_s ()] is if available the clock's current local time zone
offset to UTC in seconds. *)

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

I think this should be replaced by what I mentioned in this comment. That is:

(** [current_tz_offset_s ()] is the clock's current local time zone
    offset to UTC in seconds, if known. This is the duration local time -
    UTC time in seconds. *)
offset to UTC in seconds. *)
val period_d_ps : unit -> (int * int64) option
(** [period_d_ps ()] is if available [Some (d, ps)] representing the

This comment has been minimized.

@dbuenzli

dbuenzli Jun 18, 2016

Contributor

Again that if available (see below) I would remove it from here and add at the end of sentence a , if known.

@mattgray

This comment has been minimized.

Contributor

mattgray commented Jun 19, 2016

@hannesm - I think we should keep the PCLOCK name as well, it may be obvious when reading V1.mli that there are two clock types available, but I think keeping the distinction would improve readability within a module that consume either PCLOCK or MCLOCK.

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 19, 2016

@mattgray fine with me - but we should remove CLOCK

@mattgray mattgray changed the title from PCLOCK / MCLOCK - implementations of posix and monotonic clocks to PCLOCK / MCLOCK - implementations of POSIX and monotonic clocks Jun 20, 2016

@yomimono

This comment has been minimized.

Member

yomimono commented Jun 29, 2016

I'd like to merge this, but not until we've at least got a branch of mirage-skeleton that builds against it. I'll probably have some time to straighten that out next week if nobody else can look at it before then.

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 29, 2016

@yomimono since you're the release manager, do you have a plan which changes to get in and which not (and maybe some order)? I'm mainly asking to avoid too much rebasing (e.g. #547 has branches against all (well, at least lots of) clients; and there are various local unfinished changes on my hard disk)..

@yomimono

This comment has been minimized.

Member

yomimono commented Jun 29, 2016

I think it's probably time to revive mirage-dev, loathe as I am to involve opam remote add in anything, as the set of changed packages is getting fairly large. My current plan is to use that as a 2.9.0 staging area -- merge the large PRs for libraries like this one and the necessary changes in the revdeps, then represent them there.

@mattgray

This comment has been minimized.

Contributor

mattgray commented Jun 30, 2016

I have some incomplete patchset to remove CLOCK and replace it's dependencies in tcpip and logs (so far). suggest we roll this change in with #547

@mattgray mattgray force-pushed the mattgray:new-clock-api branch from 1e56979 to 9d8602d Jul 17, 2016

@mattgray

This comment has been minimized.

Contributor

mattgray commented Jul 17, 2016

removed CLOCK (9d8602d)

@yomimono yomimono referenced this pull request Jul 27, 2016

Closed

pending items for consideration for Mirage 3.0 #571

9 of 9 tasks complete
@yomimono

This comment has been minimized.

Member

yomimono commented Aug 16, 2016

I'm testing this and mirage/mirage-clock#7 today and will probably open a new PR with a couple more changes on top of this changeset (new backends at least) soon.

@mattgray

This comment has been minimized.

Contributor

mattgray commented Aug 16, 2016

Sounds good - see comments on other PR

On Tuesday, 16 August 2016, Mindy Preston notifications@github.com wrote:

I'm testing this and mirage/mirage-clock#7
mirage/mirage-clock#7 today and will probably
open a new PR with a couple more changes on top of this changeset (new
backends at least) soon.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#548 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAnQSjrcVlQ9WHX4C-_qVhH5-PA5uAjoks5qgezrgaJpZM4I4c7j
.

@mattgray

This comment has been minimized.

Contributor

mattgray commented Aug 16, 2016

Ok so this is the branch I have some uncommitted DEVICE work to push -
wiring up the connect methods properly. I'll complete that or at least push
my WIP tomorrow

On Tuesday, 16 August 2016, Matthew Gray <matthew.thomas.gray@gmail.com
javascript:_e(%7B%7D,'cvml','matthew.thomas.gray@gmail.com');> wrote:

Sounds good - see comments on other PR

On Tuesday, 16 August 2016, Mindy Preston notifications@github.com
wrote:

I'm testing this and mirage/mirage-clock#7
mirage/mirage-clock#7 today and will probably
open a new PR with a couple more changes on top of this changeset (new
backends at least) soon.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#548 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAnQSjrcVlQ9WHX4C-_qVhH5-PA5uAjoks5qgezrgaJpZM4I4c7j
.

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 17, 2016

So for this to be mergeable:

@hannesm

This comment has been minimized.

Member

hannesm commented Aug 17, 2016

while I said somewhere tcpip should use MCLOCK, that can be done incrementally (and it is likely less error-prone to first switch from CLOCK to PCLOCK, followed by another change set to move forward to MCLOCK)

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 17, 2016

The MCLOCK interface is a much closer approximation of most of the uses of CLOCK in tcpip, IMO.

@hannesm

This comment has been minimized.

Member

hannesm commented Aug 17, 2016

if that is the case, that's brilliant! from my recent exploration of the arp implementation, I failed to adapt it to a monotonic interface (and thus thought it might be more straightforward to go via PCLOCK)

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 17, 2016

Still hacking on this (tests!), but https://github.com/yomimono/mirage-tcpip/blob/new-clock-api/lib/arpv4/arpv4.ml is my first run at adapting ARP for MCLOCK , and it was mechanical enough that I think I must have missed something important if you found it more straightforward to go with PCLOCK - can you have a look and see if something jumps out at you?

@mattgray mattgray force-pushed the mattgray:new-clock-api branch from 7004b62 to 8a9ccd1 Aug 17, 2016

@mattgray

This comment has been minimized.

Contributor

mattgray commented Aug 17, 2016

I have all of tcpip fixed for the old (non DEVICE) interface of MCLOCK - just updating it now to take the MCLOCK.t parameter

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 17, 2016

@mattgray possibly my new-clock-api branch of tcpip will be useful for you to look at. I'm revving mirage-flow right now which is needed for tests in tcpip; you can grab it at mirage/mirage-flow#19 .

@mattgray

This comment has been minimized.

Contributor

mattgray commented Aug 17, 2016

@yomimono - cool - looks pretty complete - I'm going to change my opam pin to use that, and abandon my efforts as your's removes more of the FP math than my effort. So I'll continue working through mirage-skeleton and seeing what else needs to change.

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 17, 2016

cool, glad to hear it - I'm not convinced my branch is correct as I'm just getting it to the point where tests run again, and any feedback you have would be very welcome. :)

@yomimono yomimono referenced this pull request Aug 17, 2016

Merged

New clock api #232

@mattgray

This comment has been minimized.

Contributor

mattgray commented Aug 17, 2016

mirleft/ocaml-tls#329 <- update ocaml-tls to use PCLOCK / Ptime

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 17, 2016

Tests seem to pass in tcpip. I've PR'd it so folks can look and comment there.

avsm pushed a commit to avsm/mirage-ci-logs.backup1 that referenced this pull request Aug 18, 2016

Merging with github-metadata-x
PR mirage/mirage#548
PR mirage/mirage#568
status mirage/mirage:8a9ccd186efcb96f69c3c8807e5f12b8d87872cf:continuous-integration/travis-ci/pr
status mirage/mirage:c3495be8394753e90174047b7a824f4fbbf52331:continuous-integration/travis-ci/pr

conflicts:
mirage/mirage/commit/0d510c6627835b3df792db7bc3fb2cba082de540/status/ci/pinataci/description
mirage/mirage/commit/0d510c6627835b3df792db7bc3fb2cba082de540/status/ci/pinataci/state
mirage/mirage/commit/0d510c6627835b3df792db7bc3fb2cba082de540/status/ci/pinataci/target_url
mirage/mirage/commit/581364d405e47db9eb45bef07f4f74487de40400/status/ci/pinataci/description
mirage/mirage/commit/581364d405e47db9eb45bef07f4f74487de40400/status/ci/pinataci/state
mirage/mirage/commit/581364d405e47db9eb45bef07f4f74487de40400/status/ci/pinataci/target_url
mirage/mirage/commit/5ae72049e83f402fee4b8775de9876e691c53463/status/ci/pinataci/description
mirage/mirage/commit/5ae72049e83f402fee4b8775de9876e691c53463/status/ci/pinataci/state
mirage/mirage/commit/5ae72049e83f402fee4b8775de9876e691c53463/status/ci/pinataci/target_url
mirage/mirage/commit/7004b62d5ddeacdc81f0a12bfbd8e2be5c0d546c/status/ci/pinataci/description
mirage/mirage/commit/7004b62d5ddeacdc81f0a12bfbd8e2be5c0d546c/status/ci/pinataci/state
mirage/mirage/commit/7004b62d5ddeacdc81f0a12bfbd8e2be5c0d546c/status/ci/pinataci/target_url
mirage/mirage/commit/7914ad7c6823ea7783a5fd85912294da3ed0124f/status/ci/pinataci/description
mirage/mirage/commit/7914ad7c6823ea7783a5fd85912294da3ed0124f/status/ci/pinataci/state
mirage/mirage/commit/7914ad7c6823ea7783a5fd85912294da3ed0124f/status/ci/pinataci/target_url
mirage/mirage/commit/82c1992b583720db9b34942bc1980d8733d01930/status/ci/pinataci/description
mirage/mirage/commit/82c1992b583720db9b34942bc1980d8733d01930/status/ci/pinataci/state
mirage/mirage/commit/82c1992b583720db9b34942bc1980d8733d01930/status/ci/pinataci/target_url
mirage/mirage/commit/9f190fe4dd30c9aa6a09eb60f1cb3ede752806fa/status/ci/pinataci/description
mirage/mirage/commit/9f190fe4dd30c9aa6a09eb60f1cb3ede752806fa/status/ci/pinataci/state
mirage/mirage/commit/9f190fe4dd30c9aa6a09eb60f1cb3ede752806fa/status/ci/pinataci/target_url
mirage/mirage/commit/9f4e6f1c5937bdbdfbf1e4bab029c5ab09c8e6d1/status/ci/pinataci/description
mirage/mirage/commit/9f4e6f1c5937bdbdfbf1e4bab029c5ab09c8e6d1/status/ci/pinataci/state
mirage/mirage/commit/9f4e6f1c5937bdbdfbf1e4bab029c5ab09c8e6d1/status/ci/pinataci/target_url
mirage/mirage/commit/a9a678eb81f7c18429c8ca8c7a9122e653427942/status/ci/pinataci/description
mirage/mirage/commit/a9a678eb81f7c18429c8ca8c7a9122e653427942/status/ci/pinataci/state
mirage/mirage/commit/a9a678eb81f7c18429c8ca8c7a9122e653427942/status/ci/pinataci/target_url
mirage/mirage/commit/d0d654abbca0281d3c9e72b69f91cb447de2b13a/status/ci/pinataci/description
mirage/mirage/commit/d0d654abbca0281d3c9e72b69f91cb447de2b13a/status/ci/pinataci/state
mirage/mirage/commit/d0d654abbca0281d3c9e72b69f91cb447de2b13a/status/ci/pinataci/target_url
mirage/mirage/commit/e606f487e6869aa13b1b198596989997a48821c3/status/ci/pinataci/description
mirage/mirage/commit/e606f487e6869aa13b1b198596989997a48821c3/status/ci/pinataci/state
mirage/mirage/commit/e606f487e6869aa13b1b198596989997a48821c3/status/ci/pinataci/target_url
mirage/mirage/commit/f918e6e50b18d2f2cb1ca8d6cebbd18c059cce5a/status/ci/pinataci/description
mirage/mirage/commit/f918e6e50b18d2f2cb1ca8d6cebbd18c059cce5a/status/ci/pinataci/state
mirage/mirage/commit/f918e6e50b18d2f2cb1ca8d6cebbd18c059cce5a/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/0e57ecd9a84665030ad37aae16acac138d43ba84/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/0e57ecd9a84665030ad37aae16acac138d43ba84/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/0e57ecd9a84665030ad37aae16acac138d43ba84/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/6afdb07d9613f907079a8add9051f16a672295a7/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/6afdb07d9613f907079a8add9051f16a672295a7/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/6afdb07d9613f907079a8add9051f16a672295a7/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/6fb52fef7de4bf7d3e34abd0eb3c6beea32cf2e9/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/6fb52fef7de4bf7d3e34abd0eb3c6beea32cf2e9/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/6fb52fef7de4bf7d3e34abd0eb3c6beea32cf2e9/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/a3d5c6c44a3444315defc23c4dbf4392d3b70eee/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/a3d5c6c44a3444315defc23c4dbf4392d3b70eee/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/a3d5c6c44a3444315defc23c4dbf4392d3b70eee/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/cb96f69178b7d946ae2efa6109768159f01b9828/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/cb96f69178b7d946ae2efa6109768159f01b9828/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/cb96f69178b7d946ae2efa6109768159f01b9828/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/d1027b86e13a4f3916486343cd02444c3038195e/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/d1027b86e13a4f3916486343cd02444c3038195e/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/d1027b86e13a4f3916486343cd02444c3038195e/status/ci/pinataci/target_url
mirage/ocaml-cohttp/commit/d122dec49cef870959ed17656ef70df496e9658c/status/ci/pinataci/description
mirage/ocaml-cohttp/commit/d122dec49cef870959ed17656ef70df496e9658c/status/ci/pinataci/state
mirage/ocaml-cohttp/commit/d122dec49cef870959ed17656ef70df496e9658c/status/ci/pinataci/target_url
@mattgray

This comment has been minimized.

Contributor

mattgray commented Aug 19, 2016

so, I can now build everything in https://github.com/mattgray/mirage-skeleton/tree/new-clock-api (which is a fork of https://github.com/yomimono/mirage-skeleton/tree/new-clock-api with one more commit to fix static_website_tls)

with the following set of pinned packages:

charrua-core.0.3           git  https://github.com/haesbaert/charrua-core.git
dns.0.18.1                 git  https://github.com/mirage/ocaml-dns.git#mirage-3.0
mirage.2.9.1               git  https://github.com/yomimono/mirage.git#new-clock-api
mirage-clock-unix.1.0.0    git  https://github.com/mattgray/mirage-clock.git#new-clock-api
mirage-logs.0.2            git  https://github.com/yomimono/mirage-logs.git#new-clock-api
mirage-types.2.8.0         git  https://github.com/yomimono/mirage.git#new-clock-api
mirage-unix.2.6.0          git  https://github.com/mirage/mirage-platform.git
tcpip.2.8.0                git  https://github.com/yomimono/mirage-tcpip.git#new-clock-api
tls.0.7.1                  git  https://github.com/mattgray/ocaml-tls.git#new-clock-api

I've only tried this with the unix backend, I'll try and repeat the same process for xen this weekend.

many thanks @yomimono for your efforts fixing things up - I got some of these necessary changes done at the hackday, but that was before implementing DEVICE.

@yomimono yomimono referenced this pull request Aug 22, 2016

Merged

New clock api #579

@yomimono

This comment has been minimized.

Member

yomimono commented Aug 22, 2016

#579, a followon of this PR, has been merged.

@yomimono yomimono closed this Aug 22, 2016

@avsm avsm reopened this Sep 2, 2016

@yomimono

This comment has been minimized.

Member

yomimono commented Sep 2, 2016

@avsm, why reopen this PR?

@samoht samoht closed this Sep 2, 2016

@samoht

This comment has been minimized.

Member

samoht commented Sep 2, 2016

@yomimono Datakit is having fun.

@yomimono

This comment has been minimized.

Member

yomimono commented Sep 2, 2016

@samoht I, for one, welcome our new robot overlords.

@amirmc amirmc referenced this pull request Sep 14, 2016

Closed

Tracking issue towards MirageOS 3.0 #592

29 of 29 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment