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

base/time/time.h

Index: base/time/time.h
diff --git a/base/time/time.h b/base/time/time.h
index 0a2677838617e79ada4b219cc684631cf799a519..5f4bc541b12081b03cde00fdf52d9b9e297528a4 100644
--- a/base/time/time.h
+++ b/base/time/time.h
@@ -4,25 +4,52 @@
 
 // Time represents an absolute point in coordinated universal time (UTC),
 // internally represented as microseconds (s/1,000,000) since the Windows epoch
-// (1601-01-01 00:00:00 UTC) (See http://crbug.com/14734).  System-dependent
-// clock interface routines are defined in time_PLATFORM.cc.
+// (1601-01-01 00:00:00 UTC) (See http://crbug.com/14734). System-dependent

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

that bug has been fixed

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

Done.

+// clock interface routines are defined in time_PLATFORM.cc. Note that values
+// for Time may skew and jump around as the operating system makes adjustments
+// to synchronize (e.g., with NTP servers). Thus, client code that uses the Time
+// class must account for this.
 //
 // TimeDelta represents a duration of time, internally represented in
 // microseconds.
 //
-// TimeTicks represents an abstract time that is most of the time incrementing
-// for use in measuring time durations. It is internally represented in
-// microseconds.  It can not be converted to a human-readable time, but is
-// guaranteed not to decrease (if the user changes the computer clock,
-// Time::Now() may actually decrease or jump).  But note that TimeTicks may
-// "stand still", for example if the computer suspended.
+// TimeTicks, ThreadTicks, and TraceTicks represent an abstract time that is
+// most of the time incrementing, for use in measuring time
+// durations. Internally, they are represented in microseconds. They can not be
+// converted to a human-readable time, but are guaranteed not to decrease
+// (unlike the Time class). Note that TimeTicks may "stand still" (e.g., if the
+// computer is suspended), and ThreadTicks will "stand still" whenever the
+// thread has been de-scheduled by the operating system.
 //
-// These classes are represented as only a 64-bit value, so they can be
-// efficiently passed by value.
+// All time classes are copyable, assignable, and occupy 64-bits per
+// instance. Thus, they can be efficiently passed by-value (as opposed to

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

all signed 64 ints?

+// by-reference).
 //
 // Definitions of operator<< are provided to make these types work with
 // DCHECK_EQ() and other log macros. For human-readable formatting, see
 // "base/i18n/time_formatting.h".
+//
+// So many choices!  Which time class should you use?  Examples:

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

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

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

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.

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

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.

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

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.  ;-)

+//
+//   Time:        Interpreting the wall-clock time provided by a remote
+//                system. Detecting whether cached resources have
+//                expired. Providing the user with a display of the current date
+//                and time. Determining the amount of time between events across
+//                re-boots of the machine.
+//
+//   TimeTicks:   Tracking the amount of time a task runs. Executing delayed
+//                tasks at the right time. Computing presentation timestamps.
+//                Synchronizing audio and video using TimeTicks as a common
+//                reference clock (lip-sync). Measuring network round-trip
+//                latency.
+//
+//   ThreadTicks: Benchmarking how long the current thread has been doing actual
+//                work.
+//
+//   TraceTicks:  Measuring the real-world passage of time, but also allowing
+//                events to be correlated with those from other applications and
+//                services on the same system. Generally, this is only used
+//                directly by the event tracing infrastructure.

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

Why expose TraceTicks here then?  lets make it part of tracing.

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

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?

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

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

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

Acknowledged.

 
 #ifndef BASE_TIME_TIME_H_
 #define BASE_TIME_TIME_H_
@@ -639,14 +666,6 @@ BASE_EXPORT std::ostream& operator<<(std::ostream& os, Time time);
 // Represents monotonically non-decreasing clock time.
 class BASE_EXPORT TimeTicks : public time_internal::TimeBase<TimeTicks> {
  public:
-  // We define this even without OS_CHROMEOS for seccomp sandbox testing.
-#if defined(OS_LINUX)
-  // Force definition of the system trace clock; it is a chromeos-only api
-  // at the moment and surfacing it in the right place requires mucking
-  // with glibc et al.
-  static const clockid_t kClockSystemTrace = 11;
-#endif
-
   TimeTicks() : TimeBase(0) {
   }
 
@@ -662,43 +681,6 @@ class BASE_EXPORT TimeTicks : public time_internal::TimeBase<TimeTicks> {
   // clock will be used instead.
   static bool IsHighResolution();
 
-  // Returns true if ThreadNow() is supported on this system.
-  static bool IsThreadNowSupported() {
-#if (defined(_POSIX_THREAD_CPUTIME) && (_POSIX_THREAD_CPUTIME >= 0)) || \
-    (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_ANDROID)
-    return true;
-#else
-    return false;
-#endif
-  }
-
-  // Returns thread-specific CPU-time on systems that support this feature.
-  // Needs to be guarded with a call to IsThreadNowSupported(). Use this timer
-  // to (approximately) measure how much time the calling thread spent doing
-  // actual work vs. being de-scheduled. May return bogus results if the thread
-  // migrates to another CPU between two calls.
-  //
-  // WARNING: The returned value might NOT have the same origin as Now(). Do not
-  // perform math between TimeTicks values returned by Now() and ThreadNow() and
-  // expect meaningful results.
-  // TODO(miu): Since the timeline of these values is different, the values
-  // should be of a different type.
-  static TimeTicks ThreadNow();
-
-  // Returns the current system trace time or, if not available on this
-  // platform, a high-resolution time value; or a low-resolution time value if
-  // neither are avalable. On systems where a global trace clock is defined,
-  // timestamping TraceEvents's with this value guarantees synchronization
-  // between events collected inside chrome and events collected outside
-  // (e.g. kernel, X server).
-  //
-  // WARNING: The returned value might NOT have the same origin as Now(). Do not
-  // perform math between TimeTicks values returned by Now() and
-  // NowFromSystemTraceTime() and expect meaningful results.
-  // TODO(miu): Since the timeline of these values is different, the values
-  // should be of a different type.
-  static TimeTicks NowFromSystemTraceTime();
-
 #if defined(OS_WIN)
   // Translates an absolute QPC timestamp into a TimeTicks value. The returned
   // value has the same origin as Now(). Do NOT attempt to use this if
@@ -738,6 +720,80 @@ class BASE_EXPORT TimeTicks : public time_internal::TimeBase<TimeTicks> {
 // For logging use only.
 BASE_EXPORT std::ostream& operator<<(std::ostream& os, TimeTicks time_ticks);
 
+// ThreadTicks ----------------------------------------------------------------
+
+// Represents a clock, specific to a particular thread, than runs only while the
+// thread is running.
+class BASE_EXPORT ThreadTicks : public time_internal::TimeBase<ThreadTicks> {
+ public:
+  ThreadTicks() : TimeBase(0) {
+  }
+
+  // Returns true if ThreadTicks::Now() is supported on this system.
+  static bool IsSupported() {
+#if (defined(_POSIX_THREAD_CPUTIME) && (_POSIX_THREAD_CPUTIME >= 0)) || \
+    (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_ANDROID)
+    return true;
+#else
+    return false;
+#endif
+  }
+
+  // Returns thread-specific CPU-time on systems that support this feature.
+  // Needs to be guarded with a call to IsSupported(). Use this timer
+  // to (approximately) measure how much time the calling thread spent doing
+  // actual work vs. being de-scheduled. May return bogus results if the thread
+  // migrates to another CPU between two calls.
+  static ThreadTicks Now();
+
+ private:
+  friend class time_internal::TimeBase<ThreadTicks>;
+
+  // Please use Now() to create a new object. This is for internal use
+  // and testing.
+  explicit ThreadTicks(int64 us) : TimeBase(us) {
+  }
+};
+
+// For logging use only.
+BASE_EXPORT std::ostream& operator<<(std::ostream& os, ThreadTicks time_ticks);
+
+// TraceTicks ----------------------------------------------------------------
+
+// Represents high-resolution system trace clock time.
+class BASE_EXPORT TraceTicks : public time_internal::TimeBase<TraceTicks> {
+ public:
+  // We define this even without OS_CHROMEOS for seccomp sandbox testing.
+#if defined(OS_LINUX)
+  // Force definition of the system trace clock; it is a chromeos-only api
+  // at the moment and surfacing it in the right place requires mucking
+  // with glibc et al.
+  static const clockid_t kClockSystemTrace = 11;
+#endif
+
+  TraceTicks() : TimeBase(0) {
+  }
+
+  // Returns the current system trace time or, if not available on this
+  // platform, a high-resolution time value; or a low-resolution time value if
+  // neither are avalable. On systems where a global trace clock is defined,

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

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]

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.

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

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>

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

Ah.  Okay.  Done!  :)

+  // timestamping TraceEvents's with this value guarantees synchronization
+  // between events collected inside chrome and events collected outside
+  // (e.g. kernel, X server).
+  static TraceTicks Now();
+
+ private:
+  friend class time_internal::TimeBase<TraceTicks>;
+
+  // Please use Now() to create a new object. This is for internal use
+  // and testing.
+  explicit TraceTicks(int64 us) : TimeBase(us) {
+  }
+};
+
+// For logging use only.
+BASE_EXPORT std::ostream& operator<<(std::ostream& os, TraceTicks time_ticks);
+
 }  // namespace base
 
 #endif  // BASE_TIME_TIME_H_