Remove std::chrono if it is not used for ArgumentCoder or PersistentCoder

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Remove std::chrono if it is not used for ArgumentCoder or PersistentCoder

[Yusuke Suzuki]

Created attachment 330213 [details]
Patch

[Yusuke Suzuki]

Created attachment 330215 [details]
Patch

[Yusuke Suzuki]

Created attachment 330234 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 330234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330234&action=review

Added some notes.

> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:351
>  
> -    auto startTime = std::chrono::system_clock::now();
> +    auto startTime = MonotonicTime::now();

Note: When measuring time, we should use MonotonicTime.

> Source/WebKit/NetworkProcess/cache/NetworkCacheStatistics.cpp:96
> -    auto startTime = std::chrono::system_clock::now();
> +    auto startTime = MonotonicTime::now();

Note: When measuring time, we should use MonotonicTime.

[Konstantin Tokarev]

Comment on attachment 330234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330234&action=review

/me personally would stay away from changing network and Apple-specific code from wall clock to monotonic, but YMMV

>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:351
>> +    auto startTime = MonotonicTime::now();
> 
> Note: When measuring time, we should use MonotonicTime.

Note that there are cases when time values are passed between processes. I don't know if MonotonicTime can guarantee that its values are system-wide on all ports & platforms.

FWIW, at least current implementation in Qt port isn't cross-process-safe (which caused me a longish debug session lately)

[Konstantin Tokarev]

At least I think changes from system_clock to MonotonicTime should be in a separate patch, if there is a regression it would be easier to bisect

[Konstantin Tokarev]

Comment on attachment 330234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330234&action=review

> Source/WebCore/fileapi/FileReader.cpp:182
> +    if (std::isnan(m_lastProgressNotificationTime)) {

I'm not sure overloading std::isnan for MonotonicTime was a good idea. IMNSHO overloading stuff in std:: is confusing and should be avoided at all costs, unless polyfilling missing C++ standard features, or for things like std::swap which are implicitly used by existing std algorithm.

Here I would propose using something like if (!m_lastProgressNotificationTime.isValid()), and MonotonicTime::invalid() could be used to construct such values. Or even (carefully) change behavior of default ctor, so that MonotonicTime() is invalid and MonotonicTime { 0 } is epoch zero

[Yusuke Suzuki]

Comment on attachment 330234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330234&action=review

Thank you.

>> Source/WebCore/fileapi/FileReader.cpp:182
>> +    if (std::isnan(m_lastProgressNotificationTime)) {
> 
> I'm not sure overloading std::isnan for MonotonicTime was a good idea. IMNSHO overloading stuff in std:: is confusing and should be avoided at all costs, unless polyfilling missing C++ standard features, or for things like std::swap which are implicitly used by existing std algorithm.
> 
> Here I would propose using something like if (!m_lastProgressNotificationTime.isValid()), and MonotonicTime::invalid() could be used to construct such values. Or even (carefully) change behavior of default ctor, so that MonotonicTime() is invalid and MonotonicTime { 0 } is epoch zero

I think that we know Seconds, MonotonicTime, and WallTime are double. Because of this,

1. all the arithmetic operations onto them follow IEEE double semantics.
2. we have MonotonicTime::infinity(), MonotonicTime::nan() etc. because it is double.

So I think having an overload for std::isnan, std::isfinite, std::isinf is not so bad idea.

BTW, we should not have MonotonicTime(double) constructor. Basically, this MonotonicTime should be produced only from MonotonicTime::now() or the other utility functions in WTF.
Making std::optional<MonotonicTime> m_lastProgressNotificationTime would be one idea. Not sure it is better than using MonotonicTime::nan().

>>> Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:351
>>> +    auto startTime = MonotonicTime::now();
>> 
>> Note: When measuring time, we should use MonotonicTime.
> 
> Note that there are cases when time values are passed between processes. I don't know if MonotonicTime can guarantee that its values are system-wide on all ports & platforms.
> 
> FWIW, at least current implementation in Qt port isn't cross-process-safe (which caused me a longish debug session lately)

OK, for now, changed it to WallTime.

[Konstantin Tokarev]

>So I think having an overload for std::isnan, std::isfinite, std::isinf is not so bad idea.

It's rather confusing for those who didn't use these classes yet. At first you think variable is primitive, then you start thinking there must be implicit conversion, and only then you realize there are overloads in std namespace

>BTW, we should not have MonotonicTime(double) constructor

Agree, this is my mistake. It should have been MonotonicTime::null() then, or something alike.

[Yusuke Suzuki]

(In reply to Konstantin Tokarev from comment #9)
> >So I think having an overload for std::isnan, std::isfinite, std::isinf is not so bad idea.
> 
> It's rather confusing for those who didn't use these classes yet. At first
> you think variable is primitive, then you start thinking there must be
> implicit conversion, and only then you realize there are overloads in std
> namespace

So, I think having each function in {MonotonicTime,WallTime,Seconds} is better. Like, `MonotonicTime::{isnan,isinf,isfinite}`.
Their interface should mimic double, otherwise, we need to learn the semantics of these types from scratch.

Double interface is very important since it ensures that these types are not overflow (this is the most important thing and it is different from std::chrono types). It will go to -+infinity if it becomes huge. And if we do some meaningless operations (like, 10_s / 0), it produces NaN seconds.

If we know that these types are doubles, we easily answer the question like "what does `NaN seconds + 20 seconds` produce?".
It is not clear if we use MonotonicTime::null() since we do not know what happens when we perform `null seconds + 20 seconds`.

[Konstantin Tokarev]

Agree

[Yusuke Suzuki]

Created attachment 330235 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 330235 [details]
Patch

Thank you!

[WebKit Commit Bot]

Comment on attachment 330235 [details]
Patch

Clearing flags on attachment: 330235

Committed r226307: <https://trac.webkit.org/changeset/226307>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36261374>