[webkit-dev] RFC: stop using std::chrono, go back to using doubles for time

Mon May 23 12:50:01 PDT 2016

I think the actual issue here was that std::chrono::duration can't
represent infinite duration and some of our code has been hacking around
that with std::chrono::duration::max(). Clock time + duration is not going
to overflow if the duration is natural (a finite constant or produced by
diffing time_points).

I have found that using Optional<std::chrono::milliseconds> in cases where
the duration legitimately can be unspecified/infinite works well. In most
cases it can't be.

If we decide to write our own time type system lets make sure it covers all
the existing use of std::chrono without losing any of the static typing.
But maybe simply disallowing std::chrono::duration::max() in the style
checker would solve the immediate problem here?

   antti

On Mon, May 23, 2016 at 4:41 AM, Filip Pizlo <fpizlo at apple.com> wrote:

> Hi everyone!
>
> I’d like us to stop using std::chrono and go back to using doubles for
> time.  First I list the things that I think we wanted to get from
> std::chrono - the reasons why we started switching to it in the first
> place.  Then I list some disadvantages of std::chrono that we've seen from
> fixing std::chrono-based code.  Finally I propose some options for how to
> use doubles for time.
>
> *Why we switched to std::chrono*
>
> A year ago we started using std::chrono for measuring time.  std::chrono
> has a rich typesystem for expressing many different kinds of time.  For
> example, you can distinguish between an absolute point in time and a
> relative time.  And you can distinguish between different units, like
> nanoseconds, milliseconds, etc.
>
> Before this, we used doubles for time.  std::chrono’s advantages over
> doubles are:
>
> *Easy to remember what unit is used:* We sometimes used doubles for
> milliseconds and sometimes for seconds.  std::chrono prevents you from
> getting the two confused.
>
> *Easy to remember what kind of clock is used:* We sometimes use the
> monotonic clock and sometimes the wall clock (aka the real time clock).
> Bad things would happen if we passed a time measured using the monotonic
> clock to functions that expected time measured using the wall clock, and
> vice-versa.  I know that I’ve made this mistake in the past, and it can be
> painful to debug.
>
> In short, std::chrono uses compile-time type checking to catch some bugs.
>
> *Disadvantages of using std::chrono*
>
> We’ve seen some problems with std::chrono, and I think that the problems
> outweigh the advantages.  std::chrono suffers from a heavily templatized
> API that results in template creep in our own internal APIs.  std::chrono’s
> default of integers without overflow protection means that math involving
> std::chrono is inherently more dangerous than math involving double.  This
> is particularly bad when we use time to speak about timeouts.
>
> *Too many templates:* std::chrono uses templates heavily.  It’s overkill
> for measuring time.  This leads to verbosity and template creep throughout
> common algorithms that take time as an argument.  For example if we use
> doubles, a method for sleeping for a second might look like
> sleepForSeconds(double).  This works even if someone wants to sleep for a
> nanoseconds, since 0.000001 is easy to represent using a double.  Also,
> multiplying or dividing a double by a small constant factor (1,000,000,000
> is small by double standards) is virtually guaranteed to avoid any loss of
> precision.  But as soon as such a utility gets std::chronified, it becomes
> a template.  This is because you cannot have
> sleepFor(std::chrono::seconds), since that wouldn’t allow you to represent
> fractions of seconds.  This brings me to my next point.
>
> *Overflow danger:* std::chrono is based on integers and its math methods
> do not support overflow protection.  This has led to serious bugs like
> https://bugs.webkit.org/show_bug.cgi?id=157924.  This cancels out the
> “remember what unit is used” benefit cited above.  It’s true that I know
> what type of time I have, but as soon as I duration_cast it to another
> unit, I may overflow.  The type system does not help!  This is insane:
> std::chrono requires you to do more work when writing multi-unit code, so
> that you satisfy the type checker, but you still have to be just as
> paranoid around multi-unit scenarios.  Forgetting that you have
> milliseconds and using it as seconds is trivially fixable.  But if
> std::chrono flags such an error and you fix it with a duration_cast (as any
> std::chrono tutorial will tell you to do), you’ve just introduced an
> unchecked overflow and such unchecked overflows are known to cause bugs
> that manifest as pages not working correctly.
>
> I think that doubles are better than std::chrono in multi-unit scenarios.
> It may be possible to have std::chrono work with doubles, but this probably
> implies us writing our own clocks.  std::chrono’s default clocks use
> integers, not doubles.  It also may be possible to teach std::chrono to do
> overflow protection, but that would make me so sad since using double means
> not having to worry about overflow at all.
>
> The overflow issue is interesting because of its implications for how we
> do timeouts.  The way to have a method with an optional timeout is to do
> one of these things:
>
> - Use 0 to mean no timeout.
> - Have one function for timeout and one for no timeout.
> - Have some form of +Inf or INT_MAX to mean no timeout.  This makes so
> much mathematical sense.
>
> WebKit takes the +Inf/INT_MAX approach.  I like this approach the best
> because it makes the most mathematical sense: not giving a timeout is
> exactly like asking for a timeout at time-like infinity.  When used with
> doubles, this Just Works.  +Inf is greater than any value and it gets
> preserved properly in math (+Inf * real = +Inf, so it survives gracefully
> in unit conversions; +Inf + real = +Inf, so it also survives
> absolute-to-relative conversions).
>
> But this doesn’t work with std::chrono.  The closest thing to +Inf is
> duration::max(), i.e. some kind of UINT_MAX, but this is guaranteed to
> overflow anytime it’s converted to a more precise unit of time
> (nanoseconds::max() converted to milliseconds is something bogus).  It
> appears that std::chrono doesn’t have a good story for infinite timeout,
> which means that anyone writing a function that can optionally have a
> timeout is going to have a bad time.  We have plenty of such functions in
> WebKit.  For example, I’m not sure how to come up with a feel-good solution
> to https://bugs.webkit.org/show_bug.cgi?id=157937 so long as we use
> std::chrono.
>
> *Going back to doubles*
>
> Considering these facts, I propose that we switch back to using doubles
> for time.  We can either simply revert to the way we used doubles before,
> or we can come up with some more sophisticated approach that blends the
> best of both worlds.  I prefer plain doubles because I love simplicity.
>
> *Simply revert to our old ways:* I like this approach the best because it
> involves only very simple changes.  Prior to std::chrono, we used a double
> to measure time in seconds.  It was understood that seconds was the default
> unit.  We would use both monotonic and wall clocks, and we used double for
> both of them.
>
> *Come up with a new type system: *Having learned from std::chrono and
> doubles, it seems that the best typesystem for time would comprise three
> classes: Seconds, WallTime, and MonotonicTime.  Seconds would be a class
> that holds a double and supports +/+=/-/-=/</<=/>/>=/==/!= operations, as
> well as conversions to a raw double for when you really need it.  WallTime
> and MonotonicTime would be wrappers for Seconds with a more limited set of
> available operations.  You can convert WallTime or MonotonicTime to Seconds
> and vice-versa, but some operators are overloaded to make casts unnecessary
> in most cases (WallTime + Seconds = WallTime, WallTime - WallTime =
> Seconds, etc).  This would save us from forgetting the unit or the clock.
> The name of the Seconds class is a dead give-away, and WallTime and
> MonotonicTime will not yield you a value that is unit-sensitive unless you
> say something like WallTime::toSeconds().  There will be no easy way to
> convert WallTime to MonotonicTime and vice-versa, since we want to
> discourage such conversions.
>
> Personally I feel very comfortable with doubles for time.  I like to put
> the word “Seconds” into variable names and function names
> (waitForSeconds(double) is a dead give-away).  On the other hand, I sort of
> like the idea of a type system to protect clock mix-ups.  I think that’s
> the biggest benefit we got from std::chrono.
>
> If it was entirely up to me, I’d go for doubles.  I think that there needs
> to be a high burden of proof for using types to catch semantic bugs.  A
> type system *will* slow you down when writing code, so the EV (expected
> value) of the time savings from bugs caught early needs to be greater than
> the EV of the time lost due to spoonfeeding the compiler or having to
> remember how to use those classes.  Although I know that using doubles
> sometimes meant we had bugs, I don’t think they were frequent or severe
> enough for the odds to be good for the Seconds/WallTime/MonotonicTime
> solution.
>
> Thoughts?
>
> -Filip
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-dev/attachments/20160523/d79eb212/attachment.html>