Use CLOCK_MONOTONIC_RAW for TimeTicks on Posix systems.

Use CLOCK_MONOTONIC_RAW for TimeTicks on Posix systems.

BUG=internal b/29618700

[jameswest, 2016-07-20 23:18:18]

Description was changed from

==========
Use CLOCK_MONOTONIC_RAW for TimeTicks on Posix systems.

BUG=internal b/29618700
==========

to

==========
Use CLOCK_MONOTONIC_RAW for TimeTicks on Posix systems.

BUG=internal b/29618700
==========

[jameswest, 2016-07-20 23:18:18]

jameswest@google.com changed reviewers:
+ alokp@chromium.org, kmackay@chromium.org, miu@chromium.org

[miu, 2016-07-27 00:28:28]

This seems fine, but there are a several spots throughout the code base where
devs have been assuming CLOCK_MONOTONIC is being used. Just grepping the code
base and spot-checking a few, these things look like they could break:

base/synchronization/condition_variable_posix.cc (wait times on cond variables)
base/third_party/libevent/README.chromium
ui/events/ozone/evdev/input_device_factory_evdev.cc (UI input event timestamps)
ui/events/ozone/evdev/libgestures_glue/gesture_timer_provider.cc
ui/gl/sync_control_vsync_provider.cc (vsync timestamps)

...and for the sandbox, it seems there's a test that explicitly disallows the
_RAW variant (maybe this is only NaCl-related, I'm not sure):

sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc:95

I'd suggest you loop in the owners for these on this review. You should probably
also e-mail an announcement to chromium-dev@ for extra safety.

[jameswest, 2016-07-27 23:24:54]

jameswest@google.com changed reviewers:
+ jln@chromium.org, mdempsky@chromium.org, piman@chromium.org,
spang@chromium.org

[mdempsky, 2016-07-27 23:40:46]

I don't recall any security-relevant concerns to CLOCK_MONOTONIC vs
CLOCK_MONOTONIC_RAW.  I think that sandbox test is just to make sure filtering
is working as intended, and CLOCK_MONOTONIC_RAW happens to not be allowed
currently.

That said, CLOCK_MONOTONIC_RAW is a Linux extension.  What does this code do on
non-Linux POSIX systems?

[jameswest, 2016-07-28 00:30:36]

I can add a check if CLOCK_MONOTONIC_RAW is defined and fall back to
CLOCK_MONOTONIC if it's not. That would mean that everything needs to handle it
being either one depending on the system.

Since this code review has some new reviewers and doesn't explain the reason for
the change, it came from this code review:
https://codereview.chromium.org/2101303004/. We require CLOCK_MONOTONIC_RAW for
synchronizing playback across Cast devices in multi-room groups. We'd like to
start using a Chromium API with TimeTicks, and it will be easier if TickTicks
uses the same clock. All Cast receiver devices use a Linux OS.

[spang, 2016-07-28 00:41:04]

On 2016/07/27 00:28:28, miu wrote:
> This seems fine, but there are a several spots throughout the code base where
> devs have been assuming CLOCK_MONOTONIC is being used. Just grepping the code
> base and spot-checking a few, these things look like they could break:
> 
> base/synchronization/condition_variable_posix.cc (wait times on cond
variables)
> base/third_party/libevent/README.chromium
> ui/events/ozone/evdev/input_device_factory_evdev.cc (UI input event
timestamps)

We can't use monotonic raw for event timestamps - the kernel doesn't support
this.

Trying to changing the default clock is very ambitious, and I'm not sure it's
realistic. CLOCK_MONOTONIC is the right clock to use for many (most?) of the
current users of TimeTicks.

Maybe add a new time class for your use case?

> ui/events/ozone/evdev/libgestures_glue/gesture_timer_provider.cc
> ui/gl/sync_control_vsync_provider.cc (vsync timestamps)
> 
> ...and for the sandbox, it seems there's a test that explicitly disallows the
> _RAW variant (maybe this is only NaCl-related, I'm not sure):
> 
>
sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions_unittests.cc:95
> 
> I'd suggest you loop in the owners for these on this review. You should
probably
> also e-mail an announcement to chromium-dev@ for extra safety.

[jameswest, 2016-07-28 01:31:31]

> We can't use monotonic raw for event timestamps - the kernel doesn't support
> this.
> 
> Trying to changing the default clock is very ambitious, and I'm not sure it's
> realistic. CLOCK_MONOTONIC is the right clock to use for many (most?) of the
> current users of TimeTicks.
> 
> Maybe add a new time class for your use case?

I think we have two options then:

1) Continue to use TimeTicks with CLOCK_MONOTONIC in
AudioSourceCallback::OnMoreData. On Cast devices the actual timestamp will come
from ALSA or the SoC as a raw integer representing CLOCK_MONOTONIC_RAW. We can
pass that integer via TimeTicks using the serialization/deserialization methods.
It's hacky, but the hack will be isolated to internal Chromecast code.

2) Create a new class, say AudioTimestamp, that is used in
AudioSourceCallback::OnMoreData and explicitly supports both use cases.

[miu, 2016-07-30 22:40:23]

I initially liked the idea of moving TimeTicks over to CLOCK_MONOTONIC_RAW,
since that's what we do on Windows (QPC() is a constant-frequency high-res
clock). However, now that we've heard back from other stakeholders, it's seems
that we should not proceed with this change.

I'll comment on the audio design on the other code review
(https://codereview.chromium.org/2101303004/).

[spang, 2016-08-02 15:20:33]

On 2016/07/30 22:40:23, miu wrote:
> I initially liked the idea of moving TimeTicks over to CLOCK_MONOTONIC_RAW,
> since that's what we do on Windows (QPC() is a constant-frequency high-res
> clock). However, now that we've heard back from other stakeholders, it's seems
> that we should not proceed with this change.

Thanks.

Another argument against is that CLOCK_MONOTONIC is exposed through the VDSO on
some architectures including x86 & arm64. No architectures optimize access to
CLOCK_MONOTONIC_RAW in this way. So another argument against is that it'd
regress performance.

>  
> I'll comment on the audio design on the other code review
> (https://codereview.chromium.org/2101303004/).