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

Improve the CLOCK API #442

Closed
samoht opened this Issue Aug 10, 2015 · 35 comments

Comments

Projects
None yet
6 participants
@samoht
Member

samoht commented Aug 10, 2015

Now that we have:

we should use them.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 10, 2015

Contributor

The idea was to redefine CLOCK, a clock representing "calendar" time to

module type CLOCK = sig
  val now_d_ps : unit -> int * int64
  (** [now_d_ps ()] is [(d, ps)] representing the POSIX time occuring
      at [d] * 86'400e12 + [ps] POSIX picoseconds from the epoch
      1970-01-01 00:00:00 UTC. [ps] is in the range
      \[[0];[86_399_999_999_999_999L]\]. *)

  val period_d_ps : unit -> (int * int64) option
  (** [period_d_ps ()] is if available [Some (d, ps)] representing the
      clock's picosecond period [d] * 86'400e12 + [ps]. [ps] is in the
      range \[[0];[86_399_999_999_999_999L]\]. *)

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

dbuenzli commented Aug 10, 2015

The idea was to redefine CLOCK, a clock representing "calendar" time to

module type CLOCK = sig
  val now_d_ps : unit -> int * int64
  (** [now_d_ps ()] is [(d, ps)] representing the POSIX time occuring
      at [d] * 86'400e12 + [ps] POSIX picoseconds from the epoch
      1970-01-01 00:00:00 UTC. [ps] is in the range
      \[[0];[86_399_999_999_999_999L]\]. *)

  val period_d_ps : unit -> (int * int64) option
  (** [period_d_ps ()] is if available [Some (d, ps)] representing the
      clock's picosecond period [d] * 86'400e12 + [ps]. [ps] is in the
      range \[[0];[86_399_999_999_999_999L]\]. *)

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

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Aug 12, 2015

Contributor

Would it be better to redefine CLOCK (and therefore mirage-types) in terms of Ptime directly?

eg.

module type CLOCK = sig
  val now_d_ps : unit -> Ptime.t
  (** etc **)
end

Then it would be usable directly eg. like this:

Ptime.to_rfc3339 (CLOCK.now_d_ps ())

Or am I missing something about how the module defined in CLOCK will be consumed by the "end-user" unikernel author?

This is based on a quick bit of experimentation I did into adding the int * int64 signature, which you can see here: mattgray@46be4aa and mattgray/mirage-clock@801e609

I'll try and work up an example of a CLOCK using Ptime.t when I get a chance.

Contributor

mattgray commented Aug 12, 2015

Would it be better to redefine CLOCK (and therefore mirage-types) in terms of Ptime directly?

eg.

module type CLOCK = sig
  val now_d_ps : unit -> Ptime.t
  (** etc **)
end

Then it would be usable directly eg. like this:

Ptime.to_rfc3339 (CLOCK.now_d_ps ())

Or am I missing something about how the module defined in CLOCK will be consumed by the "end-user" unikernel author?

This is based on a quick bit of experimentation I did into adding the int * int64 signature, which you can see here: mattgray@46be4aa and mattgray/mirage-clock@801e609

I'll try and work up an example of a CLOCK using Ptime.t when I get a chance.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 12, 2015

Contributor

You don't want CLOCK to depend on ptime.

Contributor

dbuenzli commented Aug 12, 2015

You don't want CLOCK to depend on ptime.

@avsm

This comment has been minimized.

Show comment
Hide comment
@avsm

avsm Aug 12, 2015

Member

@mattgray the CLOCK module type should ideally be independent of any concrete dependencies. Libraries that use it can impose their own library structures on top of it, such as mapping Ptime.t to the types that CLOCK exposes.

Member

avsm commented Aug 12, 2015

@mattgray the CLOCK module type should ideally be independent of any concrete dependencies. Libraries that use it can impose their own library structures on top of it, such as mapping Ptime.t to the types that CLOCK exposes.

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Aug 15, 2015

Contributor

So, as discussed in the meeting on Wednesday, I'm going to have a go at implementing this suggested change. I'm going to tackle it roughly as follows..

First of all add the interface suggested by @dbuenzli above alongside the existing CLOCK interface, and get it working with mirage-clock-{unix,xen} - using more or less the same bindings / OCaml implementation based on Unix that we currently have - this will probably mean hardcoding period_d_ps to None to begin with.

After that, I'll look at altering the bindings in mirage-platform and the use of Unix.gettimeofday in mirage-clock-unix so that we can expose nanosecond times as well as the clock resolution where available (using clock_gettime ?)

Contributor

mattgray commented Aug 15, 2015

So, as discussed in the meeting on Wednesday, I'm going to have a go at implementing this suggested change. I'm going to tackle it roughly as follows..

First of all add the interface suggested by @dbuenzli above alongside the existing CLOCK interface, and get it working with mirage-clock-{unix,xen} - using more or less the same bindings / OCaml implementation based on Unix that we currently have - this will probably mean hardcoding period_d_ps to None to begin with.

After that, I'll look at altering the bindings in mirage-platform and the use of Unix.gettimeofday in mirage-clock-unix so that we can expose nanosecond times as well as the clock resolution where available (using clock_gettime ?)

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Aug 17, 2015

Contributor

Should the CLOCK type (or another type in mirage.ml) also contain a way to retrieve monotonic wall-clock time (currently slightly hidden in OS.Time.Monotonic)? That way it would be more discoverable and serve to illustrate the differences between monotonic time and the "calendar" time that CLOCK currently has. We could have now_monotonic_d_ps etc

Contributor

mattgray commented Aug 17, 2015

Should the CLOCK type (or another type in mirage.ml) also contain a way to retrieve monotonic wall-clock time (currently slightly hidden in OS.Time.Monotonic)? That way it would be more discoverable and serve to illustrate the differences between monotonic time and the "calendar" time that CLOCK currently has. We could have now_monotonic_d_ps etc

@avsm

This comment has been minimized.

Show comment
Hide comment
@avsm

avsm Aug 17, 2015

Member

Not sure -- this would mean that every backend needs to expose a mechanism for retrieving monotonic time (e.g. JS).

Member

avsm commented Aug 17, 2015

Not sure -- this would mean that every backend needs to expose a mechanism for retrieving monotonic time (e.g. JS).

@samoht

This comment has been minimized.

Show comment
Hide comment
@samoht

samoht Aug 17, 2015

Member

I think adding a signature for monotonic clock makes sense. For JS we won't be able to use some implementation because there is no monotonic clock but that's fine. btw, see some new time signatures proposed by @dbuenzli here: https://github.com/dbuenzli/tick/blob/master/src/tick.mli would be nice to check how easy/hard it is to implement and use these signatures in practice.

Member

samoht commented Aug 17, 2015

I think adding a signature for monotonic clock makes sense. For JS we won't be able to use some implementation because there is no monotonic clock but that's fine. btw, see some new time signatures proposed by @dbuenzli here: https://github.com/dbuenzli/tick/blob/master/src/tick.mli would be nice to check how easy/hard it is to implement and use these signatures in practice.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Aug 17, 2015

Contributor

For JS we won't be able to use some implementation because there is no monotonic clock but that's fine.

It's perfectly possible to get monotonic time for JavaScript. See mtime's platform support.

As for the interface I would rather suggest to use a cheaper representation than the one used by PCLOCK. For example this.

Contributor

dbuenzli commented Aug 17, 2015

For JS we won't be able to use some implementation because there is no monotonic clock but that's fine.

It's perfectly possible to get monotonic time for JavaScript. See mtime's platform support.

As for the interface I would rather suggest to use a cheaper representation than the one used by PCLOCK. For example this.

@avsm

This comment has been minimized.

Show comment
Hide comment
@avsm

avsm Aug 17, 2015

Member

It's perfectly possible to get monotonic time for JavaScript. See mtime's platform support.

Great! No objections from me then.

Member

avsm commented Aug 17, 2015

It's perfectly possible to get monotonic time for JavaScript. See mtime's platform support.

Great! No objections from me then.

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Sep 15, 2015

Contributor

So, I've been (very slowly) looking at implementing these interfaces and have reached the conclusion that implementing current_tz_offset_s on Xen isn't possible, as Mini-OS doesn't anywhere (that I can see) expose any timezone information from the host to the guest. Possibly it isn't provided at all by Xen. In any case it might not be desirable to have the guest timezone tied to the host.

I think the best way to provide local time offsets from UTC is via a library that parses the IANA zoneinfo files. This library needn't be Mirage specific, it would just need a way to provide the zoneinfo files via KV_RO. There is this module in Core: https://github.com/janestreet/core/blob/master/src/zone.mli

Would be interested in hearing if anyone has an alternative idea for implementing local time offsets.

I'll continue with adding the clock period to PCLOCK, and implementing MCLOCK

Contributor

mattgray commented Sep 15, 2015

So, I've been (very slowly) looking at implementing these interfaces and have reached the conclusion that implementing current_tz_offset_s on Xen isn't possible, as Mini-OS doesn't anywhere (that I can see) expose any timezone information from the host to the guest. Possibly it isn't provided at all by Xen. In any case it might not be desirable to have the guest timezone tied to the host.

I think the best way to provide local time offsets from UTC is via a library that parses the IANA zoneinfo files. This library needn't be Mirage specific, it would just need a way to provide the zoneinfo files via KV_RO. There is this module in Core: https://github.com/janestreet/core/blob/master/src/zone.mli

Would be interested in hearing if anyone has an alternative idea for implementing local time offsets.

I'll continue with adding the clock period to PCLOCK, and implementing MCLOCK

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Sep 15, 2015

Contributor

reached the conclusion that implementing current_tz_offset_s on Xen isn't possible

If there's no way of getting that from the VM you can always return 0 for now. For the unix backend you can have a look at here.

I think the best way to provide local time offsets from UTC is via a library that parses the IANA zoneinfo files.

That wouldn't help you in determining what's the system or user notion of local time (and you want to stay away from the tz database).

Contributor

dbuenzli commented Sep 15, 2015

reached the conclusion that implementing current_tz_offset_s on Xen isn't possible

If there's no way of getting that from the VM you can always return 0 for now. For the unix backend you can have a look at here.

I think the best way to provide local time offsets from UTC is via a library that parses the IANA zoneinfo files.

That wouldn't help you in determining what's the system or user notion of local time (and you want to stay away from the tz database).

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Sep 15, 2015

Contributor

Btw I vaguely remember @avsm mentioning that this could be stored in xenstore I don't know much about all that stuff works but it seems that there's a timeoffset key mentioned on this page.

Contributor

dbuenzli commented Sep 15, 2015

Btw I vaguely remember @avsm mentioning that this could be stored in xenstore I don't know much about all that stuff works but it seems that there's a timeoffset key mentioned on this page.

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Sep 16, 2015

Contributor

If there's no way of getting that from the VM you can always return 0 for now. For the unix backend you can have a look at here

For the unix backend I've already implemented it here using Unix.mktime, which interprets the tm value in the local timezone, and then returning the difference between the UTC and local timestamps. I think this has the same result but I'd like some other opinions on it.

Btw I vaguely remember @avsm mentioning that this could be stored in xenstore I don't know much about all that stuff works but it seems that there's a timeoffset key mentioned on this page.

From reading that page along with HVM timeoffsets and talking to a colleague who's worked with Xenstore, it seems that the timeoffset key in Xenstore is internal-only and only present for HVM guests. It's purpose is to allow Windows guests to actually change the value of the emulated RTC, which is how Windows deals with timezone changes. Unless @avsm was referring to a different key in Xenstore.

In any case, I'll press on with the rest of the API, and return 0 for the time offset in Xen guests as you suggest.

Contributor

mattgray commented Sep 16, 2015

If there's no way of getting that from the VM you can always return 0 for now. For the unix backend you can have a look at here

For the unix backend I've already implemented it here using Unix.mktime, which interprets the tm value in the local timezone, and then returning the difference between the UTC and local timestamps. I think this has the same result but I'd like some other opinions on it.

Btw I vaguely remember @avsm mentioning that this could be stored in xenstore I don't know much about all that stuff works but it seems that there's a timeoffset key mentioned on this page.

From reading that page along with HVM timeoffsets and talking to a colleague who's worked with Xenstore, it seems that the timeoffset key in Xenstore is internal-only and only present for HVM guests. It's purpose is to allow Windows guests to actually change the value of the emulated RTC, which is how Windows deals with timezone changes. Unless @avsm was referring to a different key in Xenstore.

In any case, I'll press on with the rest of the API, and return 0 for the time offset in Xen guests as you suggest.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Sep 16, 2015

Contributor

For the unix backend I've already implemented it here using Unix.mktime, which interprets the tm value in the local timezone, and then returning the difference between the UTC and local timestamps. I think this has the same result but I'd like some other opinions on it.

This seems more clever than what I was doing...

Contributor

dbuenzli commented Sep 16, 2015

For the unix backend I've already implemented it here using Unix.mktime, which interprets the tm value in the local timezone, and then returning the difference between the UTC and local timestamps. I think this has the same result but I'd like some other opinions on it.

This seems more clever than what I was doing...

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Oct 13, 2015

Contributor

So, I've implemented everything for PCLOCK (although the new functions are still on CLOCK, I'm leaving the module refactoring til last because it requires fixing all the dependencies)

You can see the signature changes here: master...mattgray:new-clock-api

and the implementation here: mirage/mirage-clock@master...mattgray:new-clock-api

Please let me know if this is heading in the right direction... In particular:

  • I'm not happy with how period_d_ps is implemented:
    • On unix and macosx now_d_ps goes through Unix.gettimeofday as before, so it's not explicitly tied to the clock for which we wish to find the resolution. Should now_d_ps be a binding to the relevant posix / mach call eg. clock_gettime?
    • on OS X it reports that the clock period is 10ms, which does not seem to be the case if one looks at two consecutive calls to either Unix.gettimeofday or some C that calls the underlying mach code directly. Either this value is wrong or incorrectly documented, or I've made some drastic units error somewhere and ended up 10million times off the correct value...
    • On Xen the period is hardcoded to 1us, see the comment for my justification for this.
  • I've started building mirage-clock-{unix,xen} with an Oasis generated build system, as they use cstubs now and it just seemed easier to use Oasis.
  • Xen has no way to tell the UTC offset (see previous discussion). It's currently hardcoded to 0, but maybe this should return an int option to indicate that the offset cannot be determined?
Contributor

mattgray commented Oct 13, 2015

So, I've implemented everything for PCLOCK (although the new functions are still on CLOCK, I'm leaving the module refactoring til last because it requires fixing all the dependencies)

You can see the signature changes here: master...mattgray:new-clock-api

and the implementation here: mirage/mirage-clock@master...mattgray:new-clock-api

Please let me know if this is heading in the right direction... In particular:

  • I'm not happy with how period_d_ps is implemented:
    • On unix and macosx now_d_ps goes through Unix.gettimeofday as before, so it's not explicitly tied to the clock for which we wish to find the resolution. Should now_d_ps be a binding to the relevant posix / mach call eg. clock_gettime?
    • on OS X it reports that the clock period is 10ms, which does not seem to be the case if one looks at two consecutive calls to either Unix.gettimeofday or some C that calls the underlying mach code directly. Either this value is wrong or incorrectly documented, or I've made some drastic units error somewhere and ended up 10million times off the correct value...
    • On Xen the period is hardcoded to 1us, see the comment for my justification for this.
  • I've started building mirage-clock-{unix,xen} with an Oasis generated build system, as they use cstubs now and it just seemed easier to use Oasis.
  • Xen has no way to tell the UTC offset (see previous discussion). It's currently hardcoded to 0, but maybe this should return an int option to indicate that the offset cannot be determined?
@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Oct 13, 2015

Contributor

See also here for a small unikernel that exercises the new API https://github.com/mattgray/mirage-new-clock-api-test/

Contributor

mattgray commented Oct 13, 2015

See also here for a small unikernel that exercises the new API https://github.com/mattgray/mirage-new-clock-api-test/

@haesbaert

This comment has been minimized.

Show comment
Hide comment
@haesbaert

haesbaert Oct 13, 2015

Member

On Tuesday, 13 October 2015, Matt Gray notifications@github.com wrote:

So, I've implemented everything for PCLOCK (although the new functions are
still on CLOCK, I'm leaving the module refactoring til last because it
requires fixing all the dependencies)

You can see the signature changes here: master...mattgray:new-clock-api
master...mattgray:new-clock-api

and the implementation here: mirage/mirage-clock@
master...mattgray:new-clock-api
mirage/mirage-clock@master...mattgray:new-clock-api

Please let me know if this is heading in the right direction... In
particular:

  • I'm not happy with how period_d_ps is implemented:
    • On unix and macosx now_d_ps goes through Unix.gettimeofday as
      before, so it's not explicitly tied to the clock for which we get the
      resolution. Should now_d_ps be a binding to the relevant posix /
      mach call eg. clock_gettime?
    • on OS X it reports that the clock period is 10ms, which does not
      seem to be the case if one looks at two consecutive calls to either
      Unix.gettimeofday or some C that calls the underlying mach code
      directly. Either this value is wrong or incorrectly documented, or I've
      made some drastic units error somewhere and ended up 10million times off
      the correct value...

Im jumping on the boat without much info so I might be totally off.

Historically bsds use the rtc clock for gettimeofday and the lapic clock
for ticks, they are independent sources, so a 1000hz lapic clock would
produce the effects you see in clock_foo. Gettiimeofday cant use the lapic
clock since it wants higher resolution.

    • On Xen the period is hardcoded to 1us, see the comment for my
      justification for this.
  • I've started building mirage-clock-{unix,xen} with an Oasis
    generated build system, as they use cstubs now and it just seemed easier to
    use Oasis.
  • Xen has no way to tell the UTC offset (see previous discussion).
    It's currently hardcoded to 0, but maybe this should return an int
    option to indicate that the offset cannot be determined?


Reply to this email directly or view it on GitHub
#442 (comment).

Member

haesbaert commented Oct 13, 2015

On Tuesday, 13 October 2015, Matt Gray notifications@github.com wrote:

So, I've implemented everything for PCLOCK (although the new functions are
still on CLOCK, I'm leaving the module refactoring til last because it
requires fixing all the dependencies)

You can see the signature changes here: master...mattgray:new-clock-api
master...mattgray:new-clock-api

and the implementation here: mirage/mirage-clock@
master...mattgray:new-clock-api
mirage/mirage-clock@master...mattgray:new-clock-api

Please let me know if this is heading in the right direction... In
particular:

  • I'm not happy with how period_d_ps is implemented:
    • On unix and macosx now_d_ps goes through Unix.gettimeofday as
      before, so it's not explicitly tied to the clock for which we get the
      resolution. Should now_d_ps be a binding to the relevant posix /
      mach call eg. clock_gettime?
    • on OS X it reports that the clock period is 10ms, which does not
      seem to be the case if one looks at two consecutive calls to either
      Unix.gettimeofday or some C that calls the underlying mach code
      directly. Either this value is wrong or incorrectly documented, or I've
      made some drastic units error somewhere and ended up 10million times off
      the correct value...

Im jumping on the boat without much info so I might be totally off.

Historically bsds use the rtc clock for gettimeofday and the lapic clock
for ticks, they are independent sources, so a 1000hz lapic clock would
produce the effects you see in clock_foo. Gettiimeofday cant use the lapic
clock since it wants higher resolution.

    • On Xen the period is hardcoded to 1us, see the comment for my
      justification for this.
  • I've started building mirage-clock-{unix,xen} with an Oasis
    generated build system, as they use cstubs now and it just seemed easier to
    use Oasis.
  • Xen has no way to tell the UTC offset (see previous discussion).
    It's currently hardcoded to 0, but maybe this should return an int
    option to indicate that the offset cannot be determined?


Reply to this email directly or view it on GitHub
#442 (comment).

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Oct 13, 2015

Contributor

Please let me know if this is heading in the right direction... In particular:

Doesn't seem to go backward, in contrast to POSIX time.

On unix and macosx now_d_ps goes through Unix.gettimeofday as before, so it's not explicitly tied to the clock for which we wish to find the resolution. Should now_d_ps be a binding to the relevant posix / mach call eg. clock_gettime?

One thing for sure is that if you use gettimeofday you should not pretend that the resolution is the one you get throught clock_getres as there absolutely no standardized link between the two.

I've started building mirage-clock-{unix,xen} with an Oasis generated build system, as they use cstubs now and it just seemed easier to use Oasis.

Using a C stub is also easy using plain ocamlbuild (see for example here). But then I'm not the owner of the repo so I'll let them comment on this.

Xen has no way to tell the UTC offset (see previous discussion). It's currently hardcoded to 0, but maybe this should return an int option to indicate that the offset cannot be determined?

I think this may be a good idea as there is actually a convention to record this in RFC 3339 time stamps. At time moment Ptime uses Z if you don't specify any time zone but this is very easy to change to enforce the convention.

Contributor

dbuenzli commented Oct 13, 2015

Please let me know if this is heading in the right direction... In particular:

Doesn't seem to go backward, in contrast to POSIX time.

On unix and macosx now_d_ps goes through Unix.gettimeofday as before, so it's not explicitly tied to the clock for which we wish to find the resolution. Should now_d_ps be a binding to the relevant posix / mach call eg. clock_gettime?

One thing for sure is that if you use gettimeofday you should not pretend that the resolution is the one you get throught clock_getres as there absolutely no standardized link between the two.

I've started building mirage-clock-{unix,xen} with an Oasis generated build system, as they use cstubs now and it just seemed easier to use Oasis.

Using a C stub is also easy using plain ocamlbuild (see for example here). But then I'm not the owner of the repo so I'll let them comment on this.

Xen has no way to tell the UTC offset (see previous discussion). It's currently hardcoded to 0, but maybe this should return an int option to indicate that the offset cannot be determined?

I think this may be a good idea as there is actually a convention to record this in RFC 3339 time stamps. At time moment Ptime uses Z if you don't specify any time zone but this is very easy to change to enforce the convention.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Oct 14, 2015

Contributor

Should now_d_ps be a binding to the relevant posix / mach call eg. clock_gettime?

That's what I would do for Ptime's OS dependent module. On darwin you could try clock_get_calendar_nanotime. There doesn't seem to be a function to return the actual clock period so I guess you should simply return 1ns.

Contributor

dbuenzli commented Oct 14, 2015

Should now_d_ps be a binding to the relevant posix / mach call eg. clock_gettime?

That's what I would do for Ptime's OS dependent module. On darwin you could try clock_get_calendar_nanotime. There doesn't seem to be a function to return the actual clock period so I guess you should simply return 1ns.

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Oct 28, 2015

Contributor

One thing for sure is that if you use gettimeofday you should not pretend that the resolution is the one you get throught clock_getres as there absolutely no standardized link between the two.

I've made some more changes here to address this.

On linux clock_gettime and clock_getres are used. On Darwin and Xen gettimeofday is used, and the clock period is hardcoded to 1 microsecond.

On darwin you could try clock_get_calendar_nanotime

I looked into this and it seems to be for kernel extensions only, hence falling back to gettimeofday on Darwin.

Contributor

mattgray commented Oct 28, 2015

One thing for sure is that if you use gettimeofday you should not pretend that the resolution is the one you get throught clock_getres as there absolutely no standardized link between the two.

I've made some more changes here to address this.

On linux clock_gettime and clock_getres are used. On Darwin and Xen gettimeofday is used, and the clock period is hardcoded to 1 microsecond.

On darwin you could try clock_get_calendar_nanotime

I looked into this and it seems to be for kernel extensions only, hence falling back to gettimeofday on Darwin.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Oct 28, 2015

Contributor

On Darwin and Xen gettimeofday is used, and the clock period is hardcoded to 1 microsecond.

I think it's preferable to return None, the resolution of gettimeofday is unspecified by specification.

Contributor

dbuenzli commented Oct 28, 2015

On Darwin and Xen gettimeofday is used, and the clock period is hardcoded to 1 microsecond.

I think it's preferable to return None, the resolution of gettimeofday is unspecified by specification.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 22, 2015

Contributor

For the unix backend I've already implemented it here using Unix.mktime, which interprets the tm value in the local timezone, and then returning the difference between the UTC and local timestamps. I think this has the same result but I'd like some other opinions on it.

@mattgray I was reviewing your technique for computing the timezone offset in the Ptime_clock module to be distributed with Ptime and I had hard time figuring out what you were doing since you get the right values while making the wrong symbolic computation (which should be tzoff = local - utc); your code is pretending to do tzoff = utc - local. In fact I pretend and try to show below that what you call ts_local is not the local time.

Here's the understanding I derive from the code, let us define the following two self-describing types:

type 'a time constraint 'a = [ `UTC | `Local ] (* timestamp *)
type 'a date constraint 'a = [ `UTC | `Local ] (* datetime *) 

This allows us to describe the unix functions used from a timezone perspective as follows:

val gettimeofday : unit -> [`UTC] time
val gmtime : [`UTC] time -> [`UTC] date
val mktime : [`Local] date -> [`UTC] time

Using the above functions the meaning of your algorithm is roughly rewritten as follows:

let current_timezone_offset () =
  let t_utc = gettimeofday () in
  let d_utc = gmtime t_utc in
  let alpha = mktime d_utc in
  t_utc - alpha

Now let the following values all represent the same point in time:

val t_utc   : [`UTC] time
val d_utc   : [`UTC] date
val t_local : [`Local] time  
val d_local : [`Local] date

and tzoff be the timezone offset to compute. With ad-hoc polymorphism for arithmetic on the various combination of types mentioned above I would say that the only sound interpetation is that your algorithm assumes the following:

(1) tzoff = t_local - t_utc
(2) tzoff = d_local - d_utc
(3) mktime d_local = t_utc
(4) mktime (d_local - tzoff) = (mktime d_local) - tzoff

Now applying equational reasoning and expanding definitions we get for the result of your function:

  t_utc - alpha
= t_utc - (mktime d_utc)             by definition of alpha
= t_utc - (mktime (d_local - tzoff)) by (2)
= t_utc - ((mktime d_local) - tzoff) by (4)
= t_utc - (t_utc - tzoff)            by (3)
= tzoff                              by arithmetic

This means that the name ts_local of your variable is very confusing since it's absolutely not what it represents.

Since (3) holds by specification of mktime what we need to be sure for you algorithm to work is that (4) always holds. I have been trying to think when this could not be the case but can't make a case in my mind (also maybe (4) can be reformulated in a way that makes it easier to grasp that it is correct). Just out of curiosity how did you come up with this technique ? While I like it's simplicity in contrast to mine with the tricky year wrap, I have to admit that I find it much less obvious to convince myself that is really correct; maybe I'm missing something obvious.

Also since all this timezone stuff is always utterly confusing we should make the documentation of current_tz_offset_s clearer by making the equation explicit. I would suggest:

val current_tz_offset_s : unit -> int option
(** [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. *)

Finally you should put the right copyright attributions and dates in the source files, your name in the implementation of the module (including the C stubs file were they are missing) and mine in the interface.

Contributor

dbuenzli commented Dec 22, 2015

For the unix backend I've already implemented it here using Unix.mktime, which interprets the tm value in the local timezone, and then returning the difference between the UTC and local timestamps. I think this has the same result but I'd like some other opinions on it.

@mattgray I was reviewing your technique for computing the timezone offset in the Ptime_clock module to be distributed with Ptime and I had hard time figuring out what you were doing since you get the right values while making the wrong symbolic computation (which should be tzoff = local - utc); your code is pretending to do tzoff = utc - local. In fact I pretend and try to show below that what you call ts_local is not the local time.

Here's the understanding I derive from the code, let us define the following two self-describing types:

type 'a time constraint 'a = [ `UTC | `Local ] (* timestamp *)
type 'a date constraint 'a = [ `UTC | `Local ] (* datetime *) 

This allows us to describe the unix functions used from a timezone perspective as follows:

val gettimeofday : unit -> [`UTC] time
val gmtime : [`UTC] time -> [`UTC] date
val mktime : [`Local] date -> [`UTC] time

Using the above functions the meaning of your algorithm is roughly rewritten as follows:

let current_timezone_offset () =
  let t_utc = gettimeofday () in
  let d_utc = gmtime t_utc in
  let alpha = mktime d_utc in
  t_utc - alpha

Now let the following values all represent the same point in time:

val t_utc   : [`UTC] time
val d_utc   : [`UTC] date
val t_local : [`Local] time  
val d_local : [`Local] date

and tzoff be the timezone offset to compute. With ad-hoc polymorphism for arithmetic on the various combination of types mentioned above I would say that the only sound interpetation is that your algorithm assumes the following:

(1) tzoff = t_local - t_utc
(2) tzoff = d_local - d_utc
(3) mktime d_local = t_utc
(4) mktime (d_local - tzoff) = (mktime d_local) - tzoff

Now applying equational reasoning and expanding definitions we get for the result of your function:

  t_utc - alpha
= t_utc - (mktime d_utc)             by definition of alpha
= t_utc - (mktime (d_local - tzoff)) by (2)
= t_utc - ((mktime d_local) - tzoff) by (4)
= t_utc - (t_utc - tzoff)            by (3)
= tzoff                              by arithmetic

This means that the name ts_local of your variable is very confusing since it's absolutely not what it represents.

Since (3) holds by specification of mktime what we need to be sure for you algorithm to work is that (4) always holds. I have been trying to think when this could not be the case but can't make a case in my mind (also maybe (4) can be reformulated in a way that makes it easier to grasp that it is correct). Just out of curiosity how did you come up with this technique ? While I like it's simplicity in contrast to mine with the tricky year wrap, I have to admit that I find it much less obvious to convince myself that is really correct; maybe I'm missing something obvious.

Also since all this timezone stuff is always utterly confusing we should make the documentation of current_tz_offset_s clearer by making the equation explicit. I would suggest:

val current_tz_offset_s : unit -> int option
(** [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. *)

Finally you should put the right copyright attributions and dates in the source files, your name in the implementation of the module (including the C stubs file were they are missing) and mine in the interface.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 22, 2015

Contributor

maybe I'm missing something obvious.

Oh but in fact this is simply a way to find the coefficient of a function that is known to simply subtract a coefficient. We have:

val f : float -> t
val g : t -> float

and we know that g is such that:

g (f x) = x - c

so to find c we just do x - (g (f x)) = x - (x - c) = c. So in fact solving for 0. you can even avoid the call to gettimeofday and simply write:

let current_time_zone_offset () = 
  let tm = Unix.gmtime 0. in
  - (int_of_float (fst (Unix.mktime tm)))
Contributor

dbuenzli commented Dec 22, 2015

maybe I'm missing something obvious.

Oh but in fact this is simply a way to find the coefficient of a function that is known to simply subtract a coefficient. We have:

val f : float -> t
val g : t -> float

and we know that g is such that:

g (f x) = x - c

so to find c we just do x - (g (f x)) = x - (x - c) = c. So in fact solving for 0. you can even avoid the call to gettimeofday and simply write:

let current_time_zone_offset () = 
  let tm = Unix.gmtime 0. in
  - (int_of_float (fst (Unix.mktime tm)))
@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 23, 2015

Contributor

So in fact solving for 0. you can even avoid the call to gettimeofday and simply write:

Except don't, it seems unreliable on Darwin, while it doesn't seem to be if you solve against the current value of gettimeofday.

Contributor

dbuenzli commented Dec 23, 2015

So in fact solving for 0. you can even avoid the call to gettimeofday and simply write:

Except don't, it seems unreliable on Darwin, while it doesn't seem to be if you solve against the current value of gettimeofday.

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Dec 24, 2015

Contributor

Hi @dbuenzli, and thank you for the very thorough analysis.

You are quite right, I followed through your workings and ts_local / alpha is actually something like "what would the UTC time be if it were the current UTC date in the local timezone". Which is a pretty confusing idea and I'm not sure if I can convince myself that it will always yield the correct offset. I prefer the longer version, which with a bit of working through is pretty easy to understand.

So in fact solving for 0. you can even avoid the call to gettimeofday and simply write:

let current_time_zone_offset () = 
  let tm = Unix.gmtime 0. in
  - (int_of_float (fst (Unix.mktime tm)))

Except don't, it seems unreliable on Darwin, while it doesn't seem to be if you solve against the current value of gettimeofday.

Intuitively this can't work, because the current timezone offset must depend on the current time, due to things like daylight saving time and changes to timezones over the years. Aside: although currently we are close to Jan 1 and therefore I would expect that calculation to give the correct timezone offset in the UK (where I am), it's actually wrong (gives +3600) because the UK kept daylight savings time from Oct 1968 until March 1971 (https://en.wikipedia.org/wiki/British_Summer_Time#Periods_of_deviation)

Finally you should put the right copyright attributions and dates in the source files, your name in the implementation of the module (including the C stubs file were they are missing) and mine in the interface.

Yes, I'll fix all these. I've been intending to fix them all up in one go, but really I should be updating the attributions and dates as I go along.

Contributor

mattgray commented Dec 24, 2015

Hi @dbuenzli, and thank you for the very thorough analysis.

You are quite right, I followed through your workings and ts_local / alpha is actually something like "what would the UTC time be if it were the current UTC date in the local timezone". Which is a pretty confusing idea and I'm not sure if I can convince myself that it will always yield the correct offset. I prefer the longer version, which with a bit of working through is pretty easy to understand.

So in fact solving for 0. you can even avoid the call to gettimeofday and simply write:

let current_time_zone_offset () = 
  let tm = Unix.gmtime 0. in
  - (int_of_float (fst (Unix.mktime tm)))

Except don't, it seems unreliable on Darwin, while it doesn't seem to be if you solve against the current value of gettimeofday.

Intuitively this can't work, because the current timezone offset must depend on the current time, due to things like daylight saving time and changes to timezones over the years. Aside: although currently we are close to Jan 1 and therefore I would expect that calculation to give the correct timezone offset in the UK (where I am), it's actually wrong (gives +3600) because the UK kept daylight savings time from Oct 1968 until March 1971 (https://en.wikipedia.org/wiki/British_Summer_Time#Periods_of_deviation)

Finally you should put the right copyright attributions and dates in the source files, your name in the implementation of the module (including the C stubs file were they are missing) and mine in the interface.

Yes, I'll fix all these. I've been intending to fix them all up in one go, but really I should be updating the attributions and dates as I go along.

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Dec 24, 2015

Contributor

I'm now not sure that we should even be including current_time_zone_offset or anything timezone related in this module. First of all, we can't supply anything meaningful in Xen for this value. And we also then force any future implementations of PCLOCK to provide time zone info, even thought the different clock sources that an implementor might use have no concept of timezone internally. It seems like a similar argument for removing CLOCK.gmtime (See mirage/mirage-clock#1 (comment)). And many applications (logging springs to mind) require UTC posix time only.

Contributor

mattgray commented Dec 24, 2015

I'm now not sure that we should even be including current_time_zone_offset or anything timezone related in this module. First of all, we can't supply anything meaningful in Xen for this value. And we also then force any future implementations of PCLOCK to provide time zone info, even thought the different clock sources that an implementor might use have no concept of timezone internally. It seems like a similar argument for removing CLOCK.gmtime (See mirage/mirage-clock#1 (comment)). And many applications (logging springs to mind) require UTC posix time only.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 24, 2015

Contributor

You are quite right, I followed through your workings and ts_local / alpha is actually something like "what would the UTC time be if it were the current UTC date in the local timezone". Which is a pretty confusing idea and I'm not sure if I can convince myself that it will always yield the correct offset. I prefer the longer version, which with a bit of working through is pretty easy to understand.

Funny is that I eventually switched to your version in Ptime_clock, but I think you are right I'll switch back to my earlier technique.

Another thing I found while reviewing ptime's documentation. Is that the documentation of current_tz_offset_s as suggested above is confusing. We should not talk about "the clock's current time zone offset" it may lead people to think that now_d_ps returns time stamps in local time. We should make it clearer that the time zone offset is something entirely separate.

I improved the documentation of the signature see here for a rendering (you may have to reload). You can c&p it from there.

Contributor

dbuenzli commented Dec 24, 2015

You are quite right, I followed through your workings and ts_local / alpha is actually something like "what would the UTC time be if it were the current UTC date in the local timezone". Which is a pretty confusing idea and I'm not sure if I can convince myself that it will always yield the correct offset. I prefer the longer version, which with a bit of working through is pretty easy to understand.

Funny is that I eventually switched to your version in Ptime_clock, but I think you are right I'll switch back to my earlier technique.

Another thing I found while reviewing ptime's documentation. Is that the documentation of current_tz_offset_s as suggested above is confusing. We should not talk about "the clock's current time zone offset" it may lead people to think that now_d_ps returns time stamps in local time. We should make it clearer that the time zone offset is something entirely separate.

I improved the documentation of the signature see here for a rendering (you may have to reload). You can c&p it from there.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 24, 2015

Contributor

I'm now not sure that we should even be including current_time_zone_offset or anything timezone related in this module.

I still think it's a good thing to have. Those who don't have the information can simply always return [None] which is really not an implementation burden.

Having the timezone information is useful for example when sending email, it can give contextual information to the recipient about when the person did send his message from his perspective.

Contributor

dbuenzli commented Dec 24, 2015

I'm now not sure that we should even be including current_time_zone_offset or anything timezone related in this module.

I still think it's a good thing to have. Those who don't have the information can simply always return [None] which is really not an implementation burden.

Having the timezone information is useful for example when sending email, it can give contextual information to the recipient about when the person did send his message from his perspective.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 24, 2015

Contributor

Intuitively this can't work, because the current timezone offset must depend on the current time, due to things like daylight saving time and changes to timezones over the years. Aside: although currently we are close to Jan 1 and therefore I would expect that calculation to give the correct timezone offset in the UK (where I am), it's actually wrong (gives +3600) because the UK kept daylight savings time from Oct 1968 until March 1971 (https://en.wikipedia.org/wiki/British_Summer_Time#Periods_of_deviation)

I thought that gmtime wouldn't bother redo all the history and I suspect it doesnt, I tried with OCaml's birth date in the 90's and it still gives +3600. On Darwin it seems something rather racy I swear I saw it give the right answer +0 in some cases.

Contributor

dbuenzli commented Dec 24, 2015

Intuitively this can't work, because the current timezone offset must depend on the current time, due to things like daylight saving time and changes to timezones over the years. Aside: although currently we are close to Jan 1 and therefore I would expect that calculation to give the correct timezone offset in the UK (where I am), it's actually wrong (gives +3600) because the UK kept daylight savings time from Oct 1968 until March 1971 (https://en.wikipedia.org/wiki/British_Summer_Time#Periods_of_deviation)

I thought that gmtime wouldn't bother redo all the history and I suspect it doesnt, I tried with OCaml's birth date in the 90's and it still gives +3600. On Darwin it seems something rather racy I swear I saw it give the right answer +0 in some cases.

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm May 9, 2016

Member

@mattgray any chance now that functoria is merged and 2.9.0 is out that you have a PR for mirage-types and mirage-clock to get it reviewed+merged?

Member

hannesm commented May 9, 2016

@mattgray any chance now that functoria is merged and 2.9.0 is out that you have a PR for mirage-types and mirage-clock to get it reviewed+merged?

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray May 9, 2016

Contributor

hi @hannesm - I don't have a PR ready right now, but I think everything is in place now, so I should be able to whip one up soon enough. The main blocker here is finding to to work on it around day job. I hope I can spend a bit of time on it this coming weekend, how does that sound?

Contributor

mattgray commented May 9, 2016

hi @hannesm - I don't have a PR ready right now, but I think everything is in place now, so I should be able to whip one up soon enough. The main blocker here is finding to to work on it around day job. I hope I can spend a bit of time on it this coming weekend, how does that sound?

@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm May 9, 2016

Member

@mattgray thx, it sounds excellent! :)

Member

hannesm commented May 9, 2016

@mattgray thx, it sounds excellent! :)

@mattgray

This comment has been minimized.

Show comment
Hide comment
@mattgray

mattgray Jun 17, 2016

Contributor

here you are @hannesm - I haven't removed the CLOCK code, so this does not (yet) break compatibility

Contributor

mattgray commented Jun 17, 2016

here you are @hannesm - I haven't removed the CLOCK code, so this does not (yet) break compatibility

@yomimono yomimono referenced this issue Jul 27, 2016

Closed

pending items for consideration for Mirage 3.0 #571

9 of 9 tasks complete
@hannesm

This comment has been minimized.

Show comment
Hide comment
@hannesm

hannesm Sep 27, 2016

Member

thanks to @mattgray this is fixed now (and will be part of the 3.0 release really soon now)! :)

Member

hannesm commented Sep 27, 2016

thanks to @mattgray this is fixed now (and will be part of the 3.0 release really soon now)! :)

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