Add Clock.monotonic for measuring elapsed time

[ysbaddaden]

Time uses a realtime clock which is subject to jumps and fluctuations over time (e.g. leap seconds, manually changing computer's time) which is a source of bugs when measuring elapsed time.

A monotonic clock, in comparison, doesn't mean anything by itself but is guaranteed to be linearly increasing, thus useful to measure time duration more correctly. It's not perfect, since it's still affected by NTP adjustments (at least on Linux) but still better than realtime.

I propose here a new Clock type to mark the difference from Time, with sugar elapsed? and duration(&block) methods.

Achieve work for a given duration:

clock = Clock.monotonic

until clock.elapsed?(5.seconds)
  do_something
end

Measuring how long a block takes to run:

span = Clock.duration { sleep(5.1234) } # => Time::Span
span.to_f # roughly 5.1234

The clock precision is platform specific, yet all calculations are achieved through Time::Span which has a 10th of millisecond precision.

[ysbaddaden]

Note that Sierra now supports clock_gettime, but since the precision isn't as good, and more importantly it's not portable (a binary won't run on older versions) I chose to stick to mach_absolute_time.

[luislavena]

@ysbaddaden perhaps you can make that comment into the code so the knowledge of this decision is not lost in the Pull Request?

[ysbaddaden]

Done.

[ysbaddaden]

Specs are failing because 1 nanosecond didn't elapsed? Seriously? Or do I have a bug?

[asterite]

@ysbaddaden In all math operations (like -) you are going through Time::Span, which doesn't have nanosecond precision. Isn't that the bug?

I think I wouldn't relate Clock to Time::Span, or make Time::Span have nanosecond precision (I obviously prefer the second choice)

[asterite]

(though I guess the spec should pass even more, because it would give a Time::Span with value 0?)

[ysbaddaden]

I made some corrections. Like elapsed? should check for "greater than or equal" not just "greater than" and requiring "c/mach/mach_time" on Darwin.

[ysbaddaden]

Green! Now, some questions:

• What do you think of the public API?

  • Maybe Clock.new should be protected to favor the Clock.monotonic constructor?
  • Maybe seconds could be protected? It doesn't mean anything by itself; or maybe add to_f?

• What do you think of the implementation?

  • Maybe we could keep the value as a UInt64 (nanoseconds) instead of a Float64 (fractional seconds)?
  • Maybe we could use seconds directly, and have an overload for operations against Time::Span instead of the reverse?
  • Maybe Time::Span should support nanosecond precision? or a 10th of a millisecond microsecond is enough? Thought maybe 10ns or 100ns would be closer to clock precision?

[RX14]

@ysbaddaden It's a 10th of a microsecond precision , not a millisecond. There are 10,000 ticks per millisecond. That code comment was noted as wrong a long time ago, and I thought it got changed.

[RX14]

I would be in favour of this implementation storing time in ticks since an undefined epoch (instead of seconds since an undefined epoch) to keep it in sync with the other time implementations.

[ysbaddaden]

@RX14 how would you achieve that?

[RX14]

@ysbaddaden you would get nanosecond values from the OS in an Int64, then just do an integer division by 100 to get your value in ticks, which you store in the class just like Time except without the timezone stuff.

[ysbaddaden]

Ah, I wrongly read you and thought you wanted ticks since the UNIX epoch... I understand now. I tried to store an UInt64, but got some weird errors, so I scrapped it; as long as the public API is accepted, we can still refactor the internals later.

[RX14]

Technical debt accumulates over time. If we know that a better implementation exists, why merge a subpar one? I don't see any particularly urgent reason this class needs to be in the stdlib right now.

[ysbaddaden]

Granted. I just did it.

[RX14]

This should be equal to Time::Span::TicksPerSecond IMHO.

[ysbaddaden]

No, Clock and Time::Span don't have the same precision: 1ns vs 100ns.

[RX14]

I realised, just wasn't sure if there was any need for the difference. We throw away precision in Time.compute_second_and_tenth_microsecond on linux. I think it's probably for the best to keep the higher precision here however.

[ysbaddaden]

I added a commit that changes Time::Span tick precision from a 10th of microsecond to 1 nanosecond, which is obviously great for Clock.

This reduces the number of days that Time::Span can have; there are still plenty of them thought (it's maximum is now 106751.23:47:16.854775807).

An issue is that ticks for Time and Time::Span are no longer in sync (100ns vs 1ns) and I had to duplicate the calculate_ticks method. Adding a Clock::Span type would duplicate almost everything from Time::Span... so I'm not sure what's the best solution here. Maybe add nanosecond precision to Time, but I don't seem to have enough space for that.

[asterite]

@ysbaddaden Awesome!

I wouldn't mind changing the internal representation of Time (and of course Time::Span) to allow nanosecond precision. If it doesn't fit in a Int64 then lets expand it. For example we could store nano seconds in a separate variable or something like that.

LLVM also supports 128-bits integers. I know Rust added support for them some weeks ago. I tried to do the same in Crystal and it was pretty easy and worked well, I just didn't committed it or told anyone because I wanted to discuss it first... for example I don't know if 128-bits integers are supported in all platforms (if LLVM does the transforms in all of them).

In any case, if we eventually end up using 128 bits it's the same as now using two Int64s, so I'd go for it :-)

Another thing that would be nice is if Time knew its location/offset (other than just utc/local/unknown)... that's another topic to discuss, but this will also make Time bigger, and I don't think it's bad.

[ysbaddaden]

Expanding the Time struct to 128bits seems acceptable. We'd have enough space to add nanosecond precision and the TZ+DST offset.

Let's try an Int128 :-)

[RX14]

Reducing the length of a Time::Span from 30,000 years to just 200 seems like quite a drastic change. I'm fairly sure I use time spans longer than that when calculating Julian dates for predict.cr. I would be in favour of keeping Time::Span as is, but that doesn't solve the issue with Clock precision.

[asterite]

Note that adding Int128 would take some time, so I'd probably start with an implementation that just uses a couple of Int64.

[ysbaddaden]

@RX14 seriously? 😨 I guess we should have 2 ticks everywhere (seconds / nanoseconds).

[ysbaddaden]

We can have the following Time struct on 128bits, no less because of struct padding, that we take advantage of to add more properties for free. Both Clock and Time::Span would follow the same seconds + nanoseconds, which actually matches the POSIX timespec struct.

struct Time
  @seconds : Int64
  @nanoseconds : Int32
  @offset : Int16
  @kind : Kind

  enum Kind : Int8
    Unspecified = 0
    UTC = 1
    Local = 2
  end
end

[asterite]

@ysbaddaden Nice! Though probably @offset + @kind would have to go away have we should have some sort of Time::Location type (because many locations can have different offsets, and I guess each of these can have different DST rules, but I'm not sure)

[asterite]

(though I wouldn't worry about Time::Location right now... I should stop commenting this issue now to avoid further confusion :-P)

[ysbaddaden]

Yeah, TZ and DST is hard. They can change at arbitrary dates for example. It's better and recommended to store the offset itself.

I propose to keep the Kind, because there is some space, and we can know whether it's UTC or Local (e.g. London). Or maybe we don't care, it's both and that's it?

[RX14]

Go's design document on this is a very interesting read: https://github.com/golang/proposal/blob/master/design/12914-monotonic.md

It might be a good idea to do something similar so that users don't have to think whether they want monotonic time or not: they just use Time. Differences are always calculated against the monotonic clock, yet times are always representable as wall clock times.

[straight-shoota]

Now Time::Span has nanosecond precision (#5022) and I think this PR should be rebased?

While it is a really amazing solution, I really don't think Crystal should follow Go's approach to merge the concepts of monotonic and wall time into one struct (see #4556 (issue-comment)).

Based on #4556 (issue-comment) I'd suggest to rethink proper naming.
Proposals are Clock and Time::Measure. I'll throw Stopwatch into the ring as it depicts the concept palpably: A stopwatch counts time and doesn't care about timezones, leap seconds or daylight savings.

Some ideas for the public API:

# measure execution time of a block
Time.measure { do_something }
Clock.measure { do_something }
Stopwatch.measure { do_something }

# start a clock to take (multiple) measurements
x = Time.monotonic
x = Time::Measure.new
x = Time.clock
x = Time.stopwatch
x = Clock.start
x = Clock.new
x = Stopwatch.start
x = Stopwatch.new

It would also be nice if a clock could be created with an initial time span. This would allow to serialize it and create a new one with time elapsed starting from X. This would require an instance variable for the time difference. This would also allow to stop and restart a clock. But this can be added later.

[RX14]

Please make Time "just work". Go has an amazing solution which means that programmers don't have to think about monotonic clocks and can simply write the dumb code and it works. Expecting all programmers to think about monotonic code and get it correct all the time is just impractical.

[straight-shoota]

Programmers should have to think about what they're doing 😜

This approach is not just doing everything right, it also has its drawbacks. And I'm not convinced it is worth going that way without any legacy concerns. The motivation for this solution was to casually fix existing code and avoid that future code using a different feature for monotonic time wouldn't compile under Go < 1.9.

Some instances of Time will in certain circumstances behave slightly different than others. I'm not sure if there is a real issue with this, but it's intransparent and involves subtle changes in behaviour. Maybe I'm just over-skeptical, but it may have some unknown consequences... I'm wondering if there are any post-release thoughts about this for Go. Couldn't find anything right away (which should be a good sign).

Combining wall and monotonic time in the Time struct works only if it stays represented as two fields for milliseconds and nanoseconds. Having individual attributes has some niceties as well. Both design questions are related, and as not settled for anything.

And keep in mind that, for simple measurements of elapsed time, using the block method should be the preferred style anyway; for that it doesn't matter if Time has monotonic included or not. It would be worth implementing only this for now to make a monotonic clock available, while the other decisions have time to be made.
Even for multiple measurements, I'd argue clock.elapsed is more idiomatic than Time.now - start.

Crystal shouldn't be forced to Time.now measurements just because this is the only way in Ruby.
If someone uses Time.now to calculate monotonic time regardless, then be it. It's easy to spot and fix.

[ysbaddaden]

Rebased but closed anyway. We can do better :)