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

[Ryosuke Niwa]

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
>