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

[Filip Pizło]

You guys are making a convincing case for Seconds/WallTime/MonotonicTime!

-Filip

> On May 22, 2016, at 11:19 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> 
> I'm with Brady here.  In WebCore, there are enough DOM and network
> APIs that mix wall time and monotonically increasing time (e.g. there
> has been proposals to use monodically increasing time in
> event.prototype.timeStamp even though various event related code
> relies on it being a wall clock time) that having a type system would
> be helpful.
> - R. Niwa
> 
> 
>> On Sun, May 22, 2016 at 10:47 PM, Brady Eidson <beidson at apple.com> wrote:
>> 
>> On May 22, 2016, at 6:41 PM, 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?
>> 
>> 
>> In this description of the State of Time in WebKit™, I fixated on a few key
>> points:
>> 
>> 1 - When we used plain doubles, we had one class of subtle bugs.
>> 2 - When we switched to chrono, we had a different class of subtle bugs.
>> (and template creep)
>> 3 - There exists a solution - non-templated custom classes - that removes
>> both classes of subtle bugs, without the template creep.
>> 
>> The WebKit project believes in tool-based assistance (regression tests,
>> benchmarking, healthy family of scripts for both common and critical tasks,
>> etc etc)
>> 
>> And it sounds like we have the ability to enlist another tool - the compiler
>> - to prevent a very human class of bugs from creeping in to the project.
>> 
>> Since we’ve already acclimated ourselves to using a class-based solution for
>> time/durations (We use chromo already), I disagree with the pessimism about
>> the cost of using Seconds/WT/MT.
>> 
>> I think the cost to individual programmers in the future is the same as
>> using chrono, which we already agreed to do.
>> 
>> I think we should do Seconds/WT/MT.
>> 
>> Thanks,
>> ~Brady
>> 
>> 
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>