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

TIME: use an int64 for sleep #547

Merged
merged 1 commit into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@hannesm
Member

hannesm commented Jun 16, 2016

I got derailed while wanting to adapt the RANDOM module type with the TIME module type.

Its documentation:

  val sleep: float -> unit io
  (** [sleep nsec] Block the current thread for. {b FIXME:} remove float. *)

I propose to have: val sleep : Int64 -> unit io, the amount of nanoseconds (this is what we get out of the monotonic clock on xen, and likely as well from Lwt_unix.sleep. The range will be up to 9223372036854775807L nanoseconds (106751 days).

I've PRs for (compiles + test run with 4.03 on Unix (FreeBSD)):

anyone aware of other users of (OS.)Time.sleep (our website tutorial for sure)? Any comments on this proposed API change?

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from 9619800 to c785bc2 Jun 16, 2016

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from c785bc2 to 839d970 Jun 16, 2016

val sleep: float -> unit io
(** [sleep nsec] Block the current thread for. {b FIXME:} remove float. *)
val sleep: int64 -> unit io
(** [sleep n] Block the current thread for n nanoseconds. *)

This comment has been minimized.

@dbuenzli

dbuenzli Jun 16, 2016

Contributor

Signed ? Unsigned ? What happens with a negative value ?

This comment has been minimized.

@hannesm

hannesm Jun 16, 2016

Member

signed (no feelings about it, but that's what I can rely on using Caml's Int64).

Behaviour for negative values is implementation-defined - I couldn't find the source for lwt, but our xen implementation skips a negative sleep.

This comment has been minimized.

@dbuenzli

dbuenzli Jun 17, 2016

Contributor

Behaviour for negative values is implementation-defined

Please no, raise Invalid_argument or make it zero but implementation defined is a path you don't want to get into.

This comment has been minimized.

@hannesm

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from 839d970 to d42401b Jun 17, 2016

@yomimono

This comment has been minimized.

Member

yomimono commented Jun 17, 2016

Could we also provide convenience functions like sleep_microseconds, sleep_milliseconds, sleep_seconds? I often want to sleep for durations like that.

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 17, 2016

I don't see any benefit. If you need micro/milli/seconds often enough locally, you can write your own local functions. If it is popular enough, develop a small (pure) library providing these functions. If we embed it into TIME, every implementation (xen/unix/solo5) has to have the same boilerplate. Also, you'll need to define what happens at the boundaries (would a sleep_milliseconds get an int64? should it then loop if it exceeds the native nanoseconds interface? using an int would prevent it from having the same domain (you can express more sleep in the nanosecond case than in an int milliseconds one); and those cases should be handled uniformly by all backends (thus, as said, I'd prefer a simple and pure library).

I initially switched to milliseconds, but when I disovered that internally nanoseconds are used anyways, and everything is converted into a int64, We should not hide this from the user, but instead expose the raw interface to avoid unnecessary conversions.

@talex5

This comment has been minimized.

Contributor

talex5 commented Jun 17, 2016

Hmm, this will break just about every Mirage program :-( And hiding the unit in the comment makes mistakes very likely (e.g. ms vs us vs ns).

What's wrong with float seconds here? Seconds are the SI units for time, which is what we want, and floats give more precision for small values, which is also what we want.

@dbuenzli

This comment has been minimized.

Contributor

dbuenzli commented Jun 17, 2016

And hiding the unit in the comment makes mistakes very likely (e.g. ms vs us vs ns).

+1

What's wrong with float seconds here? Seconds are the SI units for time, which is what we want, and floats give more precision for small values, which is also what we want.

No, you will only be able to represent seconds up to 2^53 precisely. Most sub-second durations you may want to express will not be represented precisely.

@talex5

This comment has been minimized.

Contributor

talex5 commented Jun 17, 2016

Well, sleep isn't precise in any case. It's only a lower-bound on the actual sleep time. And since time is continuous, there's no reason to think rounding to an integer will be an improvement (unlike with e.g. money where you want to represent 0.10 exactly but don't care about 0.101).

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 17, 2016

I'm happy to rename the sleep to nanosleep to avoid confusion about orders of magnitude.

I don't consider Hmm, this will break just about every Mirage program :-( to be a problem since we're talking about a new major release of something which user base is pretty small.

I don't think float should be exposed from any reasonable systems interface: they are imprecise, the underlying hardware deals with integers anyway, they are very tricky to reason about (since basic algebraic laws do not hold). Also #203 has as goal to remove floating point numbers (make MirageOS more portable, there are various platforms without floating point operations out there), which is also happening in the revised CLOCK interface afaict.

I think that converting an amount (in the developer's head as milliseconds) to a floating point number (in seconds), just to convert it into an int64 (and scale it into nanoseconds) to perform it on a CPU sounds horrible to me. Also, note that Unix provides a sleep in seconds and a nanosleep with a struct consisting of seconds and nanoseconds, both as integral numbers.

@yomimono

This comment has been minimized.

Member

yomimono commented Jun 17, 2016

On 06/17/2016 12:00 PM, Hannes Mehnert wrote:

I'm happy to rename the |sleep| to |nanosleep| to avoid confusion
about orders of magnitude.

I'd appreciate that. The combination of that + a thin library for
conversions sounds much less prone to developer confusion to me (your
earlier points about extra boilerplate and sizes vs types being well taken).

I don't consider |Hmm, this will break just about every Mirage program
:-(| to be a problem since we're talking about a new major release of
something which user base is pretty small.

I agree with this. A major version release is a great time to break
just about every Mirage program. ;)

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from d42401b to b17cad2 Jun 17, 2016

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 17, 2016

this (and all its dependencies) now has Time.nanosleep

@dbuenzli

This comment has been minimized.

Contributor

dbuenzli commented Jun 17, 2016

this (and all its dependencies) now has Time.nanosleep

This is personal but my own conventions about this (which you have e.g. in Ptime and Mtime) is to suffix the function with the units using the SI abbreviations so I would rather call that Time.sleep_ns.

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from b17cad2 to 4b66c49 Jun 17, 2016

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 17, 2016

@dbuenzli point taken and renamed to sleep_ns. let's be consistent with the rest of the ecosystem if we make such a change.

@yomimono

This comment has been minimized.

Member

yomimono commented Jun 29, 2016

Your timing is fortuitous; I was just thinking about merging this. I'm going to submit PRs for the branches you made, unless you object, and merge this soon.

@yomimono

This comment has been minimized.

Member

yomimono commented Jun 29, 2016

@hannesm, ping me when the merge conflict here is resolved and I'll start clicking green buttons where I have the power to do so.

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 29, 2016

@yomimono this is now rebased [as are the other PRs] on top of master, and using the EXTRA_REMOTE mirage-dev (in a branch hopefully pointing to the required things)

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from 86ec8c5 to eb9a6e8 Jun 29, 2016

@hannesm

This comment has been minimized.

Member

hannesm commented Jun 29, 2016

an alternative (or adjustment) would be to introduce a type uint64 = int64 and get sleep to work with full 64 bit (needs a uint64 to float conversion for the unix backend, and further investigation for the xen backend). Discussion about getting uint32/uint64 into Caml is ongoing in mantis http://caml.inria.fr/mantis/view.php?id=7280

since it would be a type alias, the clients would not need to change luckily :)
a helper library doing the conversions might be mtime if it chooses to use uint64 instead of float.

thus, good to merge, but may be not final yet.

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from eb9a6e8 to 840119d Jul 27, 2016

@hannesm hannesm force-pushed the hannesm:sleep-nsec branch from 840119d to 05abed1 Jul 27, 2016

@hannesm

This comment has been minimized.

Member

hannesm commented Jul 27, 2016

I provide a conversion library, and rebased the PRs using this (tcpip/dns/mirage-skeleton). I also decided to treat the int64 passed in to sleep_ns as unsigned.

@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 yomimono merged commit 8e1d6f5 into mirage:master Jul 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hannesm hannesm deleted the hannesm:sleep-nsec branch Jul 28, 2016

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