WTF::ParkingLot should stop using std::chrono because std::chrono::duration casts are prone to overflows

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Zan Dobersek]

Spun off from bug #144908
https://bugs.webkit.org/show_bug.cgi?id=144908

Casting between different std::chrono::duration specializations (e.g. seconds or milliseconds to microseconds) can easily result in an overflow of the duration value. In the worst-case scenario we can end up using a negative value to schedule platform-specific timers. In case of GLib, this can end up in continuous and immediate timer dispatches (bugs #144908 and #150930).

Current suggestions revolve around using clamping and/or overflow checks that could maybe be baked into a std::chrono::duration template specialization.

[Darin Adler]

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

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 293632 [details]
patch so far

It's looking unlikely that any version of this patch will be less than 100kb.

[Filip Pizlo]

Created attachment 293673 [details]
more

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 293790 [details]
rebased

JSC is happy with this.  I haven't gotten WebCore to build yet.

[Filip Pizlo]

Created attachment 293853 [details]
close to done

[WebKit Commit Bot]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 293892 [details]
the patch

[WebKit Commit Bot]

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.

[JF Bastien]

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...

[Filip Pizlo]

(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!

[JF Bastien]

> > > 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

[Filip Pizlo]

Created attachment 293908 [details]
the patch

Fixed a bunch of things.

[WebKit Commit Bot]

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.

[Filip Pizlo]

(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?

[Filip Pizlo]

Created attachment 293911 [details]
the patch

[WebKit Commit Bot]

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.

[Ryosuke Niwa]

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>()

[JF Bastien]

> > > > > 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 :-)

[JF Bastien]

> 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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 293913 [details]
the patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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".

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 293919 [details]
the patch

[WebKit Commit Bot]

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.

[Andy Estes]

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.

[Filip Pizlo]

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

[Filip Pizlo]

(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!

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 293932 [details]
the patch

[WebKit Commit Bot]

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.

[JF Bastien]

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.

[Filip Pizlo]

(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.

[JF Bastien]

(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
;-)

[Filip Pizlo]

(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.

[Ryosuke Niwa]

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

[Andy Estes]

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.

[Filip Pizlo]

(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!

[Filip Pizlo]

Created attachment 293963 [details]
patch for landing

[WebKit Commit Bot]

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.

[Filip Pizlo]

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

[Csaba Osztrogonác]

(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

[Yusuke Suzuki]

(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.

[Darin Adler]

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?

[Darin Adler]

(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) {

[Filip Pizlo]

(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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

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.

[Filip Pizlo]

(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.

[Darin Adler]

(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.

[Simon Fraser (smfr)]

(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.

[Saam Barati]

(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.

[Saam Barati]

(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.

[Csaba Osztrogonác]

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]
...

[Darin Adler]

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).

[Csaba Osztrogonác]

(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.