Bug 152045

Anders, do you have ideas on the best idiom for this?

Created attachment 293572 [details] possibly solution This std::chrono nonsense has got to stop. I've had to write so much boilerplate defend-against-overflow code because of std::chrono. I don't want to do this anymore. This patch creates wrappers for doubles so that we can use doubles for time. The types are easy and have operator overloading for algebra: Seconds WallTime MonotonicTime TimeWithDynamicClockType I'm going to see how easy it is to teach existing std::chrono clients to use this instead.

I'm pretty sure I need this to do proper scheduling in the concurrent GC.

I've started thinking about what the transition plan should look like. One approach would be to teach these new classes how to convert to and from std::chrono. Another approach is to switch all clients of std::chrono over to this in one go. It's not obvious that the incremental route will be less risky, because conversions to and from std::chrono require some weirdness. Right now, I'm assuming that the Seconds/WallTime/MonotonicTime classes will use time according to CurrentTime.h. This implies a certain epoch. To convert a time according to CurrentTime.h into a time according to std::chrono means going one of three routes: (1) assume without evidence that all implementations of std::chrono and all of our ports' implementations of CurrentTime.h agree and perform the conversion directly, (2) perform the conversion using the current time as a reference (chronoTime = ourTime - ourNow + chronoNow), which means every conversion site pays two time syscalls, or (3) convert CurrentTime.h to actually use std::chrono behind the scenes to force the same time base. This assumes that all of the ports would be happy with WebKit's notion of time suddenly shifting to whatever std::chrono thinks. I think that (2) is easiest to get correct, but it risks performance regressions, which means that I will have to allow time for benchmark running to validate it. So, all of these approaches will be time consuming in their own way. On the other hand, it might be that the number of clients of std::chrono is small enough that I can simply convert them over in one go. I think that I'm going to start with that. So far it has been easy. I'll worry if my patch goes over 100kb.

Created attachment 293632 [details] patch so far It's looking unlikely that any version of this patch will be less than 100kb.

Created attachment 293673 [details] more

*** Bug 157937 has been marked as a duplicate of this bug. ***

Created attachment 293790 [details] rebased JSC is happy with this. I haven't gotten WebCore to build yet.

Created attachment 293853 [details] close to done

Attachment 293853 [details] did not pass style-queue: ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

It turns out that if I convert ParkingLot to using the new time classes without providing a mechanism for converting between std::chrono and those new classes, then I can make this work with a ~100KB patch by just converting all things that flow time into ParkingLot.

Created attachment 293892 [details] the patch

Attachment 293892 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 55 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 293892 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=293892&action=review > Source/WTF/wtf/Condition.h:51 > + // are unlikely to be affected by the cost of conversions, it is better to use MonoronicTime. MoronicTime? :) > Source/WTF/wtf/Condition.h:73 > + if (timeout < timeout.nowWithSameClock()) { I'm not sure I understand the invariants w.r.t. inf and NaN. Can they ever occur here (should it be an assert) or is it impossible by construction? e.g. can Atomic.Futex ever trick us to do something bad? It seems like this isn't under attacker control otherwise, and here is a bit late to check invariants, but I can't convince myself that all attacker-controlled times were sanitized before getting here. I guess the worst case is a hang, whereas an assert would cause a crash (probably a better outcome). > Source/WTF/wtf/Condition.h:111 > + return waitUntil(lock, MonotonicTime::now() + relativeTimeout, predicate); What would the semantics of waitFor(NaN) be? Should that be zero? Inf "just works", so do negative values. > Source/WTF/wtf/CurrentTime.cpp:343 > + fakeCondition.waitFor(fakeLock, Seconds(value)); Why not change `sleep` above to accept Seconds instead of double? > Source/WTF/wtf/MonotonicTime.h:69 > + explicit operator bool() const { return !!m_value; } I don't understand the intent of converting to bool. "if not epoch"? > Source/WTF/wtf/MonotonicTime.h:111 > + } Does it make sense to have == and != ? I think you want to use < and > only (and maybe isinf / isnan). > Source/WTF/wtf/Seconds.h:142 > + } Same here, I'm not sure about strict equality. > Source/WTF/wtf/Seconds.h:166 > + WTF_EXPORT_PRIVATE void sleep() const; I... ew... Seconds().sleep()... ew...

(In reply to comment #14) > Comment on attachment 293892 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293892&action=review > > > Source/WTF/wtf/Condition.h:51 > > + // are unlikely to be affected by the cost of conversions, it is better to use MonoronicTime. > > MoronicTime? :) That's great. > > > Source/WTF/wtf/Condition.h:73 > > + if (timeout < timeout.nowWithSameClock()) { > > I'm not sure I understand the invariants w.r.t. inf and NaN. Can they ever > occur here (should it be an assert) or is it impossible by construction? NaN could happen here if the passed timeout was NaN. I don't attempt to guarantee sensible behavior if you pass a NaN timeout, but looking at ParkingLot, it looks like it will timeout immediately because it waits while timeout.nowWithSameClock() < timeout. > e.g. can Atomic.Futex ever trick us to do something bad? AtomicsObject.wake does the right thing according to the spec. It checks for NaN explicitly. > It seems like this > isn't under attacker control otherwise, and here is a bit late to check > invariants, but I can't convince myself that all attacker-controlled times > were sanitized before getting here. I guess the worst case is a hang, > whereas an assert would cause a crash (probably a better outcome). > > > Source/WTF/wtf/Condition.h:111 > > + return waitUntil(lock, MonotonicTime::now() + relativeTimeout, predicate); > > What would the semantics of waitFor(NaN) be? Should that be zero? > Inf "just works", so do negative values. > > > Source/WTF/wtf/CurrentTime.cpp:343 > > + fakeCondition.waitFor(fakeLock, Seconds(value)); > > Why not change `sleep` above to accept Seconds instead of double? CurrentTime.h is currently for our deprecated double-based time. There's still code that uses that, and I don't want to change all of it in this patch. > > > Source/WTF/wtf/MonotonicTime.h:69 > > + explicit operator bool() const { return !!m_value; } > > I don't understand the intent of converting to bool. "if not epoch"? In code that used double for time, we use 0 to mean "I don't have a time". > > > Source/WTF/wtf/MonotonicTime.h:111 > > + } > > Does it make sense to have == and != ? I think you want to use < and > only > (and maybe isinf / isnan). It does make sense! ==/!= only become strange if you've transformed values with math that could round. We use it to compare to infinity in a few places, and it's our way of checking for NaN. > > > Source/WTF/wtf/Seconds.h:142 > > + } > > Same here, I'm not sure about strict equality. > > > Source/WTF/wtf/Seconds.h:166 > > + WTF_EXPORT_PRIVATE void sleep() const; > > I... ew... Seconds().sleep()... ew... Hahaha!

> > > Source/WTF/wtf/Condition.h:73 > > > + if (timeout < timeout.nowWithSameClock()) { > > > > I'm not sure I understand the invariants w.r.t. inf and NaN. Can they ever > > occur here (should it be an assert) or is it impossible by construction? > > NaN could happen here if the passed timeout was NaN. I don't attempt to > guarantee sensible behavior if you pass a NaN timeout, but looking at > ParkingLot, it looks like it will timeout immediately because it waits while > timeout.nowWithSameClock() < timeout. It seems like code should treat NaN as infinity when waiting. NaNs propagate, so a mistake would be easy to spot when it infloops, but it if just insta-wakes then I don't think bugs will get noticed. > > > Source/WTF/wtf/CurrentTime.cpp:343 > > > + fakeCondition.waitFor(fakeLock, Seconds(value)); > > > > Why not change `sleep` above to accept Seconds instead of double? > > CurrentTime.h is currently for our deprecated double-based time. There's > still code that uses that, and I don't want to change all of it in this > patch. Bug #? > > > Source/WTF/wtf/MonotonicTime.h:69 > > > + explicit operator bool() const { return !!m_value; } > > > > I don't understand the intent of converting to bool. "if not epoch"? > > In code that used double for time, we use 0 to mean "I don't have a time". That seems weird to me. I guess I don't understand the usecase. Wouldn't NaN time be "I don't have a time"? It doesn't need to be in this patch, but it's probably worth changing later. > > > Source/WTF/wtf/MonotonicTime.h:111 > > > + } > > > > Does it make sense to have == and != ? I think you want to use < and > only > > (and maybe isinf / isnan). > > It does make sense! ==/!= only become strange if you've transformed values > with math that could round. I read this as: it works if you don't mess up. < and > always work, they just seem better. > We use it to compare to infinity in a few places, and it's our way of > checking for NaN. I'm not a fan of that ;-) > > > Source/WTF/wtf/Seconds.h:142 > > > + } > > > > Same here, I'm not sure about strict equality. > > > > > Source/WTF/wtf/Seconds.h:166 > > > + WTF_EXPORT_PRIVATE void sleep() const; > > > > I... ew... Seconds().sleep()... ew... > > Hahaha! https://www.youtube.com/watch?v=PEZWYXPvmS8

Created attachment 293908 [details] the patch Fixed a bunch of things.

Attachment 293908 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #16) > > > > Source/WTF/wtf/Condition.h:73 > > > > + if (timeout < timeout.nowWithSameClock()) { > > > > > > I'm not sure I understand the invariants w.r.t. inf and NaN. Can they ever > > > occur here (should it be an assert) or is it impossible by construction? > > > > NaN could happen here if the passed timeout was NaN. I don't attempt to > > guarantee sensible behavior if you pass a NaN timeout, but looking at > > ParkingLot, it looks like it will timeout immediately because it waits while > > timeout.nowWithSameClock() < timeout. > > It seems like code should treat NaN as infinity when waiting. NaNs > propagate, so a mistake would be easy to spot when it infloops, but it if > just insta-wakes then I don't think bugs will get noticed. It'll get noticed both ways. Insta-waking registers as a spin. > > > > > > Source/WTF/wtf/CurrentTime.cpp:343 > > > > + fakeCondition.waitFor(fakeLock, Seconds(value)); > > > > > > Why not change `sleep` above to accept Seconds instead of double? > > > > CurrentTime.h is currently for our deprecated double-based time. There's > > still code that uses that, and I don't want to change all of it in this > > patch. > > Bug #? I don't think we have a bug for that, because I don't think there's agreement on the extent to which we should get rid of that old code. > > > > > > Source/WTF/wtf/MonotonicTime.h:69 > > > > + explicit operator bool() const { return !!m_value; } > > > > > > I don't understand the intent of converting to bool. "if not epoch"? > > > > In code that used double for time, we use 0 to mean "I don't have a time". > > That seems weird to me. I guess I don't understand the usecase. Wouldn't NaN > time be "I don't have a time"? It doesn't need to be in this patch, but it's > probably worth changing later. Well, there's code that does that already. Also, it because Seconds and the Time classes are meant to just be stronger-typed numbers, it makes sense for them to respond to operator! the same way that a number would. > > > > > > Source/WTF/wtf/MonotonicTime.h:111 > > > > + } > > > > > > Does it make sense to have == and != ? I think you want to use < and > only > > > (and maybe isinf / isnan). > > > > It does make sense! ==/!= only become strange if you've transformed values > > with math that could round. > > I read this as: it works if you don't mess up. < and > always work, they > just seem better. Right, and since this is a stronger-typed number, it should support those things the same way that numbers support them. > > > > We use it to compare to infinity in a few places, and it's our way of > > checking for NaN. > > I'm not a fan of that ;-) > > > > > > Source/WTF/wtf/Seconds.h:142 > > > > + } > > > > > > Same here, I'm not sure about strict equality. > > > > > > > Source/WTF/wtf/Seconds.h:166 > > > > + WTF_EXPORT_PRIVATE void sleep() const; > > > > > > I... ew... Seconds().sleep()... ew... > > > > Hahaha! > > https://www.youtube.com/watch?v=PEZWYXPvmS8 These reactions are worth it. But seriously: any technical reason not to do it this way?

Created attachment 293911 [details] the patch

Attachment 293911 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/WallTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/TimeWithDynamicClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MonotonicTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 8 in 59 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 293892 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=293892&action=review > Source/WTF/wtf/MonotonicTime.h:42 > +// MonotonicTime::now().secondsSinceEpoch().value() is the same as > +// WTF::monotonicallyIncreasingTime(). Typically, we convert these statements into assertions instead of adding a comment in WebCore. > Source/WTF/wtf/MonotonicTime.h:48 > + // This is the epoch. So, x.secondsSinceEpoch() should be the same as x - MonotonicTime(). > + MonotonicTime() { } It's a bit confusing for MonotonicTime() to mean epoch time & "I don't have a time". > Source/WTF/wtf/MonotonicTime.h:53 > + // Call this if you know for sure that the double represents time according to > + // WTF::monotonicallyIncreasingTime(). It must be in seconds and it must be from the same time > + // source. > + static MonotonicTime fromRawDouble(double value) It's unclear to me in which unit double would be in. I presume it's either in milliseconds or seconds but why don't we make that explicit? e.g. fromRawMilliseconds. > Source/WTF/wtf/Seconds.cpp:46 > +WallTime Seconds::operator+(WallTime other) const > +{ > + return other + *this; > +} > + > +MonotonicTime Seconds::operator+(MonotonicTime other) const > +{ > + return other + *this; > +} > + It seems like we could just inline these? It's a bit silly to call a function just to add two numbers. > Source/WTF/wtf/TimeWithDynamicClockType.cpp:98 > +bool TimeWithDynamicClockType::operator<(const TimeWithDynamicClockType& other) const > +{ > + RELEASE_ASSERT(m_type == other.m_type); > + return m_value < other.m_value; > +} Again, it seems like we should be inlining these. > Source/WTF/wtf/TimeWithDynamicClockType.h:65 > + WTF_EXPORT_PRIVATE TimeWithDynamicClockType nowWithSameClock() const; It seems that the common cause of calling now() is to get the current time in the same clock so I'd call this just now() if I were you. > Source/WTF/wtf/TimeWithDynamicClockType.h:67 > + TimeWithDynamicClockType withSameClockAndRawDouble(double value) const Ditto about calling this just fromRawDouble although I'd prefer something like fromRawMilliseconds which clarifies the unit of time. > Source/WTF/wtf/TimeWithDynamicClockType.h:72 > + // Asserts that the time is of the type you want. This comment would have been unnecessary if the functions were inlined. Also, I'm not sure RELEASE_ASSERT would buy us much compared to regular ASSERT. > Source/WTF/wtf/TimeWithDynamicClockType.h:74 > + WTF_EXPORT_PRIVATE WallTime wallTime() const; > + WTF_EXPORT_PRIVATE MonotonicTime monotonicTime() const; I usually prefer a template for things like this. e.g. TimeWithDynamicClockType t1; t1.time<WallTime>() t1.approximte<MonotonicTime>()

> > > > > Source/WTF/wtf/Seconds.h:142 > > > > > + } > > > > > > > > Same here, I'm not sure about strict equality. > > > > > > > > > Source/WTF/wtf/Seconds.h:166 > > > > > + WTF_EXPORT_PRIVATE void sleep() const; > > > > > > > > I... ew... Seconds().sleep()... ew... > > > > > > Hahaha! > > > > https://www.youtube.com/watch?v=PEZWYXPvmS8 > > These reactions are worth it. > > But seriously: any technical reason not to do it this way? Is "fugly" a technical reason? I guess it's fine, it's just weird layering that the class that counts time can actually suspend it as well. I mean, could you also write a Money class so I can Dollar().SendTo("JF") ? At the end of the day it doesn't really matter, seems convenient, and fits the "WTF" moniker :-)

> could you also write a Money class so I can Dollar().SendTo("JF") ? Let me change this to Dollar(31e37).sendTo("JF"). I assume you'd zero-initialize.

(In reply to comment #22) > Comment on attachment 293892 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293892&action=review > > > Source/WTF/wtf/MonotonicTime.h:42 > > +// MonotonicTime::now().secondsSinceEpoch().value() is the same as > > +// WTF::monotonicallyIncreasingTime(). > > Typically, we convert these statements into assertions instead of adding a > comment in WebCore. It's not possible to assert this, because every time you call monotonicallyIncreasingTime() or MonotonicTime::now(), they return a new value, almost by definition. > > > Source/WTF/wtf/MonotonicTime.h:48 > > + // This is the epoch. So, x.secondsSinceEpoch() should be the same as x - MonotonicTime(). > > + MonotonicTime() { } > > It's a bit confusing for MonotonicTime() to mean epoch time & "I don't have > a time". Usually, zero is the default value of numbers. Usually, zero converts to false in operator bool. I'd rather stick to these conventions because if I came up with new ones then they'd probably be more surprising than these. I don't think that this implies that !time means "I don't have the time". The goal of these types is to be numeric when appropriate to easy transition from code that uses doubles already, and so it's nice to have operator! do something similar to what WK expects. It so happens that in WebCore there is some code that uses a zero time to mean "I don't have a time". > > > Source/WTF/wtf/MonotonicTime.h:53 > > + // Call this if you know for sure that the double represents time according to > > + // WTF::monotonicallyIncreasingTime(). It must be in seconds and it must be from the same time > > + // source. > > + static MonotonicTime fromRawDouble(double value) > > It's unclear to me in which unit double would be in. > I presume it's either in milliseconds or seconds but why don't we make that > explicit? > e.g. fromRawMilliseconds. I'll rename it to fromRawSeconds(). > > > Source/WTF/wtf/Seconds.cpp:46 > > +WallTime Seconds::operator+(WallTime other) const > > +{ > > + return other + *this; > > +} > > + > > +MonotonicTime Seconds::operator+(MonotonicTime other) const > > +{ > > + return other + *this; > > +} > > + > > It seems like we could just inline these? It's a bit silly to call a > function just to add two numbers. We could do it, but it probably doesn't matter since none of the code that does time computations is on critical path. > > > Source/WTF/wtf/TimeWithDynamicClockType.cpp:98 > > +bool TimeWithDynamicClockType::operator<(const TimeWithDynamicClockType& other) const > > +{ > > + RELEASE_ASSERT(m_type == other.m_type); > > + return m_value < other.m_value; > > +} > > Again, it seems like we should be inlining these. This makes the RELEASE_ASSERT have a nicer backtrace, and none of these functions are on critical path. > > > Source/WTF/wtf/TimeWithDynamicClockType.h:65 > > + WTF_EXPORT_PRIVATE TimeWithDynamicClockType nowWithSameClock() const; > > It seems that the common cause of calling now() is to get the current time > in the same clock so I'd call this just now() if I were you. I named it this way to make uses look intuitive. In your version, we'd have code that says: if (timeout.now() < timeout) .... With my version it says: if (timeout.nowWithSameClock() < timeout) .... In your version, if I had not seen the code before, I'd be confused why a time has a now() method that returns a time. But calling it nowWithSameClock() gives a pretty good hint as to why. > > > Source/WTF/wtf/TimeWithDynamicClockType.h:67 > > + TimeWithDynamicClockType withSameClockAndRawDouble(double value) const > > Ditto about calling this just fromRawDouble although I'd prefer something > like fromRawMilliseconds which clarifies the unit of time. I'll rename to withSameClockAndRawSeconds(). Again, I wanted the method name to answer the question: why isn't this a static method? > > > Source/WTF/wtf/TimeWithDynamicClockType.h:72 > > + // Asserts that the time is of the type you want. > > This comment would have been unnecessary if the functions were inlined. > Also, I'm not sure RELEASE_ASSERT would buy us much compared to regular > ASSERT. Concurrency bugs often only repro in release because of timing. > > > Source/WTF/wtf/TimeWithDynamicClockType.h:74 > > + WTF_EXPORT_PRIVATE WallTime wallTime() const; > > + WTF_EXPORT_PRIVATE MonotonicTime monotonicTime() const; > > I usually prefer a template for things like this. e.g. > TimeWithDynamicClockType t1; > t1.time<WallTime>() > t1.approximte<MonotonicTime>() I think that would make the code more complicated because then these types would have to have adapter methods, like WallTime::approximateFrom(WallTime | MonotonicTime | TimeWithDynamicClockType). So, that's not any better. As it stands, instead of using templates to range over both types of time, you can just use TimeWithDynamicClockType. That's great, because it eliminates template bloat from code that simply won't need it: none of the time code in WK is perf critical.

Created attachment 293913 [details] the patch

Attachment 293913 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/Seconds.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/WallTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/TimeWithDynamicClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MonotonicTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 9 in 60 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 293913 [details] the patch I would like to see types that differentiate between absolute times and time deltas, because it makes reading code that does time math easier. Deltas could be described as a "time interval".

(In reply to comment #28) > Comment on attachment 293913 [details] > the patch > > I would like to see types that differentiate between absolute times and time > deltas, because it makes reading code that does time math easier. Deltas > could be described as a "time interval". This patch does this! :-) For measuring time intervals, there is a type simply called Seconds. It's never vended from, or accepted by, functions that speak of moments in time. It's only used to describe intervals, which I believe is consistent with the SI definition of seconds. For absolute times, there are three types: WallTime (for wall-clock or real-time clock times like gettimeofday()) and MonotonicTime, as well as a type that dynamically holds either a WallTime or MonotonicTime, called TimeWithDynamicClockType.

Created attachment 293919 [details] the patch

Attachment 293919 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/Seconds.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/WallTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/TimeWithDynamicClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MonotonicTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 9 in 60 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 293919 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=293919&action=review > Source/WebKit2/Platform/IPC/Connection.cpp:472 > // Clamp the timeout to however much time is remaining on our clock. This comment's no longer relevant.

It almost looks like the iOS builders are getting an internal compiler error. I'll see what happens when I build it myself.

(In reply to comment #32) > Comment on attachment 293919 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293919&action=review > > > Source/WebKit2/Platform/IPC/Connection.cpp:472 > > // Clamp the timeout to however much time is remaining on our clock. > > This comment's no longer relevant. Oops, fixed!

The iOS build failure was a trivial mistake. Too bad the EWS log didn't didn't show it clearly.

Created attachment 293932 [details] the patch

Attachment 293932 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/Seconds.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/WallTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/threads/BinarySemaphore.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/TimeWithDynamicClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MonotonicTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 9 in 60 files If any of these errors are false positives, please file a bug against check-webkit-style.

I contacted Howard (original libc++ author, who standardized std::chrono). He thinks this should be sufficient for our needs: ``` using Seconds = std::chrono::duration<double>; using WallTime = std::chrono::time_point<std::chrono::system_clock, Seconds>; ``` It seems like we're discussing two things in this issue: 1. objections over the API being wordy / ugly 2. trying to avoid bugs We can debate 1. forever, not worth it. You code is pretty small, probably unlikely to have bugs. I think Howard's suggestion fixes 2.? Howard's experience has been that bugs are rare and easy to fix, I don't have an opinion here. Floating-point seems to do what we want for a browser (we don't really need exact precision for time, the properties of FP work great for us, and avoiding the pitfalls of overflow is nice). Your code did remove a workaround for a libstdc++ impl bug as well. Not much to do about that if we keep std::chrono. It's a bit of a shame if the STL doesn't work for us. That wouldn't be new for WTF, but if there's something we can take to the committee I'd like to make sure I understand it.

(In reply to comment #38) > I contacted Howard (original libc++ author, who standardized std::chrono). > He thinks this should be sufficient for our needs: > > ``` > using Seconds = std::chrono::duration<double>; > using WallTime = std::chrono::time_point<std::chrono::system_clock, Seconds>; > ``` It's not: - We need a mechanism to reject code that tries to use chrono with int types. This would do no such thing. - TimeWithDynamicClockType gives us functionality that chrono does not have. - You forgot MonotonicTime. > > It seems like we're discussing two things in this issue: > 1. objections over the API being wordy / ugly > 2. trying to avoid bugs And: 3. TimeWithDynamicClockType > > We can debate 1. forever, not worth it. You code is pretty small, probably > unlikely to have bugs. > > I think Howard's suggestion fixes 2.? It doesn't because you could still accidentally use integer times, and that's what you get by default when you use the built-in chrono types. The point here is to eliminate chrono from our codebase, because it encourages usage patterns that are not safe. > Howard's experience has been that bugs > are rare and easy to fix, I don't have an opinion here. We have found them to be incredibly difficult. > Floating-point seems > to do what we want for a browser (we don't really need exact precision for > time, the properties of FP work great for us, and avoiding the pitfalls of > overflow is nice). > > Your code did remove a workaround for a libstdc++ impl bug as well. Not much > to do about that if we keep std::chrono. > > It's a bit of a shame if the STL doesn't work for us. That wouldn't be new > for WTF, but if there's something we can take to the committee I'd like to > make sure I understand it. I think it's a simple bug: chrono should not have allowed integers to be used as the representation.

(In reply to comment #39) > (In reply to comment #38) > > I contacted Howard (original libc++ author, who standardized std::chrono). > > He thinks this should be sufficient for our needs: > > > > ``` > > using Seconds = std::chrono::duration<double>; > > using WallTime = std::chrono::time_point<std::chrono::system_clock, Seconds>; > > ``` > > It's not: > > - We need a mechanism to reject code that tries to use chrono with int > types. This would do no such thing. check-webkit-style would :-) > - TimeWithDynamicClockType gives us functionality that chrono does not have. > - You forgot MonotonicTime. > > > > > It seems like we're discussing two things in this issue: > > 1. objections over the API being wordy / ugly > > 2. trying to avoid bugs > > And: > > 3. TimeWithDynamicClockType True. To be clear, I'm not trying to derail your patch: I want to distill this for the committee. > I think it's a simple bug: chrono should not have allowed integers to be > used as the representation. That's the curse of generality: the committee doesn't just have those for the use you make of it, IIUC it's also therefore for computations that wanted to be precise. You can argue that the default is wrong, but then you're sounding like that guy: https://twitter.com/isotrumpp/status/729901801814691840 ;-)

(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #38) > > > I contacted Howard (original libc++ author, who standardized std::chrono). > > > He thinks this should be sufficient for our needs: > > > > > > ``` > > > using Seconds = std::chrono::duration<double>; > > > using WallTime = std::chrono::time_point<std::chrono::system_clock, Seconds>; > > > ``` > > > > It's not: > > > > - We need a mechanism to reject code that tries to use chrono with int > > types. This would do no such thing. > > check-webkit-style would :-) It's better to have APIs that don't need style rules. > > > > - TimeWithDynamicClockType gives us functionality that chrono does not have. > > - You forgot MonotonicTime. > > > > > > > > It seems like we're discussing two things in this issue: > > > 1. objections over the API being wordy / ugly > > > 2. trying to avoid bugs > > > > And: > > > > 3. TimeWithDynamicClockType > > True. To be clear, I'm not trying to derail your patch: I want to distill > this for the committee. > > > > I think it's a simple bug: chrono should not have allowed integers to be > > used as the representation. > > That's the curse of generality: the committee doesn't just have those for > the use you make of it, IIUC it's also therefore for computations that > wanted to be precise. > > You can argue that the default is wrong, but then you're sounding like that > guy: > https://twitter.com/isotrumpp/status/729901801814691840 > ;-) The default is bad enough that it's the reason why the API is getting ripped out of WebKit.

We already had a discussion about having these classes on webkit-dev, and most of people who engaged in that discussion have given a positive support for it. I don't think having a separate side discussion on Bugzilla is helpful. If you feel that these classes aren't needed, then you should raise that point on webkit-dev thread: https://lists.webkit.org/pipermail/webkit-dev/2016-November/028497.html

Comment on attachment 293932 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=293932&action=review r=me. I'd like to see mac-debug EWS go green before this lands. > Source/JavaScriptCore/runtime/AtomicsObject.cpp:308 > + Stray whitespace. > Source/WTF/wtf/ClockType.h:27 > +#ifndef WTF_ClockType_h > +#define WTF_ClockType_h Let's use #pragma once instead. > Source/WTF/wtf/CurrentTime.cpp:2 > + * Copyright (C) 2006, 2008, 2009, 2010, 2016 Apple Inc. All rights reserved. This can be written as 2006-2016. > Source/WTF/wtf/MonotonicTime.h:27 > +#ifndef WTF_MonotonicTime_h > +#define WTF_MonotonicTime_h #pragma once > Source/WTF/wtf/Seconds.h:27 > +#ifndef WTF_Seconds_h > +#define WTF_Seconds_h #pragma once > Source/WTF/wtf/TimeWithDynamicClockType.cpp:129 > +void TimeWithDynamicClockType::sleep() const > +{ > + Lock fakeLock; > + Condition fakeCondition; > + LockHolder fakeLocker(fakeLock); > + fakeCondition.waitUntil(fakeLock, *this); > +} I also find it weird that this is a member function on a class that's supposed to represent a time value. Seems like it should be a free function that takes a TimeWithDynamicClock as a parameter. > Source/WTF/wtf/TimeWithDynamicClockType.h:27 > +#ifndef WTF_TimeWithDynamicClockType_h > +#define WTF_TimeWithDynamicClockType_h #pragma once > Source/WTF/wtf/WallTime.h:27 > +#ifndef WTF_WallTime_h > +#define WTF_WallTime_h #pragma once > Source/WTF/wtf/threads/BinarySemaphore.cpp:2 > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. 2010-2016 > Source/WTF/wtf/threads/BinarySemaphore.h:2 > + * Copyright (C) 2010, 2011, 2016 Apple Inc. All rights reserved. 2010-2016. > Source/WTF/wtf/threads/BinarySemaphore.h:30 > +#include <wtf/TimeWithDynamicClockType.h> This should go below ThreadingPrimitives.h. > Source/WebCore/platform/MainThreadSharedTimer.h:3 > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. 2006-2016. > Source/WebCore/platform/SharedTimer.h:2 > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. 2006-2016. > Source/WebCore/platform/ThreadTimers.cpp:2 > + * Copyright (C) 2006, 2008, 2016 Apple Inc. All rights reserved. 2006-2016. > Source/WebCore/platform/cf/MainThreadSharedTimerCF.cpp:2 > + * Copyright (C) 2006, 2010, 2015-2016 Apple Inc. All rights reserved. 2006-2016. > Source/WebCore/platform/glib/MainThreadSharedTimerGLib.cpp:2 > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. 2006-2016. > Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:2 > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. 2006-2016. > Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:124 > +void MainThreadSharedTimer::setFireInterval(Seconds intervalInSeconds) Don't see why the "InSeconds" is needed now that the type is Seconds. > Source/WebKit2/Platform/IPC/Connection.cpp:2 > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. 2010-2016. > Source/WebKit2/Platform/IPC/Connection.h:2 > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. 2010-2016. > Source/WebKit2/Platform/IPC/MessageSender.h:2 > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. 2010-2016. > Source/WebKit2/UIProcess/ChildProcessProxy.h:2 > + * Copyright (C) 2012, 2016 Apple Inc. All rights reserved. 2012-2016. > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:2 > + * Copyright (C) 2012, 2016 Apple Inc. All rights reserved. 2012-2016. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:2 > + * Copyright (C) 2013, 2016 Apple Inc. All rights reserved. 2013-2016. > Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:2 > + * Copyright (C) 2012-2014, 2016 Apple Inc. All rights reserved. 2012-2016. > Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:2 > + * Copyright (C) 2012-2014, 2016 Apple Inc. All rights reserved. 2012-2016. > Source/WebKit2/UIProcess/mac/TiledCoreAnimationDrawingAreaProxy.mm:2 > + * Copyright (C) 2011, 2016 Apple Inc. All rights reserved. 2011-2016. > Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:2 > + * Copyright (C) 2014, 2016 Apple Inc. All rights reserved. 2014-2016. > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:2 > + * Copyright (C) 2010, 2011, 2015-2016 Apple Inc. All rights reserved. 2010-2016.

(In reply to comment #43) > Comment on attachment 293932 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293932&action=review > > r=me. I'd like to see mac-debug EWS go green before this lands. Will do! > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:308 > > + > > Stray whitespace. Fixed. > > > Source/WTF/wtf/ClockType.h:27 > > +#ifndef WTF_ClockType_h > > +#define WTF_ClockType_h > > Let's use #pragma once instead. If I do this, Windows EWS is red. It seems to include these headers along multiple paths. > > > Source/WTF/wtf/CurrentTime.cpp:2 > > + * Copyright (C) 2006, 2008, 2009, 2010, 2016 Apple Inc. All rights reserved. > > This can be written as 2006-2016. Fixed. > > > Source/WTF/wtf/MonotonicTime.h:27 > > +#ifndef WTF_MonotonicTime_h > > +#define WTF_MonotonicTime_h > > #pragma once > > > Source/WTF/wtf/Seconds.h:27 > > +#ifndef WTF_Seconds_h > > +#define WTF_Seconds_h > > #pragma once > > > Source/WTF/wtf/TimeWithDynamicClockType.cpp:129 > > +void TimeWithDynamicClockType::sleep() const > > +{ > > + Lock fakeLock; > > + Condition fakeCondition; > > + LockHolder fakeLocker(fakeLock); > > + fakeCondition.waitUntil(fakeLock, *this); > > +} > > I also find it weird that this is a member function on a class that's > supposed to represent a time value. Seems like it should be a free function > that takes a TimeWithDynamicClock as a parameter. OK, I made this a free function. > > > Source/WTF/wtf/TimeWithDynamicClockType.h:27 > > +#ifndef WTF_TimeWithDynamicClockType_h > > +#define WTF_TimeWithDynamicClockType_h > > #pragma once > > > Source/WTF/wtf/WallTime.h:27 > > +#ifndef WTF_WallTime_h > > +#define WTF_WallTime_h > > #pragma once > > > Source/WTF/wtf/threads/BinarySemaphore.cpp:2 > > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. > > 2010-2016 Fixed. > > > Source/WTF/wtf/threads/BinarySemaphore.h:2 > > + * Copyright (C) 2010, 2011, 2016 Apple Inc. All rights reserved. > > 2010-2016. Fixed. > > > Source/WTF/wtf/threads/BinarySemaphore.h:30 > > +#include <wtf/TimeWithDynamicClockType.h> > > This should go below ThreadingPrimitives.h. Fixed. > > > Source/WebCore/platform/MainThreadSharedTimer.h:3 > > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. > > 2006-2016. Fixed. > > > Source/WebCore/platform/SharedTimer.h:2 > > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. > > 2006-2016. Fixed. > > > Source/WebCore/platform/ThreadTimers.cpp:2 > > + * Copyright (C) 2006, 2008, 2016 Apple Inc. All rights reserved. > > 2006-2016. FIxed. > > > Source/WebCore/platform/cf/MainThreadSharedTimerCF.cpp:2 > > + * Copyright (C) 2006, 2010, 2015-2016 Apple Inc. All rights reserved. > > 2006-2016. Fixed. > > > Source/WebCore/platform/glib/MainThreadSharedTimerGLib.cpp:2 > > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. > > 2006-2016. Fixed. > > > Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:2 > > + * Copyright (C) 2006, 2016 Apple Inc. All rights reserved. > > 2006-2016. Fixed. > > > Source/WebCore/platform/win/MainThreadSharedTimerWin.cpp:124 > > +void MainThreadSharedTimer::setFireInterval(Seconds intervalInSeconds) > > Don't see why the "InSeconds" is needed now that the type is Seconds. This code is pretty weird, and I decided to leave it weird. Even before my change, it had these variables: intervalInSeconds, which was what it seemed to be interval, which was the interval in milliseconds intervalInMS, which was a clipped interval that respects USER_TIMER_MAXIMUM I cannot simply rename intervalInSeconds, and I didn't want to try to make bigger changes to this code. > > > Source/WebKit2/Platform/IPC/Connection.cpp:2 > > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. > > 2010-2016. Fixed. > > > Source/WebKit2/Platform/IPC/Connection.h:2 > > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. > > 2010-2016. Fixed. > > > Source/WebKit2/Platform/IPC/MessageSender.h:2 > > + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved. > > 2010-2016. Fixed. > > > Source/WebKit2/UIProcess/ChildProcessProxy.h:2 > > + * Copyright (C) 2012, 2016 Apple Inc. All rights reserved. > > 2012-2016. Fixed. > > > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:2 > > + * Copyright (C) 2012, 2016 Apple Inc. All rights reserved. > > 2012-2016. Fixed. > > > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:2 > > + * Copyright (C) 2013, 2016 Apple Inc. All rights reserved. > > 2013-2016. FIxed. > > > Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:2 > > + * Copyright (C) 2012-2014, 2016 Apple Inc. All rights reserved. > > 2012-2016. Fixed. > > > Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:2 > > + * Copyright (C) 2012-2014, 2016 Apple Inc. All rights reserved. > > 2012-2016. Fixed. > > > Source/WebKit2/UIProcess/mac/TiledCoreAnimationDrawingAreaProxy.mm:2 > > + * Copyright (C) 2011, 2016 Apple Inc. All rights reserved. > > 2011-2016. Fixed. > > > Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:2 > > + * Copyright (C) 2014, 2016 Apple Inc. All rights reserved. > > 2014-2016. Fixed. > > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:2 > > + * Copyright (C) 2010, 2011, 2015-2016 Apple Inc. All rights reserved. > > 2010-2016. Fixed. Thanks for the detailed review!

Created attachment 293963 [details] patch for landing

Attachment 293963 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Tools/TestWebKitAPI/Tests/WTF/Condition.cpp:250: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/Seconds.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/WallTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/TimeWithDynamicClockType.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MonotonicTime.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/MessageQueue.h:132: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/ICStats.cpp:67: More than one command on the same line [whitespace/newline] [4] Total errors found: 8 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.

Landed in https://trac.webkit.org/changeset/208415

(In reply to comment #47) > Landed in https://trac.webkit.org/changeset/208415 It broke the builf of the JSCOnly port: https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/3599/steps/compile-webkit/logs/stdio

(In reply to comment #48) > (In reply to comment #47) > > Landed in https://trac.webkit.org/changeset/208415 > > It broke the builf of the JSCOnly port: > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/ > 3599/steps/compile-webkit/logs/stdio https://bugs.webkit.org/show_bug.cgi?id=164447 fix is coming.

Comment on attachment 293963 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=293963&action=review > Source/JavaScriptCore/runtime/AtomicsObject.cpp:317 Would you be willing to write this kind of expression as !std::isnan(timeout) for clarity, or is there a reason to prefer this current form?

(In reply to comment #50) > Would you be willing to write this kind of expression as > !std::isnan(timeout) for clarity, or is there a reason to prefer this > current form? For some reason, bugs.webkit.org omitted the line of code. It's: if (timeout == timeout) {

(In reply to comment #50) > Comment on attachment 293963 [details] > patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293963&action=review > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:317 > > > Would you be willing to write this kind of expression as > !std::isnan(timeout) for clarity, or is there a reason to prefer this > current form? I find == to be clearer because it's a primitive operator of the language. In general, if there is a primitive operator in the language that does exactly what I want, then I'd rather use that instead of using a library function that wraps it. If x is a float and IEEE semantics are obeyed, there is no other meaning for x == x other than "is x not NaN". I think that std::isnan is appropriate for projects that compile with math optimization settings that break x==x and other IEEE rules. We can't use those settings.

(In reply to comment #52) > I find == to be clearer because it's a primitive operator of the language. OK. I understand the reason behind your preference and I don’t agree that it’s clearer to most programmers, even ones who are fully expert in the IEEE semantics. If you are willing, I suggest you talk to the other programmers you are working with to make sure they concur, since they are the people that need to read and understand the code.

If we do reach consensus that using "x == x" and "x != x" directly is clearer than using std::nan then we should follow through: Since I prefer consistent style across WebKit, I suggest we consider whether we want to change the 30 or so places in JavaScriptCore that use std::isnan and the 70 or so places in WebCore that use std::isnan. I have to say, though, that reading them over, I don’t think they would be clearer if they used the operator instead of the function.

Two more thoughts: 1) Didn't mean to come on so strong above! But I do think a lot of the current code using std::isnan is pretty easy to read and I hope you will take a look before fully committing to what you prefer. 2) I think the change is great. I’m sad that std::chrono was such a bust for our project. Some of the other recent C++ features have helped us simplify and streamline our code, but it's good that we took action before this one made things a lot worse. Only thing I’m sad about is losing the nice literal syntax (1ms, 1h, etc.). Maybe we can find a way to make that syntax work for durations with our classes.

(In reply to comment #55) > Two more thoughts: > > 1) Didn't mean to come on so strong above! But I do think a lot of the > current code using std::isnan is pretty easy to read and I hope you will > take a look before fully committing to what you prefer. > > 2) I think the change is great. I’m sad that std::chrono was such a bust for > our project. Some of the other recent C++ features have helped us simplify > and streamline our code, but it's good that we took action before this one > made things a lot worse. Only thing I’m sad about is losing the nice literal > syntax (1ms, 1h, etc.). Maybe we can find a way to make that syntax work for > durations with our classes. I think that it would be easy to get that back, but I didn't try, because I haven't yet found code that uses the literal syntax.

(In reply to comment #56) > I haven't yet found code that uses the literal syntax I found some uses of ms (edited out the ones that were using 0ms) WebCore/inspector/InspectorOverlay.cpp: const auto removeDelay = 250ms; WebCore/loader/ProgressTracker.cpp:static const auto progressNotificationTimeInterval = 200ms; WebCore/page/DOMTimer.cpp:static const auto maxIntervalForUserGestureForwarding = 1000ms; // One second matches Gecko. WebCore/page/DOMTimer.cpp:static const auto minIntervalForNonUserObservableChangeTimers = 1000ms; // Empirically determined to maximize battery life. WebCore/page/DOMTimer.cpp: auto interval = std::max(1ms, m_originalInterval); WebCore/page/DOMTimer.h: static std::chrono::milliseconds defaultMinimumInterval() { return 4ms; } WebCore/page/DOMTimer.h: static std::chrono::milliseconds defaultAlignmentInterval() { return 0ms; } WebCore/page/DOMTimer.h: static std::chrono::milliseconds hiddenPageAlignmentInterval() { return 1000ms; } WebCore/page/ResourceUsageThread.cpp: auto difference = 500ms - duration; WebCore/page/Settings.cpp:static const auto layoutScheduleThreshold = 250ms; WebCore/page/mac/ServicesOverlayController.mm: auto minimumTimeUntilHighlightShouldBeShown = 200ms; WebCore/page/mac/ServicesOverlayController.mm: minimumTimeUntilHighlightShouldBeShown = 1000ms; WebCore/platform/gamepad/cocoa/GameControllerGamepadProvider.mm:static const std::chrono::milliseconds InputNotificationDelay = 16ms; WebCore/platform/graphics/cg/IOSurfacePool.cpp:const std::chrono::milliseconds collectionInterval = 500ms; WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp: const auto readTimeout = 1500ms; WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:const std::chrono::milliseconds volatileSecondaryBackingStoreAgeThreshold = 200ms; WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:const std::chrono::milliseconds volatilityTimerInterval = 200ms; WebKit2/UIProcess/Cocoa/ViewGestureController.cpp:static const std::chrono::milliseconds swipeSnapshotRemovalActiveLoadMonitoringInterval = 250ms; WebKit2/WebProcess/Network/WebLoaderStrategy.cpp: return 500ms; WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm: const auto initialFlushDelay = 500ms; WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm: const auto flushDelay = 1500ms; Not sure if we use any of the other units.

(In reply to comment #53) > (In reply to comment #52) > > I find == to be clearer because it's a primitive operator of the language. > > OK. I understand the reason behind your preference and I don’t agree that > it’s clearer to most programmers, even ones who are fully expert in the IEEE > semantics. > > If you are willing, I suggest you talk to the other programmers you are > working with to make sure they concur, since they are the people that need > to read and understand the code. I would find isnan() much more readable. Using == here requires much more brainpower to process.

(In reply to comment #58) > (In reply to comment #53) > > (In reply to comment #52) > > > I find == to be clearer because it's a primitive operator of the language. > > > > OK. I understand the reason behind your preference and I don’t agree that > > it’s clearer to most programmers, even ones who are fully expert in the IEEE > > semantics. > > > > If you are willing, I suggest you talk to the other programmers you are > > working with to make sure they concur, since they are the people that need > > to read and understand the code. > > I would find isnan() much more readable. Using == here requires much more > brainpower to process. I agree.

(In reply to comment #59) > (In reply to comment #58) > > (In reply to comment #53) > > > (In reply to comment #52) > > > > I find == to be clearer because it's a primitive operator of the language. > > > > > > OK. I understand the reason behind your preference and I don’t agree that > > > it’s clearer to most programmers, even ones who are fully expert in the IEEE > > > semantics. > > > > > > If you are willing, I suggest you talk to the other programmers you are > > > working with to make sure they concur, since they are the people that need > > > to read and understand the code. > > > > I would find isnan() much more readable. Using == here requires much more > > brainpower to process. > > I agree. To elaborate on my preference: I don't always agree that we should pick using a primitive operator over a function. This comes down to style/experience more many folks, but when the operator is doing non-obvious things, it should be given a name. I'm reminded of the first chapter of SICP which basically boils down to abstraction is naming, and I think this operation deserves a name.

Comment on attachment 293963 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=293963&action=review > Tools/TestWebKitAPI/Tests/WTF/Time.cpp:185 > +TEST(WTF_Time, less) > +{ > + EXPECT_EQ(false, s(2) < s(1)); > + EXPECT_EQ(false, s(2) < s(2)); > + EXPECT_EQ(true, s(2) < s(3)); > + EXPECT_EQ(false, wt(2) < wt(1)); > + EXPECT_EQ(false, wt(2) < wt(2)); > + EXPECT_EQ(true, wt(2) < wt(3)); > + EXPECT_EQ(false, mt(2) < mt(1)); > + EXPECT_EQ(false, mt(2) < mt(2)); > + EXPECT_EQ(true, mt(2) < mt(3)); > + EXPECT_EQ(false, dtw(2) < dtw(1)); > + EXPECT_EQ(false, dtw(2) < dtw(2)); > + EXPECT_EQ(true, dtw(2) < dtw(3)); > + EXPECT_EQ(false, dtm(2) < dtm(1)); > + EXPECT_EQ(false, dtm(2) < dtm(2)); > + EXPECT_EQ(true, dtm(2) < dtm(3)); > +} This code introduced zillion warnings on Linux: ../../Tools/TestWebKitAPI/Tests/WTF/Time.cpp: In member function 'virtual void TestWebKitAPI::WTF_Time_less_Test::TestBody()': ../../Tools/TestWebKitAPI/Tests/WTF/Time.cpp:170:158: warning: converting 'false' to pointer type for argument 1 of 'char testing::internal::IsNullLiteralHelper(testing::internal::Secret*)' [-Wconversion-null] ../../Tools/TestWebKitAPI/Tests/WTF/Time.cpp:171:158: warning: converting 'false' to pointer type for argument 1 of 'char testing::internal::IsNullLiteralHelper(testing::internal::Secret*)' [-Wconversion-null] ../../Tools/TestWebKitAPI/Tests/WTF/Time.cpp:173:158: warning: converting 'false' to pointer type for argument 1 of 'char testing::internal::IsNullLiteralHelper(testing::internal::Secret*)' [-Wconversion-null] ...

Best fix for that is to use EXPECT_TRUE(x) and EXPECT_FALSE(y) instead of EXPECT_EQ(true, x) and EXPECT_EQ(false, y).

(In reply to comment #62) > Best fix for that is to use EXPECT_TRUE(x) and EXPECT_FALSE(y) instead of > EXPECT_EQ(true, x) and EXPECT_EQ(false, y). Thanks for the idea, fixing in bug165061.