Bug 181175 – [WTF] Add clock_gettime based monotonicallyIncreasingTime implementation for Linux and BSDs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Add clock_gettime based monotonicallyIncreasingTime implementation for Linux and BSDs

[Yusuke Suzuki]

Created attachment 330214 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 330214 [details]
Patch

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

> Source/WTF/wtf/CurrentTime.cpp:277
> +}

FYI, Qt's implementation of monotonic clock timer uses more sofisticated check, see https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qelapsedtimer_unix.cpp.html#65 and from line 115. On Linux, it always assumes monotonic clock to be present, on other systems it checks values of macros and, if needed, what sysconf(_SC_MONOTONIC_CLOCK) returns

[Konstantin Tokarev]

Here is a version of that file under LGPL 2 (so that portions could be imported if needed): http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qelapsedtimer_unix.cpp?h=5.6

[Yusuke Suzuki]

Comment on attachment 330214 [details]
Patch

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

>> Source/WTF/wtf/CurrentTime.cpp:277
>> +}
> 
> FYI, Qt's implementation of monotonic clock timer uses more sofisticated check, see https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qelapsedtimer_unix.cpp.html#65 and from line 115. On Linux, it always assumes monotonic clock to be present, on other systems it checks values of macros and, if needed, what sysconf(_SC_MONOTONIC_CLOCK) returns

But actually, are there any Linux/FreeBSD/OpenBSD/NetBSD environments that do not support monotonic clock?
For example, libcxx assumes that supported environments have this monotonic clock[1]. And if it fails, it simply crashes.

[1]: https://github.com/llvm-mirror/libcxx/blob/master/src/chrono.cpp#L220-L227

[Michael Catanzaro]

Comment on attachment 330214 [details]
Patch

This seems fine to me, since you've ensured that it only affects JSCOnly. GTK and WPE will be happy that they continue using g_get_monotonic_time, since WebKit events rely on WebKit's monotonic time matching GLib's. (It should be the same under the hood -- CLOCK_MONOTONIC -- either way, but continuing to use g_get_monotonic_time seems preferable.)

[Konstantin Tokarev]

>libcxx assumes that supported environments have this monotonic clock[1]. And if it fails, it simply crashes.

Cool. But at least it checks that CLOCK_MONOTONIC is defined,

[Konstantin Tokarev]

>But actually, are there any Linux/FreeBSD/OpenBSD/NetBSD environments that do not support monotonic clock?

It seems like no, there aren't

[Konstantin Tokarev]

Comment on attachment 330214 [details]
Patch

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

>>> Source/WTF/wtf/CurrentTime.cpp:277
>>> +}
>> 
>> FYI, Qt's implementation of monotonic clock timer uses more sofisticated check, see https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qelapsedtimer_unix.cpp.html#65 and from line 115. On Linux, it always assumes monotonic clock to be present, on other systems it checks values of macros and, if needed, what sysconf(_SC_MONOTONIC_CLOCK) returns
> 
> But actually, are there any Linux/FreeBSD/OpenBSD/NetBSD environments that do not support monotonic clock?
> For example, libcxx assumes that supported environments have this monotonic clock[1]. And if it fails, it simply crashes.
> 
> [1]: https://github.com/llvm-mirror/libcxx/blob/master/src/chrono.cpp#L220-L227

Interestingly, it seems like on Linux CLOCK_MONOTONIC isn't really monotonic, but can be affected by adjtime(3) and NTP. There is additional CLOCK_MONOTONIC_RAW which is available since Linux 2.6.28, which is truly monotonic. Neither Qt nor GLib attempt to use it in their monotonic time wrappers

[Carlos Alberto Lopez Perez]

(In reply to Konstantin Tokarev from comment #8)
> 
> Interestingly, it seems like on Linux CLOCK_MONOTONIC isn't really
> monotonic, but can be affected by adjtime(3) and NTP. There is additional
> CLOCK_MONOTONIC_RAW which is available since Linux 2.6.28, which is truly
> monotonic. Neither Qt nor GLib attempt to use it in their monotonic time
> wrappers

I think both are really monotonic, the difference is that the first is affected by time frequency adjustments via adjtime() (aka: the speed of a second), meanwhile the second doesn't gets affected by that. https://stackoverflow.com/a/14270415

[Konstantin Tokarev]

Comment on attachment 330214 [details]
Patch

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

> Source/WTF/wtf/CurrentTime.cpp:276
> +    return static_cast<double>(ts.tv_sec) + ts.tv_nsec * 1.0e9;

This code has a bug, it should either be

    static_cast<double>(ts.tv_sec) + ts.tv_nsec / 1.0e9

or

    static_cast<double>(ts.tv_sec) * 1.0e9 + ts.tv_nsec

I would prefer the former as it will result in numbers of smaller order

[Yusuke Suzuki]

Comment on attachment 330214 [details]
Patch

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

>> Source/WTF/wtf/CurrentTime.cpp:276
>> +    return static_cast<double>(ts.tv_sec) + ts.tv_nsec * 1.0e9;
> 
> This code has a bug, it should either be
> 
>     static_cast<double>(ts.tv_sec) + ts.tv_nsec / 1.0e9
> 
> or
> 
>     static_cast<double>(ts.tv_sec) * 1.0e9 + ts.tv_nsec
> 
> I would prefer the former as it will result in numbers of smaller order

Oops, fixed. double should be in seconds. The former is fine.

[Yusuke Suzuki]

Committed r226304: <https://trac.webkit.org/changeset/226304>

[Radar WebKit Bug Importer]

<rdar://problem/36261381>

[Radar WebKit Bug Importer]

<rdar://problem/36261385>

[Darin Adler]

Comment on attachment 330214 [details]
Patch

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

>>> Source/WTF/wtf/CurrentTime.cpp:276
>>> +    return static_cast<double>(ts.tv_sec) + ts.tv_nsec * 1.0e9;
>> 
>> This code has a bug, it should either be
>> 
>>     static_cast<double>(ts.tv_sec) + ts.tv_nsec / 1.0e9
>> 
>> or
>> 
>>     static_cast<double>(ts.tv_sec) * 1.0e9 + ts.tv_nsec
>> 
>> I would prefer the former as it will result in numbers of smaller order
> 
> Oops, fixed. double should be in seconds. The former is fine.

I would have suggested 1.0e-9 instead of switching from multiplication to division.