Use CLOCK_REALTIME_COARSE when available.

Use CLOCK_REALTIME_COARSE when available.

On systems that have CLOCK_REALTIME_COARSE with good enough resolution, we can
avoid making a system call to get the current time; it's serviced from the vDSO.

BUG=
LOG=N

Committed: https://crrev.com/537ed7500c087786f28f51ff5222f1c2113776d3
Cr-Commit-Position: refs/heads/master@{#28280}

[bnoordhuis, 2015-05-05 22:40:56]

ben@strongloop.com changed reviewers:
+ jochen@chromium.org

[Dean McNamee, 2015-05-06 09:09:34]

deanm@chromium.org changed reviewers:
+ deanm@chromium.org

[Dean McNamee, 2015-05-06 09:09:34]

Drive by...

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

https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
src/base/platform/time.cc:257: static clockid_t clock_id = -1;
I would maybe make a note that clockid_t is defined as an int.  The concern
would otherwise be, for example, if it were an int64 on 32-bit platforms and
this code is multithreaded, there could be a race on writing to clockid_t.  In
theory they would be writing the same value so it wouldn't really be a problem
but I usually think it's just good practice to make a note so that it is clear
that you considered the situation.

https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
src/base/platform/time.cc:261: static const clockid_t kClockRealTimeCoarse = 5;
Is there a reason you can't use the CLOCK_REALTIME_COARSE define?  Just header
versions?

https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
src/base/platform/time.cc:266: clock_id = kClockRealTimeCoarse;
I would make a note, if possible, about what the difference in precision is. 
The man page just says:

   CLOCK_REALTIME_COARSE (since Linux 2.6.32; Linux-specific)
              A faster but less precise version of CLOCK_REALTIME.  Use when
              you need very fast, but not fine-grained timestamps.

Are we talking milliseconds, microseconds, etc?

[Dean McNamee, 2015-05-06 09:17:11]

On 2015/05/06 09:09:34, Dean McNamee wrote:
> Drive by...
> 
> https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc
> File src/base/platform/time.cc (right):
> 
>
https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
> src/base/platform/time.cc:257: static clockid_t clock_id = -1;
> I would maybe make a note that clockid_t is defined as an int.  The concern
> would otherwise be, for example, if it were an int64 on 32-bit platforms and
> this code is multithreaded, there could be a race on writing to clockid_t.  In
> theory they would be writing the same value so it wouldn't really be a problem
> but I usually think it's just good practice to make a note so that it is clear
> that you considered the situation.
> 
>
https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
> src/base/platform/time.cc:261: static const clockid_t kClockRealTimeCoarse =
5;
> Is there a reason you can't use the CLOCK_REALTIME_COARSE define?  Just header
> versions?
> 
>
https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
> src/base/platform/time.cc:266: clock_id = kClockRealTimeCoarse;
> I would make a note, if possible, about what the difference in precision is. 
> The man page just says:
> 
>    CLOCK_REALTIME_COARSE (since Linux 2.6.32; Linux-specific)
>               A faster but less precise version of CLOCK_REALTIME.  Use when
>               you need very fast, but not fine-grained timestamps.
> 
> Are we talking milliseconds, microseconds, etc?

Does it make sense to check clock_getres?  Does the precision varying across
different hardware / kernel configurations?

[Benedikt Meurer, 2015-05-06 09:40:34]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-05-06 09:40:34]

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

https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
src/base/platform/time.cc:255: #if V8_OS_LINUX
How about using #ifdef CLOCK_REALTIME_COARSE here?
Amd ise CLOCK_REALTIME_COARSE below instead of the handwritten constant?

[bnoordhuis, 2015-05-06 10:44:13]

On 2015/05/06 09:09:34, Dean McNamee wrote:
>
https://codereview.chromium.org/1125003002/diff/1/src/base/platform/time.cc#n...
> src/base/platform/time.cc:261: static const clockid_t kClockRealTimeCoarse =
5;
> Is there a reason you can't use the CLOCK_REALTIME_COARSE define?  Just header
> versions?

Yes, it's not safe to assume the macro exists.

On 2015/05/06 09:17:11, Dean McNamee wrote:
> Does it make sense to check clock_getres?  Does the precision varying across
> different hardware / kernel configurations?

Yes, it depends on CONFIG_HZ.  The granularity can be as coarse as 300 ms.

I'll add the comments you suggested.

On 2015/05/06 09:40:34, Benedikt Meurer wrote:
> How about using #ifdef CLOCK_REALTIME_COARSE here?

See my reply to Dean.  The system headers may not define the macro; vice versa,
the headers may define it but the kernel may not actually support it.

Using an #ifdef guard would defeat the purpose for io.js; we compile our release
binaries on a CentOS 5 system with a 2.6.18 kernel for compatibility reasons and
it doesn't have CLOCK_REALTIME_COARSE.  Most users have much newer systems but
they wouldn't get the benefit of the faster clock if it were a compile-time
option.

[bnoordhuis, 2015-05-06 11:15:56]

Added the suggested comments.  I didn't explain what the vDSO is because IMO
that's information that's just one google query away.  PTAL.

[Benedikt Meurer, 2015-05-06 11:25:29]

https://codereview.chromium.org/1125003002/diff/20001/src/base/platform/time.cc
File src/base/platform/time.cc (right):

https://codereview.chromium.org/1125003002/diff/20001/src/base/platform/time....
src/base/platform/time.cc:255: #if V8_OS_LINUX
How about something like this instead:

#if V8_OS_LINUX && !defined(CLOCK_REALTIME_COARSE)
#define CLOCK_REALTIME_COARSE 5
#endif

...

#ifdef CLOCK_REALTIME_COARSE
  const clockid_t kInvalidClockId = static_cast<clock_id>(-1);
  static clockid_t clock_id = kInvalidClockId;
  if (V8_UNLIKELY(clock_id == kInvalidClockId)) {
    if (clock_getres(CLOCK_REALTIME_COARSE, &ts) < 0 || ts.tv_nsec > 1000 * 1000
|| ts.tv_sec > 0) {
      clock_id = CLOCK_REALTIME;
    } else {
      clock_id = CLOCK_REALTIME_COARSE;
    }
  }
  ...
#else
...
#endif

[noordhuis, 2015-05-06 11:50:02]

On 2015/05/06 11:25:29, Benedikt Meurer wrote:
>
https://codereview.chromium.org/1125003002/diff/20001/src/base/platform/time.cc
> File src/base/platform/time.cc (right):
> 
>
https://codereview.chromium.org/1125003002/diff/20001/src/base/platform/time....
> src/base/platform/time.cc:255: #if V8_OS_LINUX
> How about something like this instead:
> 
> #if V8_OS_LINUX && !defined(CLOCK_REALTIME_COARSE)
> #define CLOCK_REALTIME_COARSE 5
> #endif
> 
> ...
> 
> #ifdef CLOCK_REALTIME_COARSE
>   const clockid_t kInvalidClockId = static_cast<clock_id>(-1);
>   static clockid_t clock_id = kInvalidClockId;
>   if (V8_UNLIKELY(clock_id == kInvalidClockId)) {
>     if (clock_getres(CLOCK_REALTIME_COARSE, &ts) < 0 || ts.tv_nsec > 1000 *
1000
> || ts.tv_sec > 0) {
>       clock_id = CLOCK_REALTIME;
>     } else {
>       clock_id = CLOCK_REALTIME_COARSE;
>     }
>   }
>   ...
> #else
> ...
> #endif

I'll update it to your suggestion if you can explain to me why it's better than
the current approach.  It's different but I'm not sure I see how it's
qualitatively better.

[Benedikt Meurer, 2015-05-06 11:52:26]

IMHO it's better because it makes the main code not depend on V8_OS_LINUX, but
on the availability of a feature, i.e. CLOCK_REALTIME_COARSE. The V8_OS_LINUX
part is only 3 lines then and easy to spot.

[noordhuis, 2015-05-06 13:18:04]

On 2015/05/06 11:52:26, Benedikt Meurer wrote:
> IMHO it's better because it makes the main code not depend on V8_OS_LINUX, but
> on the availability of a feature, i.e. CLOCK_REALTIME_COARSE. The V8_OS_LINUX
> part is only 3 lines then and easy to spot.

Okay, updated; PTAL.

For background, the reason for the #if V8_OS_LINUX in v1 and v2 is to act as a
guard for platforms copying a Linux feature in a broken way.

[Dean McNamee, 2015-05-06 14:59:28]

On 2015/05/06 13:18:04, noordhuis wrote:
> On 2015/05/06 11:52:26, Benedikt Meurer wrote:
> > IMHO it's better because it makes the main code not depend on V8_OS_LINUX,
but
> > on the availability of a feature, i.e. CLOCK_REALTIME_COARSE. The
V8_OS_LINUX
> > part is only 3 lines then and easy to spot.
> 
> Okay, updated; PTAL.
> 
> For background, the reason for the #if V8_OS_LINUX in v1 and v2 is to act as a
> guard for platforms copying a Linux feature in a broken way.

Appreciate the additional comments around the clock details, thanks!

[Benedikt Meurer, 2015-05-07 04:16:39]

Thanks Ben. LGTM

[Benedikt Meurer, 2015-05-07 04:16:42]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-05-07 04:17:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125003002/40001

[commit-bot: I haz the power, 2015-05-07 05:06:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-07 05:06:16]

Patchset 3 (id:??) landed as
https://crrev.com/537ed7500c087786f28f51ff5222f1c2113776d3
Cr-Commit-Position: refs/heads/master@{#28280}

[Michael Achenbach, 2015-05-07 08:51:11]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1130083003/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] This causes layout test crashes:
http://build.chromium.org/p/client.v8/builders/V8-Blink%20Linux%2064%20%28dbg...

You can add the trybot v8_linux_layout_dbg on reland..

[yurys, 2015-05-07 10:17:19]

yurys@chromium.org changed reviewers:
+ yurys@chromium.org

[yurys, 2015-05-07 10:17:20]

Yeah, this code doesn't work well with chrome sandbox. I see the following
errors in the console and the renderer crashes on start.

../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
failure in syscall 0229
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
failure in syscall 0229
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
failure in syscall 0229
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
failure in syscall 0229

[noordhuis, 2015-05-07 12:54:34]

On 2015/05/07 10:17:20, yurys wrote:
> Yeah, this code doesn't work well with chrome sandbox. I see the following
> errors in the console and the renderer crashes on start.
> 
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> failure in syscall 0229
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> failure in syscall 0229
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> failure in syscall 0229
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> failure in syscall 0229

That's the clock_getres() system call.  I made a CL for that last year that was
accepted upstream in chromium/src/sandbox.  I'm not sure why it's not working
now.

[yurys, 2015-05-07 13:04:13]

On 2015/05/07 12:54:34, noordhuis wrote:
> On 2015/05/07 10:17:20, yurys wrote:
> > Yeah, this code doesn't work well with chrome sandbox. I see the following
> > errors in the console and the renderer crashes on start.
> > 
> >
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> > failure in syscall 0229
> >
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> > failure in syscall 0229
> >
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> > failure in syscall 0229
> >
>
../../sandbox/linux/seccomp-bpf-helpers/sigsys_handlers.cc:**CRASHING**:seccomp-bpf
> > failure in syscall 0229
> 
> That's the clock_getres() system call.  I made a CL for that last year that
was
> accepted upstream in chromium/src/sandbox.  I'm not sure why it's not working
> now.

I've no idea. I followed the instructions at
https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment to update
the sandbox binary but it didn't help. Can it be that you CL didn't land?

[noordhuis, 2015-05-07 15:39:54]

Figured it out, it's a new sandbox restriction.  I'll file a new CL once
https://codereview.chromium.org/1133653002/ lands.

Thanks for taking the time to update the sandbox, Yury.

[mdempsky, 2015-05-07 18:47:29]

mdempsky@chromium.org changed reviewers:
+ mdempsky@chromium.org

[mdempsky, 2015-05-07 18:47:29]

https://codereview.chromium.org/1125003002/diff/40001/src/base/platform/time.cc
File src/base/platform/time.cc (right):

https://codereview.chromium.org/1125003002/diff/40001/src/base/platform/time....
src/base/platform/time.cc:262: // clockid_t is an int32_t; loads and stores are
atomic.
No, they're not.  Using atomics is atomic.