Use CLOCK_MONOTONIC_COARSE and CLOCK_REALTIME_COARSE on Linux if available.

Use CLOCK_MONOTONIC_COARSE and CLOCK_REALTIME_COARSE on Linux if available.

R=bmeurer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=17611

[bnoordhuis, 2013-10-30 12:32:34]

Use clock_gettime(CLOCK_MONOTONIC_COARSE) if available, saves the overhead of a
system call on older and virtualized systems.

[Benedikt Meurer, 2013-10-30 12:49:15]

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode292
src/platform/time.cc:292: if (clock_gettime(6 /* CLOCK_MONOTONIC_COARSE */, ts)
== 0) {
I don't like hardcoding the constant here. Better add some

#if V8_OS_LINUX
# ifnfdef CLOCK_MONOTONIC_COARSE
#  define CLOCK_MONOTONIC_COARSE 6
# endif
#endif  // V8_OS_LINUX

at the beginning of the file.

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode305
src/platform/time.cc:305: Time Time::Now() {
Time::Now() has to use gettimeofday() on POSIX systems or CLOCK_REALTIME. It
should return a Time value not a TimeTicks value.

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode606
src/platform/time.cc:606: int result = GetBestMonotonicClock(&ts);
Please take a look at the Xorg GetTimeInMillis() function in os/utils.c. I think
we should use something similar here; can be safely inlined into the
HighResolutionNow() function, IMHO.

[Sven Panne, 2013-10-30 12:54:11]

DBC...

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode291
src/platform/time.cc:291: if (no_clock_monotonic_coarse == false) {
http://refactoring.com/catalog/removeDoubleNegative.html :-)

Furthermore, using a static will introduce locking behind the scenes IIRC, I am
not 100% sure if this is a problem here or not.

[bnoordhuis, 2013-10-30 13:38:37]

I suppose I should mention that if there is code in V8 that assumes that two
successive calls to clock_gettime() return different values, then that code is
now more likely to get disappointed.  I didn't find any issues after a quick
audit and test run though.

[bnoordhuis, 2013-10-30 13:38:56]

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode291
src/platform/time.cc:291: if (no_clock_monotonic_coarse == false) {
On 2013/10/30 12:54:11, Sven Panne wrote:
> http://refactoring.com/catalog/removeDoubleNegative.html :-)
> 
> Furthermore, using a static will introduce locking behind the scenes IIRC, I
am
> not 100% sure if this is a problem here or not.

Agree on the double negative but I wrote it this way to ensure that the flag
ends up in the .bss rather than the .data section.

Not sure what you mean by locking.  There is no mutex involved, if that is what
you mean.  At worst, you'll have some cache line dirtying at the start.  But
that's a one-time cost and it only happens when CLOCK_MONOTONIC_COARSE is not
supported.

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode305
src/platform/time.cc:305: Time Time::Now() {
On 2013/10/30 12:49:15, Benedikt Meurer wrote:
> Time::Now() has to use gettimeofday() on POSIX systems or CLOCK_REALTIME.

You mean CLOCK_REALTIME_COARSE is not allowed?  If so, why is that?

> It should return a Time value not a TimeTicks value.

Not sure what you mean.  Time::FromTimespec() returns a Time, right?

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode606
src/platform/time.cc:606: int result = GetBestMonotonicClock(&ts);
On 2013/10/30 12:49:15, Benedikt Meurer wrote:
> Please take a look at the Xorg GetTimeInMillis() function in os/utils.c. I
think
> we should use something similar here; can be safely inlined into the
> HighResolutionNow() function, IMHO.

What in particular do you want to steal^Wcopy?  The extra check for
CLOCK_MONOTONIC?

[Benedikt Meurer, 2013-10-30 13:50:04]

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode305
src/platform/time.cc:305: Time Time::Now() {
On 2013/10/30 13:38:56, ben2 wrote:
> On 2013/10/30 12:49:15, Benedikt Meurer wrote:
> > Time::Now() has to use gettimeofday() on POSIX systems or CLOCK_REALTIME.
> 
> You mean CLOCK_REALTIME_COARSE is not allowed?  If so, why is that?
> 
> > It should return a Time value not a TimeTicks value.
> 
> Not sure what you mean.  Time::FromTimespec() returns a Time, right?

CLOCK_MONOTONIC != CLOCK_REALTIME. Time values have to use the realtime clock.

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode606
src/platform/time.cc:606: int result = GetBestMonotonicClock(&ts);
On 2013/10/30 13:38:56, ben2 wrote:
> On 2013/10/30 12:49:15, Benedikt Meurer wrote:
> > Please take a look at the Xorg GetTimeInMillis() function in os/utils.c. I
> think
> > we should use something similar here; can be safely inlined into the
> > HighResolutionNow() function, IMHO.
> 
> What in particular do you want to steal^Wcopy?  The extra check for
> CLOCK_MONOTONIC?

    static clockid_t clockid;
    if (!clockid) {
#ifdef CLOCK_MONOTONIC_COARSE
        if (clock_getres(CLOCK_MONOTONIC_COARSE, &tp) == 0 &&
            (tp.tv_nsec / 1000) <= 1000 &&
            clock_gettime(CLOCK_MONOTONIC_COARSE, &tp) == 0)
            clockid = CLOCK_MONOTONIC_COARSE;
        else
#endif
            clockid = CLOCK_MONOTONIC;
    }
    int result = clock_gettime(clockid, &tp);

[bnoordhuis, 2013-10-30 14:18:40]

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode305
src/platform/time.cc:305: Time Time::Now() {
On 2013/10/30 13:50:04, Benedikt Meurer wrote:
> CLOCK_MONOTONIC != CLOCK_REALTIME. Time values have to use the realtime clock.

Trying to deepen my understanding here.  Is that because the return value of
Date.now() is backed by Time::Now()?

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode606
src/platform/time.cc:606: int result = GetBestMonotonicClock(&ts);
On 2013/10/30 13:50:04, Benedikt Meurer wrote:
>     static clockid_t clockid;
>     if (!clockid) {
> #ifdef CLOCK_MONOTONIC_COARSE
>         if (clock_getres(CLOCK_MONOTONIC_COARSE, &tp) == 0 &&
>             (tp.tv_nsec / 1000) <= 1000 &&
>             clock_gettime(CLOCK_MONOTONIC_COARSE, &tp) == 0)
>             clockid = CLOCK_MONOTONIC_COARSE;
>         else
> #endif
>             clockid = CLOCK_MONOTONIC;
>     }
>     int result = clock_gettime(clockid, &tp);

I'm a man of many qualities but mind reading is not one of them.  I know the
code but what in particular are the relevant bits?  The call to clock_getres()?

[Sven Panne, 2013-10-30 14:26:49]

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/1/src/platform/time.cc#newcode291
src/platform/time.cc:291: if (no_clock_monotonic_coarse == false) {
On 2013/10/30 13:38:56, ben2 wrote:
> Agree on the double negative but I wrote it this way to ensure that the flag
> ends up in the .bss rather than the .data section.
> 
> Not sure what you mean by locking. [...]

OK, I was a bit confused. What I meant was the fact that GCC protects the
initialization of a non-POD static with __cxa_guard_acquire /
__cxa_guard_release unless you specify-fno-threadsafe-statics. That's not the
case here, but our thread sanitizer tool will be unhappy about this
nevertheless, I guess...

[bnoordhuis, 2013-10-30 22:56:35]

Uploaded a new patch that only touches TimeTicks::HighResolutionNow() and checks
the clock resolution first.

[Benedikt Meurer, 2013-10-31 06:44:50]

I'm sorry for possible confusion. I just want to make sure that we don't break
some corner case, and therefore I'd prefer to rely on well-tested code from
Xorg.

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc#new...
src/platform/time.cc:581: static clock_t clock_id = -1;
I think we can safely use zero here, i.e. no explicit initialization (as Xorg
does).

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc#new...
src/platform/time.cc:583: if (clock_getres(CLOCK_MONOTONIC_COARSE, &ts) == 0 &&
This won't compile on non-Linux systems that miss the CLOCK_MONOTONIC_COARSE
define.

[bnoordhuis, 2013-10-31 12:22:23]

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc#new...
src/platform/time.cc:581: static clock_t clock_id = -1;
On 2013/10/31 06:44:50, Benedikt Meurer wrote:
> I think we can safely use zero here, i.e. no explicit initialization (as Xorg
> does).

I wrote it that way because 0 is a valid clock (CLOCK_REALTIME.)  I'm fine with
either though, just say the word.

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc#new...
src/platform/time.cc:583: if (clock_getres(CLOCK_MONOTONIC_COARSE, &ts) == 0 &&
On 2013/10/31 06:44:50, Benedikt Meurer wrote:
> This won't compile on non-Linux systems that miss the CLOCK_MONOTONIC_COARSE
> define.

If it weren't for the #if V8_OS_LINUX a few lines up, you mean? :-)

[Benedikt Meurer, 2013-11-04 09:09:57]

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc
File src/platform/time.cc (right):

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc#new...
src/platform/time.cc:581: static clock_t clock_id = -1;
On 2013/10/31 12:22:24, ben2 wrote:
> On 2013/10/31 06:44:50, Benedikt Meurer wrote:
> > I think we can safely use zero here, i.e. no explicit initialization (as
Xorg
> > does).
> 
> I wrote it that way because 0 is a valid clock (CLOCK_REALTIME.)  I'm fine
with
> either though, just say the word.

Please just use 0 here.

https://codereview.chromium.org/51333007/diff/120001/src/platform/time.cc#new...
src/platform/time.cc:583: if (clock_getres(CLOCK_MONOTONIC_COARSE, &ts) == 0 &&
On 2013/10/31 12:22:24, ben2 wrote:
> On 2013/10/31 06:44:50, Benedikt Meurer wrote:
> > This won't compile on non-Linux systems that miss the CLOCK_MONOTONIC_COARSE
> > define.
> 
> If it weren't for the #if V8_OS_LINUX a few lines up, you mean? :-)

Ups, right. Ok, that's fine.

[bnoordhuis, 2013-11-07 21:41:52]

linux: use CLOCK_MONOTONIC_COARSE v3

[bnoordhuis, 2013-11-07 21:44:23]

Sorry for the delay.  Uploaded v3 with the requested change, PTAL.

[bnoordhuis, 2013-11-07 22:20:25]

linux: use CLOCK_{REALTIME,MONOTONIC}_COARSE

[bnoordhuis, 2013-11-07 22:20:27]

linux: use CLOCK_{REALTIME,MONOTONIC}_COARSE

[bnoordhuis, 2013-11-07 22:20:27]

linux: use CLOCK_{REALTIME,MONOTONIC}_COARSE

[bnoordhuis, 2013-11-07 22:20:33]

linux: use CLOCK_{REALTIME,MONOTONIC}_COARSE

[bnoordhuis, 2013-11-07 22:21:39]

linux: use CLOCK_{REALTIME,MONOTONIC}_COARSE

[bnoordhuis, 2013-11-07 22:24:47]

Uploaded another take that also updates Time::Now() (but using
CLOCK_REALTIME_COARSE).

Sorry for the spam, the upload failed a few times with a 500 error.

[Benedikt Meurer, 2013-11-08 06:12:18]

On 2013/11/07 22:24:47, ben2 wrote:
> Uploaded another take that also updates Time::Now() (but using
> CLOCK_REALTIME_COARSE).
> 
> Sorry for the spam, the upload failed a few times with a 500 error.

Upload is still broken, please try again.

[bnoordhuis, 2013-11-10 14:53:16]

On 2013/11/08 06:12:18, Benedikt Meurer wrote:
> On 2013/11/07 22:24:47, ben2 wrote:
> > Uploaded another take that also updates Time::Now() (but using
> > CLOCK_REALTIME_COARSE).
> > 
> > Sorry for the spam, the upload failed a few times with a 500 error.
> 
> Upload is still broken, please try again.

It's the side-by-side diff that's botched (maybe because it's not a direct
descendant from the previous patch?)  The unified diff works though.

[Benedikt Meurer, 2013-11-11 07:48:27]

LGTM

[Benedikt Meurer, 2013-11-11 07:49:24]

Committed patchset #4 manually as r17611 (presubmit successful).

[Benedikt Meurer, 2013-11-11 08:49:32]

On 2013/11/11 07:49:24, Benedikt Meurer wrote:
> Committed patchset #4 manually as r17611 (presubmit successful).

We had to revert because it breaks build for Android cross-compiler host
toolchain, where we don't have librt and therefore also lack clock_getres() and
clock_gettime().

[bnoordhuis, 2013-11-11 13:15:38]

linux: use CLOCK_{REALTIME,MONOTONIC}_COARSE, v2

[bnoordhuis, 2013-11-11 13:18:51]

Uploaded a new patch that excludes Android.

I could rewrite the checks to `#if defined(CLOCK_REALTIME_COARSE)` and `#if
defined(CLOCK_MONOTONIC_COARSE)` so Android gets them for free if/when it adds
clock_getres/clock_gettime support.

[Benedikt Meurer, 2013-11-11 13:45:49]

On 2013/11/11 13:18:51, bnoordhuis wrote:
> Uploaded a new patch that excludes Android.
> 
> I could rewrite the checks to `#if defined(CLOCK_REALTIME_COARSE)` and `#if
> defined(CLOCK_MONOTONIC_COARSE)` so Android gets them for free if/when it adds
> clock_getres/clock_gettime support.

Please upload as new CL, thanks.