Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Measure time with monotonic clock #5107

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Oct 11, 2017

Introduces a Time::Measure struct and Time.measure { do_something } API (as proposed in #4556 (comment)) for measuring time using a monotonic clock, unaffected by time fluctuations such as leap seconds or manually changing the computer time.

This doesn't modify the Time struct: no double clock_gettime syscalls (or gettimeofday + mach_absolute_time syscalls on macOS); no bloat of Time with seconds in both realtime and monotonic clocks, or requiring Time#- to deal with the absense of monotonic seconds (e.g. Time.new(2016, 1, 1)); ...

This assumes developers can be educated to use better API instead of relying on the dumb, bad and broken Time.now - Time.now, just like we assume that developers can be educated to use bcrypt or argon2i to hash passwords, or use Random::Secure to generate tokens.

Examples:

elapsed = Time.measure { do_something }
# => 00:00:01.171648175
timeout = 5.seconds
timer = Time::Measure.new

until timer.elapsed?(timeout)
  do_something
  p timer.elapsed
end

Supersedes #3827.

Introduces a Time::Measure struct that records a monotonic clock
after which we can compute how much time has elpased since it was
recorded, unaffected by time fluctuations such as leap-seconds or
manually changing the computer time.
@faustinoaq
Copy link
Contributor

just like we assume that developers can be educated to use bcrypt or argon2i to hash passwords, or use Random::Secure to generate tokens.

I just learnt it using Crystal, so Crystal made me more educated 👨‍🎓

@oprypin
Copy link
Member

oprypin commented Oct 11, 2017

Could we just get a Time.monotonic returning a Time::Span or even a Float64 of seconds?
https://docs.python.org/3/library/time.html#time.monotonic


...Because I think that there are many more legitimate use cases for globally monotonic time than just measuring the time difference. Besides, we already have many benchmarking tools.
I do realize that you could keep an instance of this struct and use it as an absolute reference, but that just seems like unnecessary trouble.

@oprypin
Copy link
Member

oprypin commented Oct 11, 2017

But if you do still want to go for this kind of API, please add an atomic restart feature, to avoid another bad practice.
To repeat something at an interval, people might get the elapsed time, reset the clock (and repeat), ignoring that some time passes in between those two actions.
https://www.sfml-dev.org/tutorials/2.4/system-time.php#measuring-time

@oprypin
Copy link
Member

oprypin commented Oct 11, 2017

I just gave this more thought and realized that the use case of an absolute time reference is not covered either. For example, I might want to store the creation time for each of my objects and then order them by it. There's simply no way to do this with this kind of API even if I stored this object because it produces a different value every time its method is called.

The underlying implementation is fine and there's no reason whatsoever to hide that absolute number. Just make a method returning a Time::Span and define it as returning a non-decreasing amount of time that has passed since a constant but unknown moment in the past. No need for any new types; Time::Span already supports all the operations you might want to do with this data. And don't tell me that a separate type that happens to have all the same fields makes more sense, because this is not a coincidence.

People are indeed very happy to use the subtraction idiom. It's not only popular but also very intuitive. Just explain that they want to use it on something other than Time.now.

All in all, I cannot support an API that adds a new type and 8 new methods but covers only a small fraction of use cases that one simple and familiar method can cover. So, 👎 from me.

@ysbaddaden
Copy link
Contributor Author

Since monotonic clocks only mean something when compared, I only look for measuring time here —the obvious use case. Do you have other use cases that involve using raw monotonic clocks (e.g. as a float)?

I guess a #restart method is fine, but if measuring different times I feel we can just call Time.measure {} multiple times.

Maybe there could be a Time.start to avoid the Time::Measure.new which ain't really pretty, and somewhat hide this struct:

clock = Time.start
clock.elapsed
clock.restart
clock.elapsed?(5.seconds)

Time::Span.new(seconds: seconds - @seconds, nanoseconds: nanoseconds - @nanoseconds)
end

# Returns true once *span* time has passed since the clock was created.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to use if instead of once. It feels to me like it might suggest this could be blocking until that amount of time has passed.

@straight-shoota
Copy link
Member

Maybe just Time.measure without a block instead of Time.start?

@oprypin The problem with reusing an existing concept like Time::Span or Float64 is that it looses meaning expressed in the type.
A measurement from a monotonic clock is just an arbitrary amount of time and should not be misinterpreted as a Time::Span (because the reference is unknown) or even an ordinary number.
If you want to order objects based on creation time, you measure the time from a single Time::Measure instance when an instance is created. Then you can compare these time spans (optionally converted to float if you like). Time::Measure does not represent a point in time like Time does, but a source and reference point for instances which are expressed as a time span elapsed from the reference point.

@oprypin
Copy link
Member

oprypin commented Oct 11, 2017

@straight-shoota, I have nothing to add, because my comment already addresses all you said.


5.times do
elapsed = timer.elapsed
elapsed.should be >= previous
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add previous = elapsed after this line to ensure it is monotonic? Currently, the 3rd measurement might be smaller than the second.

struct Time
# Measure elapsed time.
#
# Time measurement relies on a monotonic clock, that should be independent to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

Time measurement relies on a monotonic clock that is strictly linearly increasing.
This clock should be independent from time fluctuations, such as leap seconds, time zone adjustments or manual changes to the computer's wall time.

@RX14
Copy link
Contributor

RX14 commented Oct 12, 2017

just like we assume that developers can be educated to use bcrypt or argon2i to hash passwords, or use Random::Secure to generate tokens.

Except there's little we as language authors can do to solve these, apart from provide easy to use classes, and document that they should be used for passwords. This is an opportunity where we can do something, and so I think we should do it.

@RX14
Copy link
Contributor

RX14 commented Oct 12, 2017

And no, go has the exact same backwards compatiability constrains we have here (none for adding an API). They could have introduced a new monotonic time API just like this PR does, and that wouldn't have "broken backwards compatability". We too have plenty of shards which use Time.now - Time.now and I'm sure - regardless of this change - there will continue to be in the future. Go's decision was inspired: fix this for everyone automatically with a small addition of overhead. If people really care about Time.now performance we can introduce named args to fill out one of the other type of time.

These kind of time bugs are special, because they only really happen on leap seconds, which occur very rarely. So it's unlikely that bugs will be found until they're run in production unless he application is fuzzed for time shifts which is very unlikely and unweildy.

@ysbaddaden
Copy link
Contributor Author

@oprypin I missed your previous comment, and ... I believe you're totally right; there can be use cases to get the monotonic clock and not necessarily measure elapsed time.

The go-like alternative of mixing a monotonic and realtime clocks in a single Time.now doesn't solve this case either —and IMHO only makes internal implementation even more complex, as if dealing with timezones and DST offsets wasn't a daunting task, already.

What about simply having Time.monotonic : Time::Span and that's it? Maybe add the Time.measure(&block) : Time::Span sugar to return an elapsed time, and nothing more —for the time being.

It would be simple enough, doesn't create new types, and would be future-proof to whether we merge realtime+monotonic clocks in Time.now (or not) and it allows to access the always increasing monotonic clock for special purposes. WDYT?

@ysbaddaden ysbaddaden closed this Oct 12, 2017
@ysbaddaden ysbaddaden deleted the std-measure-time-with-monotonic-clock branch October 12, 2017 08:33
@oprypin
Copy link
Member

oprypin commented Oct 12, 2017

What about simply having Time.monotonic : Time::Span

Well I also have said basically that so I definitely agree

@straight-shoota
Copy link
Member

I must say, I was actually surprised to see Time::Measure in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants