Add interface to system trace clock.

Add interface to system trace clock.

Add TimeTicks::NowFromSystemTraceTime to return the current system trace
event clock (falling back to TimeTicks::HighResNow when not supported).
On systems where the system trace clock is available this allows
system and chrome events to be stamped with a common clock (eliminating
post-processing time shift adjustment that can be hard).

BUG=chromium-os:27809
TEST=unit tests + collect trace data on chrome os w/ kernel that supports clock_gettime(CLOCK_SYSTEM_TRACE)

Change-Id: Ia6a500bd13a6f09c49052a1dd1421027960d8dea


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134772

[nduca, 2012-04-27 22:14:10]

I'm fine with this. +jbates for review.

[jbates, 2012-04-27 22:18:52]

LGTM

[jar (doing other things), 2012-04-27 22:50:18]

...a few questions and suggestions.

http://codereview.chromium.org/10257020/diff/1/base/time.h
File base/time.h (right):

http://codereview.chromium.org/10257020/diff/1/base/time.h#newcode489
base/time.h:489: // collected outside.
What do you mean by "Inside Chrome" vs outside?  Do you mean other processes,
such as renderer processes, etc?

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode522
base/time_unittest.cc:522: const int kTargetGranularityUs = 15000;  // 15ms
Ordinary system clocks can be set to run faster than 15ms.  It is common for
multi-media folks to set it down to closer to a few ms.  Don't you expect your
clock to reveal intervals shorter than 1ms?

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode525
base/time_unittest.cc:525: int retries = 100;  // Arbitrary.
100 sounds plausible.... but it is always nice to maximally avoid flakiness
(false failures).  You can set this really high, and then, if you take more than
about 3 or 5 minutes (don't recall the amount), then your test will fail for
"taking too long."  This creates a "design for success," as this will generally
complete really fast, and you'll be sure you are working.  When it fails, it
should be a big surprise, and we don't mind taking a long time.

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode534
base/time_unittest.cc:534: } while (delta.InMilliseconds() == 0);
Why don't you look for any change?  Why does the millisecond value have to have
changed?? Why not:
while (delta != TimeTicks())

Is there some other interesting characteristic you're watching for?

The above test will only watch for sub-granularity changes *when* we cross a
millisecond boundary.

[Sam Leffler, 2012-04-27 22:56:07]

http://codereview.chromium.org/10257020/diff/1/base/time.h
File base/time.h (right):

http://codereview.chromium.org/10257020/diff/1/base/time.h#newcode489
base/time.h:489: // collected outside.
Anything outside chrome the application.  Events coming from the kernel, X
server, etc.

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode522
base/time_unittest.cc:522: const int kTargetGranularityUs = 15000;  // 15ms
This is c&p of the HighRes test; I did not write this.

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode525
base/time_unittest.cc:525: int retries = 100;  // Arbitrary.
I don't see the HighRes test failing so think this is ok.  The new clock source
doesn't (so far) provide any significantly different/better properties than that
clock source and on all but cros is idential.

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode534
base/time_unittest.cc:534: } while (delta.InMilliseconds() == 0);
Again, this is just the existing HighRes test; if you want me to write an
entirely new test I can do that.

[jar (doing other things), 2012-04-30 22:05:57]

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode522
base/time_unittest.cc:522: const int kTargetGranularityUs = 15000;  // 15ms
On 2012/04/27 22:56:07, Sam Leffler wrote:
> This is c&p of the HighRes test; I did not write this.

I see now.  I read this as new code, and made comments, but it appears you cut
and paste the previous test to create this one.

Rather than cutting/pasting, how about factoring out the common code, and using
it?

For example, you can pass in the pointer to the function you want tested into a
shared routine.

Replication of code tends to just grow and grow in these unit tests... so shared
code is a good thing.

[Sam Leffler, 2012-04-30 22:47:58]

PTAL

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc
File base/time_unittest.cc (right):

http://codereview.chromium.org/10257020/diff/1/base/time_unittest.cc#newcode522
base/time_unittest.cc:522: const int kTargetGranularityUs = 15000;  // 15ms
On 2012/04/30 22:05:57, jar wrote:
> On 2012/04/27 22:56:07, Sam Leffler wrote:
> > This is c&p of the HighRes test; I did not write this.
> 
> I see now.  I read this as new code, and made comments, but it appears you cut
> and paste the previous test to create this one.
> 
> Rather than cutting/pasting, how about factoring out the common code, and
using
> it?
> 
> For example, you can pass in the pointer to the function you want tested into
a
> shared routine.
> 
> Replication of code tends to just grow and grow in these unit tests... so
shared
> code is a good thing.

Done.

[jar (doing other things), 2012-05-01 18:01:04]

lgtm

[commit-bot: I haz the power, 2012-05-01 18:01:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sleffler@chromium.org/10257020/12002

[commit-bot: I haz the power, 2012-05-01 18:26:17]

Try job failure for 10257020-12002 (retry) on win_rel for step "update".
It's a second try, previously, step "update" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-01 18:54:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sleffler@chromium.org/10257020/12002

[commit-bot: I haz the power, 2012-05-01 20:57:10]

Change committed as 134772