EWS doesn't hate it anymore! Reviews welcome. I've been slowly integrating feedback as I've received it.
-Filip > On Nov 4, 2016, at 11:52 AM, Filip Pizlo <[email protected]> wrote: > > 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] >> <mailto:[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 >>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >> > > _______________________________________________ > webkit-dev mailing list > [email protected] > https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-dev
