181175 2017-12-27 06:29:26 -0800 [WTF] Add clock_gettime based monotonicallyIncreasingTime implementation for Linux and BSDs 2018-01-05 12:32:36 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki annulen benjamin cdumez cgarcia clopez cmarcelo darin dbates ews-watchlist mark.lam mcatanzaro saam sam webkit-bug-importer oldest_to_newest 1384793 0 ysuzuki 2017-12-27 06:29:26 -0800 [WTF] Add clock_gettime based monotonicallyIncreasingTime implementation for Linux and BSDs 1384794 1 330214 ysuzuki 2017-12-27 06:30:44 -0800 Created attachment 330214 Patch 1384796 2 330214 annulen 2017-12-27 06:42:08 -0800 Comment on attachment 330214 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 1384797 3 annulen 2017-12-27 07:10:10 -0800 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 1384801 4 330214 ysuzuki 2017-12-27 08:12:02 -0800 Comment on attachment 330214 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 1384809 5 330214 mcatanzaro 2017-12-27 09:14:08 -0800 Comment on attachment 330214 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.) 1384813 6 annulen 2017-12-27 09:18:34 -0800 >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, 1384839 7 annulen 2017-12-27 16:49:37 -0800 >But actually, are there any Linux/FreeBSD/OpenBSD/NetBSD environments that do not support monotonic clock? It seems like no, there aren't 1384840 8 330214 annulen 2017-12-27 16:57:18 -0800 Comment on attachment 330214 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 1384841 9 clopez 2017-12-27 17:11:14 -0800 (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 1384843 10 330214 annulen 2017-12-27 17:44:36 -0800 Comment on attachment 330214 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 1384858 11 330214 ysuzuki 2017-12-28 06:13:55 -0800 Comment on attachment 330214 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. 1384860 12 ysuzuki 2017-12-28 06:17:12 -0800 Committed r226304: <https://trac.webkit.org/changeset/226304> 1385235 13 webkit-bug-importer 2018-01-02 13:18:38 -0800 <rdar://problem/36261381> 1385236 14 webkit-bug-importer 2018-01-02 13:18:44 -0800 <rdar://problem/36261385> 1386214 15 330214 darin 2018-01-05 12:32:36 -0800 Comment on attachment 330214 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. 330214 2017-12-27 06:30:44 -0800 2017-12-27 09:14:08 -0800 Patch bug-181175-20171227233043.patch text/plain 1439 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjI2Mjk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGE5NDA4ZTM1OThhNWMzNGJkM2ExYjQz MWQ3NTFjMzQ4MjdmYjk5YjEuLjRhMWZlNWJlZmY2MGVkMjUyYjgzMjU3OWMzOWZiNmUzYjA1MzA4 MTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTctMTItMjcgIFl1c3VrZSBTdXp1a2kgIDx1dGF0 YW5lLnRlYUBnbWFpbC5jb20+CisKKyAgICAgICAgW1dURl0gQWRkIGNsb2NrX2dldHRpbWUgYmFz ZWQgbW9ub3RvbmljYWxseUluY3JlYXNpbmdUaW1lIGltcGxlbWVudGF0aW9uIGZvciBMaW51eCBh bmQgQlNEcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTgxMTc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg VXNlIHBsYXRmb3JtLXByb3ZpZGVkIFBPU0lYIEFQSXMgdG8gZ2V0IG1vbm90b25pYyB0aW1lLgor CisgICAgICAgICogd3RmL0N1cnJlbnRUaW1lLmNwcDoKKyAgICAgICAgKFdURjo6bW9ub3Rvbmlj YWxseUluY3JlYXNpbmdUaW1lKToKKwogMjAxNy0xMi0yNiAgWXVzdWtlIFN1enVraSAgPHV0YXRh bmUudGVhQGdtYWlsLmNvbT4KIAogICAgICAgICBbSlNDXSBSZW1vdmUgc3RkOjpjaHJvbm8gY29t cGxldGVseQpkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvQ3VycmVudFRpbWUuY3BwIGIvU291 cmNlL1dURi93dGYvQ3VycmVudFRpbWUuY3BwCmluZGV4IDE4MjQ3NWMxYzQ1MDk4ZGNhZWU0NGUy NTQwMmJiMzljMWQ0ZDg0YTkuLjFhYzdiMTM0NDI0ZWEwZDcwNGQ2Yjk1MmVhN2YwNGQyY2ZjZTFi YzUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0N1cnJlbnRUaW1lLmNwcAorKysgYi9Tb3Vy Y2UvV1RGL3d0Zi9DdXJyZW50VGltZS5jcHAKQEAgLTI2Nyw2ICsyNjcsMTUgQEAgZG91YmxlIG1v bm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpCiAgICAgcmV0dXJuIChtYWNoX2Fic29sdXRlX3Rp bWUoKSAqIHRpbWViYXNlSW5mby5udW1lcikgLyAoMS4wZTkgKiB0aW1lYmFzZUluZm8uZGVub20p OwogfQogCisjZWxpZiBPUyhMSU5VWCkgfHwgT1MoRlJFRUJTRCkgfHwgT1MoT1BFTkJTRCkgfHwg T1MoTkVUQlNEKQorCitkb3VibGUgbW9ub3RvbmljYWxseUluY3JlYXNpbmdUaW1lKCkKK3sKKyAg ICBzdHJ1Y3QgdGltZXNwZWMgdHMgeyB9OworICAgIGNsb2NrX2dldHRpbWUoQ0xPQ0tfTU9OT1RP TklDLCAmdHMpOworICAgIHJldHVybiBzdGF0aWNfY2FzdDxkb3VibGU+KHRzLnR2X3NlYykgKyB0 cy50dl9uc2VjICogMS4wZTk7Cit9CisKICNlbHNlCiAKIGRvdWJsZSBtb25vdG9uaWNhbGx5SW5j cmVhc2luZ1RpbWUoKQo=