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 <fpi...@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@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > >
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev