Fixit: Fork base::TimeTicks --> TimeTicks + ThreadTicks + TraceTicks

Fixit: Fork base::TimeTicks --> TimeTicks + ThreadTicks + TraceTicks

TimeTicks was being overused for time values from three different clock
sources.  This change splits the class into three separate classes: The
general-purpose monotonic time (TimeTicks), the thread-local run time
(ThreadTicks), and the global system trace time (TraceTicks).  With this
change, the compiler is now able to use type-checking to guarantee
values from different clocks are not being mixed when doing time math.

This is the 2nd in a two-part change.  Part 1 factored-out the
comparison and math operator overloads common to base::Time and
base::TimeTicks into a templated base class.  The new ThreadTicks and
TraceTicks time classes also inherit from that base class.

Updated base/trace_event/* and a handful of outside-of-base uses of
ThreadNow() and NowFromSystemTraceTime() to use the new classes.  A bug
was identified and fixed, in src/ui/gl/angle_platform_impl.cc, where
values from TimeTicks::Now() were being erroneously provided to
base::TraceEvent instead of values from NowFromSystemTraceTime().

BUG=467417
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
NOTRY=true

Committed: https://crrev.com/1ab506aa9720454e32b815090a3fa3dd8d7145bc
Cr-Commit-Position: refs/heads/master@{#332080}

[miu, 2015-05-12 00:45:24]

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

[miu, 2015-05-12 00:45:25]

Now that part 1 [finally] landed successfully, here's part 2.  PTAL at
base/time/*, and if that checks out, I'll loop in specific owners for the
various dirs.

[Lei Zhang, 2015-05-13 23:31:47]

base/time* generally looks good. Some nits and random ramblings below.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/20001/base/time/time.h#newcode22
base/time/time.h:22: //
It'll be good to write some more comments here to give guidance on how to use
the new classes.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_mac.cc
File base/time/time_mac.cc (right):

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_mac.cc#n...
base/time/time_mac.cc:42: uint64_t absolute_micro;
Also use CheckedNumeric<int64_t> here?

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:84: // Helper function to get results from
clock_gettime() as TimeTicks object.
Comment needs updating.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:91: uint64_t absolute_micro;
Maybe just use CheckedNumeric<int64_t> and forget all the casting below?

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:337: uint64_t absolute_micro;
Also use CheckedNumeric<int64_t> here?

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:346: (static_cast<int64>(ts.tv_sec) *
Time::kMicrosecondsPerSecond) +
This is a repeat of line 100-101...

[mithro-old, 2015-05-19 01:25:57]

mithro@mithis.com changed reviewers:
+ mithro@mithis.com

[mithro-old, 2015-05-19 01:25:58]

LGTM at a high level, thanks for doing this! I'll leave it to Lei Zhang for a
more detailed review.

Tim 'mithro' Ansell

[miu, 2015-05-19 03:14:32]

thestig: Addressed all coments.  PTAL.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/20001/base/time/time.h#newcode22
base/time/time.h:22: //
On 2015/05/13 23:31:47, Lei Zhang wrote:
> It'll be good to write some more comments here to give guidance on how to use
> the new classes.

Done.  I added a "So many choices! / Examples" section to help guide developers
based on their use case.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_mac.cc
File base/time/time_mac.cc (right):

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_mac.cc#n...
base/time/time_mac.cc:42: uint64_t absolute_micro;
On 2015/05/13 23:31:47, Lei Zhang wrote:
> Also use CheckedNumeric<int64_t> here?

Done.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:84: // Helper function to get results from
clock_gettime() as TimeTicks object.
On 2015/05/13 23:31:47, Lei Zhang wrote:
> Comment needs updating.

Done.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:91: uint64_t absolute_micro;
On 2015/05/13 23:31:47, Lei Zhang wrote:
> Maybe just use CheckedNumeric<int64_t> and forget all the casting below?

Done.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:337: uint64_t absolute_micro;
On 2015/05/13 23:31:47, Lei Zhang wrote:
> Also use CheckedNumeric<int64_t> here?

Done.

https://codereview.chromium.org/1122153002/diff/20001/base/time/time_posix.cc...
base/time/time_posix.cc:346: (static_cast<int64>(ts.tv_sec) *
Time::kMicrosecondsPerSecond) +
On 2015/05/13 23:31:47, Lei Zhang wrote:
> This is a repeat of line 100-101...

Done.  Factored repeated code into a new ConvertTimespecToMicros() function in
the anonymous namespace.

[miu, 2015-05-19 03:21:40]

miu@chromium.org changed reviewers:
+ cpu@chromium.org, jln@chromium.org, primiano@chromium.org

[miu, 2015-05-19 03:21:41]

Adding all remaining reviewers:

cpu: Need quick-look and broad-scope OWNERS approval for changes in all
non-base/ dirs.

jln: Security review for components/tracing/tracing_messages.h

primiano: base/trace_event/*

[cpu_(ooo_6.6-7.5), 2015-05-19 19:12:02]

this seems a good thing to do, some initial comments

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode7
base/time/time.h:7: // (1601-01-01 00:00:00 UTC) (See http://crbug.com/14734).
System-dependent
that bug has been fixed

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode32
base/time/time.h:32: // So many choices!  Which time class should you use? 
Examples:
also a table of OS support and resolution would be very helpful

class        OS          resolution         max / min
====================================================
TimeFoo      WLM           1 femtosecobd    0 - 5 years
TimBar        LC           1 second         1600 ad - 2525 ad

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode52
base/time/time.h:52: //                directly by the event tracing
infrastructure.
Why expose TraceTicks here then?  lets make it part of tracing.

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcod...
base/time/time.h:779: // neither are avalable. On systems where a global trace
clock is defined,
on windows for example it is not clear, it seems it can draw from any of 3
systems?

[miu, 2015-05-20 21:58:46]

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode7
base/time/time.h:7: // (1601-01-01 00:00:00 UTC) (See http://crbug.com/14734).
System-dependent
On 2015/05/19 19:12:02, cpu wrote:
> that bug has been fixed

Done.

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode32
base/time/time.h:32: // So many choices!  Which time class should you use? 
Examples:
On 2015/05/19 19:12:01, cpu wrote:
> also a table of OS support and resolution would be very helpful

I'm not sure how to build such a table.  There's a complex matrix of OS vs.
resolution vs. range with a lot of miscellaneous "exceptions" mixed in (e.g.,
older hardware running Windows).  Also, for the range of values for the tick
classes, many platforms refuse to define in their OS API documentation what
their epoch time is, so there's no way to know this.

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode52
base/time/time.h:52: //                directly by the event tracing
infrastructure.
On 2015/05/19 19:12:02, cpu wrote:
> Why expose TraceTicks here then?  lets make it part of tracing.

There are some code modules (mostly in GPU and compositing code) that record
TraceTicks values and provide them directly to the event tracing infrastructure.
 I didn't analyze whether these external uses of TraceTicks were appropriate. 
Even if they are not, I'd prefer to scope that clean-up in a separate CL.  ;-)

For now, I've rewritten this to mention that all uses of TraceTicks in new code
be reviewed by one of the base/trace_event/OWNERS.  SGTY?

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcod...
base/time/time.h:779: // neither are avalable. On systems where a global trace
clock is defined,
On 2015/05/19 19:12:01, cpu wrote:
> on windows for example it is not clear, it seems it can draw from any of 3
> systems?

That is correct.  BTW--I didn't modify this comment from where it was in the old
code (the comment for TimeTicks::NowFromSystemTraceTime()).

I'm not sure how I should proceed w.r.t. changing this comment.  Basically, it
seems like I'd have to distill all the comments in time_win.cc, which would
probably add a dozen LOC or more to this comment, and is probably too much
implementation detail for readers of this header file.

[miu, 2015-05-20 22:00:26]

miu@chromium.org changed reviewers:
+ dcheng@chromium.org
- jln@chromium.org

[miu, 2015-05-20 22:00:27]

dcheng: Looks like jln@ is OOO.  Can you do a security review for
components/tracing/tracing_messages.h?

[dcheng, 2015-05-20 22:09:28]

I'll take a look first thing tomorrow, if that's OK.

On Wed, May 20, 2015, 15:00  <miu@chromium.org> wrote:

> dcheng: Looks like jln@ is OOO.  Can you do a security review for
> components/tracing/tracing_messages.h?
>
>
> https://codereview.chromium.org/1122153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cpu_(ooo_6.6-7.5), 2015-05-21 00:47:34]

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode25
base/time/time.h:25: // instance. Thus, they can be efficiently passed by-value
(as opposed to
all signed 64 ints?

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode32
base/time/time.h:32: // So many choices!  Which time class should you use? 
Examples:
On 2015/05/20 21:58:46, miu wrote:
> On 2015/05/19 19:12:01, cpu wrote:
> > also a table of OS support and resolution would be very helpful
> 
> I'm not sure how to build such a table.  There's a complex matrix of OS vs.
> resolution vs. range with a lot of miscellaneous "exceptions" mixed in (e.g.,
> older hardware running Windows).  Also, for the range of values for the tick
> classes, many platforms refuse to define in their OS API documentation what
> their epoch time is, so there's no way to know this.

Exactly my point. If you split 1 class in 3 and there is too complex of a
guidance when to use each then I don't know we are in better shape. Well, that
was for effect, certainly you know when to use each and so you should share this
knowledge which you are doing below but in a way that feels incomplete to me.

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode52
base/time/time.h:52: //                directly by the event tracing
infrastructure.
On 2015/05/20 21:58:46, miu wrote:
> On 2015/05/19 19:12:02, cpu wrote:
> > Why expose TraceTicks here then?  lets make it part of tracing.
> 
> There are some code modules (mostly in GPU and compositing code) that record
> TraceTicks values and provide them directly to the event tracing
infrastructure.
>  I didn't analyze whether these external uses of TraceTicks were appropriate. 
> Even if they are not, I'd prefer to scope that clean-up in a separate CL.  ;-)
> 
> For now, I've rewritten this to mention that all uses of TraceTicks in new
code
> be reviewed by one of the base/trace_event/OWNERS.  SGTY?

I follow on CL is fine with me but I will pest you if it does not happen :)

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcod...
base/time/time.h:779: // neither are avalable. On systems where a global trace
clock is defined,
On 2015/05/20 21:58:46, miu wrote:
> On 2015/05/19 19:12:01, cpu wrote:
> > on windows for example it is not clear, it seems it can draw from any of 3
> > systems?
> 
> That is correct.  BTW--I didn't modify this comment from where it was in the
old
> code (the comment for TimeTicks::NowFromSystemTraceTime()).
> 
> I'm not sure how I should proceed w.r.t. changing this comment.  Basically, it
> seems like I'd have to distill all the comments in time_win.cc, which would
> probably add a dozen LOC or more to this comment, and is probably too much
> implementation detail for readers of this header file.

Just basically say that: On windows things are complicated and it than come from
x, y, z so you want to familiarize yourself with the documentation in <file>

[Primiano Tucci (use gerrit), 2015-05-21 08:11:33]

primiano@chromium.org changed reviewers:
+ dsinclair@chromium.org, nduca@chromium.org

[Primiano Tucci (use gerrit), 2015-05-21 08:11:34]

The code changes themselves in base/trace_event and the general purpose of the
CL look ok to me, but I'd like long standing trace OWNERS (either nduca or
dsinclair) to have the final stamp on this, as I might be overlooking something.
Sorry for the extra hop.

[dcheng, 2015-05-21 18:49:04]

ipc changes lgtm

https://codereview.chromium.org/1122153002/diff/60001/base/time/time_mac.cc
File base/time/time_mac.cc (right):

https://codereview.chromium.org/1122153002/diff/60001/base/time/time_mac.cc#n...
base/time/time_mac.cc:25: int64 ComputeCurrentTicks() {
Nit: we should be using int64_t from <stdint.h>, not int64 (this comments
applies to all sized-integer types).

Also, why are we changing from unsigned to signed?

[miu, 2015-05-22 23:01:03]

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode32
base/time/time.h:32: // So many choices!  Which time class should you use? 
Examples:
On 2015/05/21 00:47:34, cpu wrote:
> Exactly my point. If you split 1 class in 3 and there is too complex of a
> guidance when to use each then I don't know we are in better shape.

Okay, so, I think we have different perspectives on what needs to be in the
header file w.r.t. comments and what the new code organization is doing.  Let me
explain my perspective:

Before this change, all of TimeTicks, ThreadTicks and TraceTicks were mashed
together as one TimeTicks class with three separate "Now()" functions.  The
"guidance" I think you're looking for was already provided in code comments for
each of the Now() functions, and this change *fully retains* those comments
where each of the three Now() functions has moved to.

I see your point that 4 classes instead of 2 seems more complex.  However, IMO,
this actually makes it *easier* for the developer: They can now easily
understand that there are many clock options and behaviors, choose the right
one, and not be able to misuse it; because the solutions are organized around
specific categories of use cases.

Furthermore, the comments here should make this organization even clearer, and
help developers quickly figure out what they should be doing.  IMO, adding
details around platform support, resolution of the clock, etc. would only serve
to confuse a developer.  It's premature to be thinking about those things before
they have selected the right class for their use case.

As to your concern about "incompleteness" in the comments, this might be where
our personal opinions diverge: Many times, a developer just needs a time source
for a simple use case, and isn't really concerned with per-platform
implementation details.  In a lot of Chromium code, I've seen that when those
details matter, developers know to dig deeper by examining the implementation
behind the header file.  Thus, I'm reluctant to include every little detail and
exception in the header file comments which, IMO, would only serve to overwhelm
a developer.

Finally, I'm not saying this change makes base/time/time.h perfect.  There is
certainly other clean-up work and usability enhancements that can be made.  But,
at some point, this already-large change has to be bounded in scope.  ;-)

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcode52
base/time/time.h:52: //                directly by the event tracing
infrastructure.
On 2015/05/21 00:47:34, cpu wrote:
> On 2015/05/20 21:58:46, miu wrote:
> > On 2015/05/19 19:12:02, cpu wrote:
> > > Why expose TraceTicks here then?  lets make it part of tracing.
> > 
> > There are some code modules (mostly in GPU and compositing code) that record
> > TraceTicks values and provide them directly to the event tracing
> infrastructure.
> >  I didn't analyze whether these external uses of TraceTicks were
appropriate. 
> > Even if they are not, I'd prefer to scope that clean-up in a separate CL. 
;-)
> > 
> > For now, I've rewritten this to mention that all uses of TraceTicks in new
> code
> > be reviewed by one of the base/trace_event/OWNERS.  SGTY?
> 
> I follow on CL is fine with me but I will pest you if it does not happen :)

Acknowledged.

https://codereview.chromium.org/1122153002/diff/40001/base/time/time.h#newcod...
base/time/time.h:779: // neither are avalable. On systems where a global trace
clock is defined,
On 2015/05/21 00:47:34, cpu wrote:
> Just basically say that: On windows things are complicated and it than come
from
> x, y, z so you want to familiarize yourself with the documentation in <file>

Ah.  Okay.  Done!  :)

https://codereview.chromium.org/1122153002/diff/60001/base/time/time_mac.cc
File base/time/time_mac.cc (right):

https://codereview.chromium.org/1122153002/diff/60001/base/time/time_mac.cc#n...
base/time/time_mac.cc:25: int64 ComputeCurrentTicks() {
On 2015/05/21 18:49:04, dcheng wrote:
> Nit: we should be using int64_t from <stdint.h>, not int64 (this comments
> applies to all sized-integer types).

Done.  Sounds like Chromium code throughout should be cleaned up...?

> Also, why are we changing from unsigned to signed?

The time classes use signed 64-bit ints, and C++'s auto-casting from unsigned to
signed integers without warning can lead to subtle bugs (e.g., 2^64-1 unsigned
--> -1 signed).

[miu, 2015-05-27 20:21:01]

Ping to everyone!  :)

cpu: I responded to your comments and made some changes.  I need OWNERS lgtm for
files in *all* dirs.

dsinclair: Would appreciate you taking a look at all the base/trace_event/*
changes.

thestig: Addressed your base/time/* comments awhile back.  I assume things all
LGTY now?

[cpu_(ooo_6.6-7.5), 2015-05-28 22:48:11]

lgtm

[Lei Zhang, 2015-05-28 22:54:08]

Yep, you've addressed all my previous comments. lgtm.

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h#newcode18
base/time/time.h:18: // durations. Internally, they are represented in
microseconds. They can not be
nit: there's space on the previous line.

[dsinclair, 2015-05-29 14:38:10]

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h#newcod...
base/time/time.h:765: class BASE_EXPORT TraceTicks : public
time_internal::TimeBase<TraceTicks> {
How is a TraceTick different from a normal TimeTick?

[miu, 2015-05-29 17:34:25]

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h#newcod...
base/time/time.h:765: class BASE_EXPORT TraceTicks : public
time_internal::TimeBase<TraceTicks> {
On 2015/05/29 14:38:10, dsinclair wrote:
> How is a TraceTick different from a normal TimeTick?

It is generated from a different clock source on most platforms.

[dsinclair, 2015-05-29 17:39:22]

base/trace_event lgtm

[miu, 2015-05-29 19:54:18]

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/1122153002/diff/80001/base/time/time.h#newcode18
base/time/time.h:18: // durations. Internally, they are represented in
microseconds. They can not be
On 2015/05/28 22:54:08, Lei Zhang wrote:
> nit: there's space on the previous line.

Done.

[miu, 2015-05-29 20:22:49]

Thanks everyone for reviewing!  :)

[miu, 2015-05-29 20:22:57]

The CQ bit was checked by miu@chromium.org

[miu, 2015-05-29 20:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com,
dcheng@chromium.org, thestig@chromium.org, dsinclair@chromium.org,
cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1122153002/#ps120001
(title: "REBASE")

[commit-bot: I haz the power, 2015-05-29 20:24:32]

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

[commit-bot: I haz the power, 2015-05-29 23:41:37]

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

[commit-bot: I haz the power, 2015-05-29 23:41:40]

Failed to apply patch for ipc/ipc_message_utils.h:
While running git apply --index -3 -p1;
  error: patch failed: ipc/ipc_message_utils.h:503
  Falling back to three-way merge...
  Applied patch to 'ipc/ipc_message_utils.h' with conflicts.
  U ipc/ipc_message_utils.h

Patch:       ipc/ipc_message_utils.h
Index: ipc/ipc_message_utils.h
diff --git a/ipc/ipc_message_utils.h b/ipc/ipc_message_utils.h
index
6787c8e2eb80dd0a6da16009c6eff03bedfe959e..068fa43858b5b9121e5a4fdbeb00596580406750
100644
--- a/ipc/ipc_message_utils.h
+++ b/ipc/ipc_message_utils.h
@@ -55,6 +55,7 @@ class NullableString16;
 class Time;
 class TimeDelta;
 class TimeTicks;
+class TraceTicks;
 struct FileDescriptor;
 }
 
@@ -503,6 +504,14 @@ struct IPC_EXPORT ParamTraits<base::TimeTicks> {
 };
 
 template <>
+struct IPC_EXPORT ParamTraits<base::TraceTicks> {
+  typedef base::TraceTicks param_type;
+  static void Write(Message* m, const param_type& p);
+  static bool Read(const Message* m, PickleIterator* iter, param_type* r);
+  static void Log(const param_type& p, std::string* l);
+};
+
+template <>
 struct ParamTraits<Tuple<>> {
   typedef Tuple<> param_type;
   static void Write(Message* m, const param_type& p) {

[commit-bot: I haz the power, 2015-05-29 23:42:37]

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

[miu, 2015-05-29 23:52:14]

The CQ bit was checked by miu@chromium.org

[miu, 2015-05-29 23:52:15]

The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com,
thestig@chromium.org, dsinclair@chromium.org, cpu@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1122153002/#ps130001
(title: "REBASE after it passed CQ but did not commit to tree")

[commit-bot: I haz the power, 2015-05-29 23:52:43]

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

[commit-bot: I haz the power, 2015-05-29 23:57:22]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-05-29 23:58:12]

Patchset 8 (id:??) landed as
https://crrev.com/1ab506aa9720454e32b815090a3fa3dd8d7145bc
Cr-Commit-Position: refs/heads/master@{#332080}