time: Add a static TimeTicks method that returns the underlying clock

time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
Committed: https://crrev.com/fdd7d49cc24ed079bfe164110970c25723d363a8
Cr-Commit-Position: refs/heads/master@{#384352}

[charliea (OOO until 10-5), 2016-03-21 17:29:53]

Patchset #1 (id:1) has been deleted

[charliea (OOO until 10-5), 2016-03-21 17:30:02]

Description was changed from

==========
tracing: Adds the clock being used as metadata to the trace

This CL is part of the broader "clock sync" architecture that we're
trying to build for tracing. This architecture aims to allow us to
combine multiple traces taken at the same time into a single trace
by writing clock sync markers into each trace and aligning the traces
based on those markers.

Two traces are implicitly synced, though, if they both use timestamps
from the same clock domain in the first place. In order for a consumer
of the traces to determine this, though, we need to write some sort of
a "clock domain identifier" into the trace.

This CL does that.

NOTE: Once this CL has been conceptually approved, I'll break it into
two pieces:

  - Adding ClockId() to base::TimeTicks
  - Writing the clock domain metadata event into the trace

DO NOT SUBMIT until this CL is broken apart.
==========

to

==========
tracing: Add the clock being used as metadata to the trace

This CL is part of the broader "clock sync" architecture that we're
trying to build for tracing. This architecture aims to allow us to
combine multiple traces taken at the same time into a single trace
by writing clock sync markers into each trace and aligning the traces
based on those markers.

Two traces are implicitly synced, though, if they both use timestamps
from the same clock domain in the first place. In order for a consumer
of the traces to determine this, though, we need to write some sort of
a "clock domain identifier" into the trace.

This CL does that.

NOTE: Once this CL has been conceptually approved, I'll break it into
two pieces:

  - Adding ClockId() to base::TimeTicks
  - Writing the clock domain metadata event into the trace

DO NOT SUBMIT until this CL is broken apart.
==========

[charliea (OOO until 10-5), 2016-03-21 17:33:44]

Patchset #1 (id:20001) has been deleted

[charliea (OOO until 10-5), 2016-03-21 17:35:16]

Patchset #1 (id:40001) has been deleted

[charliea (OOO until 10-5), 2016-03-21 17:35:22]

Patchset #1 (id:60001) has been deleted

[charliea (OOO until 10-5), 2016-03-23 19:24:24]

Description was changed from

==========
tracing: Add the clock being used as metadata to the trace

This CL is part of the broader "clock sync" architecture that we're
trying to build for tracing. This architecture aims to allow us to
combine multiple traces taken at the same time into a single trace
by writing clock sync markers into each trace and aligning the traces
based on those markers.

Two traces are implicitly synced, though, if they both use timestamps
from the same clock domain in the first place. In order for a consumer
of the traces to determine this, though, we need to write some sort of
a "clock domain identifier" into the trace.

This CL does that.

NOTE: Once this CL has been conceptually approved, I'll break it into
two pieces:

  - Adding ClockId() to base::TimeTicks
  - Writing the clock domain metadata event into the trace

DO NOT SUBMIT until this CL is broken apart.
==========

to

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
==========

[charliea (OOO until 10-5), 2016-03-23 19:24:37]

Patchset #1 (id:80001) has been deleted

[charliea (OOO until 10-5), 2016-03-23 19:25:46]

Description was changed from

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
==========

to

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
==========

[charliea (OOO until 10-5), 2016-03-23 19:26:10]

charliea@chromium.org changed reviewers:
+ nduca@chromium.org

[charliea (OOO until 10-5), 2016-03-23 19:27:50]

charliea@chromium.org changed reviewers:
+ oysteine@google.com
- nduca@chromium.org

[oystein (OOO til 10th of July), 2016-03-24 19:01:28]

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

[oystein (OOO til 10th of July), 2016-03-24 19:01:28]

Usage lg2m from a tracing-owner perspective.

[charliea (OOO until 10-5), 2016-03-24 19:21:21]

The CQ bit was checked by charliea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-24 19:21:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1824673002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1824673002/100001

[commit-bot: I haz the power, 2016-03-24 21:04:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 21:04:37]

Dry run: This issue passed the CQ dry run.

[charliea (OOO until 10-5), 2016-03-25 15:42:58]

charliea@chromium.org changed reviewers:
+ thestig@chromium.org

[charliea (OOO until 10-5), 2016-03-25 15:42:58]

thestig@, could you take a look for base/time ownership?

[charliea (OOO until 10-5), 2016-03-25 16:31:30]

charliea@chromium.org changed reviewers:
+ danakj@chromium.org
- thestig@chromium.org

[charliea (OOO until 10-5), 2016-03-25 16:31:31]

Doh, looks like thestig@ is OOO. danakj@, do you think you'd be able to take a
look?

[danakj, 2016-03-26 00:40:33]

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco...
base/time/time.h:686: static std::string ClockId();
it looks like this can return const char*.

i kinda am wondering why strings and not an enum. but maybe that's good enough
for you? it hides the platform-specific stuff in platform-specific files. but
also makes it very hard to tell what this might return.

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#...
base/time/time_win.cc:490: g_now_clock_id = now_clock_id;
Why is this after the fence?

[charliea (OOO until 10-5), 2016-03-28 15:40:51]

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco...
base/time/time.h:686: static std::string ClockId();
On 2016/03/26 00:40:33, danakj wrote:
> it looks like this can return const char*.
> 
> i kinda am wondering why strings and not an enum. but maybe that's good enough
> for you? it hides the platform-specific stuff in platform-specific files. but
> also makes it very hard to tell what this might return.

Definitely agree that it makes it hard to see the possible return values. The
main reason for the strings over the enums is that the primary way this will be
used is as metadata for Chrome traces. The primary consumer of this (at least
for right now) is going to be trace viewer (about:tracing). It's a whole
separate thing from Chrome itself and pulls in traces from a variety of sources
(Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine if
Chrome was using the same clock as one of these other tracers that was tracing
concurrently, allowing us to know if the two have timestamps on the same
timeline.

That being said, that's a lot of levels removed. Is there some idiomatic Chrome
way to map enums to strings for printing? If we had that, returning an enum
could be sufficient because we could just do something like:

  AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId()));

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#...
base/time/time_win.cc:490: g_now_clock_id = now_clock_id;
On 2016/03/26 00:40:33, danakj wrote:
> Why is this after the fence?

I'm not sure, but if the above comment is any indicator, it looks like it has to
do with making sure g_qpc_ticks_per_second is changed before the function
pointers. Naively, I'd expect that things should be okay as long as we change
g_now_function and g_now_clock_id in the same block.

[danakj, 2016-03-28 17:53:50]

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco...
base/time/time.h:686: static std::string ClockId();
On 2016/03/28 15:40:51, charliea wrote:
> On 2016/03/26 00:40:33, danakj wrote:
> > it looks like this can return const char*.
> > 
> > i kinda am wondering why strings and not an enum. but maybe that's good
enough
> > for you? it hides the platform-specific stuff in platform-specific files.
but
> > also makes it very hard to tell what this might return.
> 
> Definitely agree that it makes it hard to see the possible return values. The
> main reason for the strings over the enums is that the primary way this will
be
> used is as metadata for Chrome traces. The primary consumer of this (at least
> for right now) is going to be trace viewer (about:tracing). It's a whole
> separate thing from Chrome itself and pulls in traces from a variety of
sources
> (Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine if
> Chrome was using the same clock as one of these other tracers that was tracing
> concurrently, allowing us to know if the two have timestamps on the same
> timeline.
> 
> That being said, that's a lot of levels removed. Is there some idiomatic
Chrome
> way to map enums to strings for printing?

https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...
is a good example of doing that.

> If we had that, returning an enum
> could be sufficient because we could just do something like:
> 
>   AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId()));

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#...
base/time/time_win.cc:490: g_now_clock_id = now_clock_id;
On 2016/03/28 15:40:51, charliea wrote:
> On 2016/03/26 00:40:33, danakj wrote:
> > Why is this after the fence?
> 
> I'm not sure, but if the above comment is any indicator, it looks like it has
to
> do with making sure g_qpc_ticks_per_second is changed before the function
> pointers. Naively, I'd expect that things should be okay as long as we change
> g_now_function and g_now_clock_id in the same block.

Yeah, I stared at this for a while, and AFAICT the reason is because the now
function can use g_qpc_ticks_p_s, so if the pointer is updated we want the
g_qpc.. to be updated as well.

However, the function does not use the clock id. The fence is magical, so
putting things below makes them now depend on magic. So I'd prefer this was
added above then. Maybe just above g_qpc_t_p_s?

[danakj, 2016-03-28 17:54:21]

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco...
base/time/time.h:686: static std::string ClockId();
On 2016/03/28 17:53:50, danakj wrote:
> On 2016/03/28 15:40:51, charliea wrote:
> > On 2016/03/26 00:40:33, danakj wrote:
> > > it looks like this can return const char*.
> > > 
> > > i kinda am wondering why strings and not an enum. but maybe that's good
> enough
> > > for you? it hides the platform-specific stuff in platform-specific files.
> but
> > > also makes it very hard to tell what this might return.
> > 
> > Definitely agree that it makes it hard to see the possible return values.
The
> > main reason for the strings over the enums is that the primary way this will
> be
> > used is as metadata for Chrome traces. The primary consumer of this (at
least
> > for right now) is going to be trace viewer (about:tracing). It's a whole
> > separate thing from Chrome itself and pulls in traces from a variety of
> sources
> > (Windows ETW, Android atrace, etc.). We'll use these clock IDs to determine
if
> > Chrome was using the same clock as one of these other tracers that was
tracing
> > concurrently, allowing us to know if the two have timestamps on the same
> > timeline.
> > 
> > That being said, that's a lot of levels removed. Is there some idiomatic
> Chrome
> > way to map enums to strings for printing?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...
> is a good example of doing that.

(And I think that such a function would belong in the tracing code, not in
time.h)

> > If we had that, returning an enum
> > could be sufficient because we could just do something like:
> > 
> >   AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId()));
>

[charliea (OOO until 10-5), 2016-03-28 20:50:26]

Patchset #3 (id:140001) has been deleted

[charliea (OOO until 10-5), 2016-03-28 20:55:57]

Patchset #3 (id:160001) has been deleted

[charliea (OOO until 10-5), 2016-03-28 21:02:04]

Patchset #3 (id:180001) has been deleted

[charliea (OOO until 10-5), 2016-03-28 21:02:10]

Patchset #3 (id:200001) has been deleted

[charliea (OOO until 10-5), 2016-03-28 21:05:35]

Patchset #3 (id:220001) has been deleted

[charliea (OOO until 10-5), 2016-03-28 21:07:36]

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time.h#newco...
base/time/time.h:686: static std::string ClockId();
On 2016/03/28 17:54:21, danakj wrote:
> On 2016/03/28 17:53:50, danakj wrote:
> > On 2016/03/28 15:40:51, charliea wrote:
> > > On 2016/03/26 00:40:33, danakj wrote:
> > > > it looks like this can return const char*.
> > > > 
> > > > i kinda am wondering why strings and not an enum. but maybe that's good
> > enough
> > > > for you? it hides the platform-specific stuff in platform-specific
files.
> > but
> > > > also makes it very hard to tell what this might return.
> > > 
> > > Definitely agree that it makes it hard to see the possible return values.
> The
> > > main reason for the strings over the enums is that the primary way this
will
> > be
> > > used is as metadata for Chrome traces. The primary consumer of this (at
> least
> > > for right now) is going to be trace viewer (about:tracing). It's a whole
> > > separate thing from Chrome itself and pulls in traces from a variety of
> > sources
> > > (Windows ETW, Android atrace, etc.). We'll use these clock IDs to
determine
> if
> > > Chrome was using the same clock as one of these other tracers that was
> tracing
> > > concurrently, allowing us to know if the two have timestamps on the same
> > > timeline.
> > > 
> > > That being said, that's a lot of levels removed. Is there some idiomatic
> > Chrome
> > > way to map enums to strings for printing?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/scheduler/sched...
> > is a good example of doing that.
> 
> (And I think that such a function would belong in the tracing code, not in
> time.h)
> 
> > > If we had that, returning an enum
> > > could be sufficient because we could just do something like:
> > > 
> > >   AddTraceMetadata("clock-id", ClockIdToString(TimeTicks::ClockId()));
> > 
> 

Done.

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/100001/base/time/time_win.cc#...
base/time/time_win.cc:490: g_now_clock_id = now_clock_id;
On 2016/03/28 17:53:50, danakj wrote:
> On 2016/03/28 15:40:51, charliea wrote:
> > On 2016/03/26 00:40:33, danakj wrote:
> > > Why is this after the fence?
> > 
> > I'm not sure, but if the above comment is any indicator, it looks like it
has
> to
> > do with making sure g_qpc_ticks_per_second is changed before the function
> > pointers. Naively, I'd expect that things should be okay as long as we
change
> > g_now_function and g_now_clock_id in the same block.
> 
> Yeah, I stared at this for a while, and AFAICT the reason is because the now
> function can use g_qpc_ticks_p_s, so if the pointer is updated we want the
> g_qpc.. to be updated as well.
> 
> However, the function does not use the clock id. The fence is magical, so
> putting things below makes them now depend on magic. So I'd prefer this was
> added above then. Maybe just above g_qpc_t_p_s?

Done.

[charliea (OOO until 10-5), 2016-03-30 19:48:24]

/bump :)

[danakj, 2016-03-30 19:51:59]

https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h#newco...
base/time/time.h:647: UNKNOWN,
Not sure that you really need the UNKNOWN here. It just makes you write it in
switches and NOTREACHED() which is unneeded complexity. We never actually return
that. I think you could let the global win one just default init to 0.

LGTM otherwise.

[charliea (OOO until 10-5), 2016-03-30 20:07:44]

https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1824673002/diff/240001/base/time/time.h#newco...
base/time/time.h:647: UNKNOWN,
On 2016/03/30 19:51:58, danakj wrote:
> Not sure that you really need the UNKNOWN here. It just makes you write it in
> switches and NOTREACHED() which is unneeded complexity. We never actually
return
> that. I think you could let the global win one just default init to 0.
> 
> LGTM otherwise.

Done.

[charliea (OOO until 10-5), 2016-03-30 20:07:50]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2016-03-30 20:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1824673002/#ps260001
(title: "Removed Clock::UNKNOWN")

[commit-bot: I haz the power, 2016-03-30 20:08:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1824673002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1824673002/260001

[fmeawad, 2016-03-30 20:25:18]

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

[fmeawad, 2016-03-30 20:25:19]

https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc#...
base/time/time_win.cc:521: TimeTicks::Clock TimeTicks::GetClock() {
nit: I might be confused, but why not something like that instead:
return IsHighResolution? WIN_QPC : WIN_ROLLOVER_PROTECTED_TIME_GET_TIME;

[danakj, 2016-03-30 20:26:29]

https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc#...
base/time/time_win.cc:521: TimeTicks::Clock TimeTicks::GetClock() {
On 2016/03/30 20:25:19, fmeawad wrote:
> nit: I might be confused, but why not something like that instead:
> return IsHighResolution? WIN_QPC : WIN_ROLLOVER_PROTECTED_TIME_GET_TIME;

Oh, that's pretty nice yah.

[commit-bot: I haz the power, 2016-03-30 21:02:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-30 21:02:38]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[charliea (OOO until 10-5), 2016-03-31 15:38:39]

https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1824673002/diff/260001/base/time/time_win.cc#...
base/time/time_win.cc:521: TimeTicks::Clock TimeTicks::GetClock() {
On 2016/03/30 20:26:29, danakj wrote:
> On 2016/03/30 20:25:19, fmeawad wrote:
> > nit: I might be confused, but why not something like that instead:
> > return IsHighResolution? WIN_QPC : WIN_ROLLOVER_PROTECTED_TIME_GET_TIME;
> 
> Oh, that's pretty nice yah.

Agreed. That's a much nicer way of doing this. Done.

[charliea (OOO until 10-5), 2016-03-31 15:38:47]

The CQ bit was checked by charliea@chromium.org

[charliea (OOO until 10-5), 2016-03-31 15:38:47]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1824673002/#ps280001
(title: "Use IsHighResolution")

[commit-bot: I haz the power, 2016-03-31 15:39:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1824673002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1824673002/280001

[commit-bot: I haz the power, 2016-03-31 19:05:30]

Description was changed from

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
==========

to

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
==========

[commit-bot: I haz the power, 2016-03-31 19:05:32]

Committed patchset #5 (id:280001)

[commit-bot: I haz the power, 2016-03-31 19:07:33]

Description was changed from

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350
==========

to

==========
time: Add a static TimeTicks method that returns the underlying clock

In a follow-up CL, the return value of this method will be written into
traces recorded by Chrome so that, when syncing Chrome traces with
external traces, we have more information to determine whether the two
traces use timestamps from the same underlying clock and can therefore
be combined.

BUG=597350

Committed: https://crrev.com/fdd7d49cc24ed079bfe164110970c25723d363a8
Cr-Commit-Position: refs/heads/master@{#384352}
==========

[commit-bot: I haz the power, 2016-03-31 19:07:34]

Patchset 5 (id:??) landed as
https://crrev.com/fdd7d49cc24ed079bfe164110970c25723d363a8
Cr-Commit-Position: refs/heads/master@{#384352}