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

Fri Nov 4 11:52:58 PDT 2016

Haha, I'm fixing it!

I could use a review of the time API even while I fix some broken corners in WebCore and WK2.

-Filip

> On Nov 4, 2016, at 11:31 AM, Brent Fulgham <bfulgham at apple.com> wrote:
> 
> EWS Hates your patch! :-)
> 
>> On Nov 4, 2016, at 10:01 AM, Filip Pizlo <fpizlo at apple.com <mailto:fpizlo at apple.com>> wrote:
>> 
>> Hi everyone!
>> 
>> That last time we talked about this, there seemed to be a lot of agreement that we should go with the Seconds/MonotonicTime/WallTime approach.
>> 
>> I have implemented it: https://bugs.webkit.org/show_bug.cgi?id=152045 <https://bugs.webkit.org/show_bug.cgi?id=152045>
>> 
>> That patch just takes a subset of our time code - all of the stuff that transitively touches ParkingLot - and converts it to use the new time classes.  Reviews welcome!
>> 
>> -Filip
>> 
>> 
>> 
>>> On May 22, 2016, at 6:41 PM, Filip Pizlo <fpizlo at apple.com <mailto: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 <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 <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 <mailto: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/20161104/ad2b5baa/attachment.html>