Introducing phased profiling framework

components/metrics/profiler/tracking_synchronizer.h

Index: components/metrics/profiler/tracking_synchronizer.h
diff --git a/components/metrics/profiler/tracking_synchronizer.h b/components/metrics/profiler/tracking_synchronizer.h
index fefe8d9457eb7c9e91798261f890cf3b7c9590d6..cb5d626bcb100489aea48a7d91a7c7be3c871640 100644
--- a/components/metrics/profiler/tracking_synchronizer.h
+++ b/components/metrics/profiler/tracking_synchronizer.h
@@ -13,6 +13,8 @@
 #include "base/lazy_instance.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
 #include "content/public/browser/profiler_subscriber.h"
 
 // This class maintains state that is used to upload profiler data from the
@@ -109,6 +111,19 @@ class TrackingSynchronizer
   // reuse for a long time).
   int last_used_sequence_number_;
 
+  // Sequence of events associated with already completed profiling phases. The
+  // index in the vector is the phase number. The current phase is not included.
+  std::vector<ProfilerEventProto::ProfilerEvent>

[Alexei Svitkine (slow), 2015/03/09 17:13:35]

Don't you have a typedef for this in another file? Or is the distinction that
this one is ordered but the other one isn't?

[vadimt, 2015/03/13 23:18:01]

Yes. They are coincidentally same. I should be free to convert the other one
into a set, but this one will remain a vector.

+      phase_completion_events_sequence_;
+
+  // Time of the profiling start. Used to calculate times of phase change
+  // moments relative to this value.
+  const base::TimeTicks start_time_;

[Ilya Sherman, 2015/03/10 00:48:10]

I think these two fields are only needed by the metrics service.  I'd prefer to
see them defined there.  That is, the TrackingSynchronizer should just pass
through data that it receives from //base, to any clients who register as
interested in the data.

This especially applies to the vector, which makes absolutely no sense outside
of the metrics service.  The TrackingSynchronizer should just pass the *current*
phase.  The metrics service can compute whatever clever cache it wants to from
there.

[vadimt, 2015/03/13 23:18:01]

Can we get back to this once you see the not-yet included methods? This CL is
incomplete in the sense that doesn't contain any code dealing with phase change
events.

[Ilya Sherman, 2015/03/14 00:02:31]

Sure, we can come back to this.  Please mark it somehow in the latter CL, so
that we remember to.

[vadimt, 2015/03/14 01:39:15]

Added a TODO note that I'll remove in the next CL, hence causing a diff.

+
+  // Times of starts of all profiling phases, including the current phase. The
+  // index in the vector is the phase number.
+  std::vector<base::TimeTicks> phase_start_times_;

[Ilya Sherman, 2015/03/10 00:48:10]

I'd prefer to see the time deltas computed in //base, rather than here.  I don't
think that it's accurate to assume that all processes have the same phase
durations.

[vadimt, 2015/03/13 23:18:01]

We'll send one proto per phase. If each process provides its own version of the
time, it's not clear how to merge them.

[Ilya Sherman, 2015/03/14 00:02:31]

That suggests to me a flaw in our proto design, then.  The times are simply not
guaranteed to be aligned, so it's deceptive to send just a single pair of
timestamps for all the different processes.

[vadimt, 2015/03/14 01:39:15]

The alternative would be to send a proto per every phase in every process.
But it's semantically incorrect, since then profiling time, as seen from the
server, would multiply by the number of the processes.
What we are really interested at the server is the wallclock time gathered in
some central place, which is what we are doing. This reflects that fact that we
have a single user that experiences jank created by all processes running in
parallel, and the correct way to express jank frequency is in terms of user's
wallclock time.

+
   DISALLOW_COPY_AND_ASSIGN(TrackingSynchronizer);
 };