Introducing phased profiling framework

components/metrics/profiler/tracking_synchronizer.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_METRICS_PROFILER_TRACKING_SYNCHRONIZER_H_
 #define COMPONENTS_METRICS_PROFILER_TRACKING_SYNCHRONIZER_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #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
 // various processes, into the browser process. Such transactions are usually
 // instigated by the browser. In general, a process will respond by gathering
 // profiler data, and transmitting the pickled profiler data. We collect the
 // data in asynchronous mode that doesn't block the UI thread.
 //
 // To assure that all the processes have responded, a counter is maintained
 // to indicate the number of pending (not yet responsive) processes. We tag
@@ skipping 76 matching lines @@
 
   // We don't track the actual processes that are contacted for an update, only
   // the count of the number of processes, and we can sometimes time-out and
   // give up on a "slow to respond" process.  We use a sequence_number to be
   // sure a response from a process is associated with the current round of
   // requests. All sequence numbers used are non-negative.
   // last_used_sequence_number_ is the most recently used number (used to avoid
   // 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);
 };
 
 }  // namespace metrics
 
 #endif  // COMPONENTS_METRICS_PROFILER_TRACKING_SYNCHRONIZER_H_