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 <[email protected]> wrote: > > EWS Hates your patch! :-) > >> On Nov 4, 2016, at 10:01 AM, Filip Pizlo <[email protected] >> <mailto:[email protected]>> 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 <[email protected] >>> <mailto:[email protected]>> 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 >> [email protected] <mailto:[email protected]> >> https://lists.webkit.org/mailman/listinfo/webkit-dev >
_______________________________________________ webkit-dev mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-dev
