Introducing phased profiling framework

components/metrics/profiler/tracking_synchronizer_observer.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_OBSERVER_H_
 #define COMPONENTS_METRICS_PROFILER_TRACKING_SYNCHRONIZER_OBSERVER_H_
 
+#include <vector>
+
+#include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
+
+namespace base {
+class TimeDelta;
+}
+
 namespace tracked_objects {
-struct ProcessDataSnapshot;
+struct ProcessDataPhaseSnapshot;
 }
 
 namespace metrics {
 
+// Set of profiling events, in no guaranteed order.

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

It's confusing to have something named a set when not backed by a set<>. I guess
it's probably because we don't need to have an efficient .find() on it so
vector<> is more efficient? If so, I'd mention that in the comment.

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

Or, just call this a "list".

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

Why isn't the order guaranteed?

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

The server doesn't use order. Hence, the client has no obligations to guarantee
the order. I'm adding this to comments so that no one later will feel obliges to
guarantee any order.

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

Done.

+typedef std::vector<ProfilerEventProto::ProfilerEvent> ProfilerEventsSet;

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

Definitely please don't name the type "set" if it's backed by a vector.  I'd
prefer not to have a typedef at all... but if you must, the common convention is
to call this type "ProfilerEvents"

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

Done.

+
 // Observer for notifications from the TrackingSynchronizer class.
 class TrackingSynchronizerObserver {
  public:
-  // Received |profiler_data| from a single process of |process_type|.
+  // Received |process_data_phase| for profiling phase |profiling_phase| from a
+  // single process of |process_type|. The phase start and finish times,
+  // relative to the start time are |profiling_phase_start| and
+  // |profiling_phase_finish|. |past_profiler_events| is the set of events that
+  // caused finishing prior profiling phases.

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

nit: This final sentence is not very clear.  Please try to reword it so that
someone unfamiliar with this code could more easily understand what it means.

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

Done.

   // The observer should assume there might be more data coming until
   // |FinishedReceivingData()| is called.
   virtual void ReceivedProfilerData(
-      const tracked_objects::ProcessDataSnapshot& profiler_data,
-      int process_type) = 0;
+      const tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,

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

This is now a lot of parameters. How about defining a type to hold them and
passing that as a param?

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

In particular, why aren't these all members of ProcessDataPhaseSnapshot?

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

Because they are calculated in TrackingSynchronizer, and
ProcessDataPhaseSnapshot is sent from base/. Also see my comment on why we don;t
send times from base/.

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

I don't feel very good about creating a copy of ProcessDataPhaseSnapshot just of
this... You think that this is worth the inefficiency?

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

(a) I'd prefer to track all of this in //base, so that there's no need to create
copies.
(b) If we decide not to track all this in //base, I think the cost of a copy is
not very high.  We already create lots of copies of the data for serialization
across IPC and when bouncing the data across threads.  One more copy is not too
expensive.

But, if you are really concerned about the copy, *and* we don't track the data
in //base, we could still create a wrapper type that represents these
parameters, and references the ProcessDataPhaseSnapshot object as a
const-reference, to preserve efficiency.  I'd definitely want to see some
concrete data indicating that that level of optimization is useful before going
down that route, but it's an option.

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

Can do, but first please confirm that you are OK with NOT sending times in
base/, per another comment.

I don;t recall if you suggested passing past_profiler_events from base as well,
but the objection would be that it's wrong to make base/ dependent on event
types defined outside of it...

+      int process_id,

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

Is there a reason this is an int and not a base::ProcessId?

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

Done.

+      int process_type,

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

Again, shouldn't this be some enum type?

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

Done, but I'd rather revert it since Nacl passes values outside of the range of
ProcessType enum, so I have to cast these values to ProcessType.

WDYT?

+      int profiling_phase,

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

Likewise - if this file is depending on the protobuf, why not use the enum from
there?

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

No, this is just a number of the profiling phase. It's a consequential number.
For the protobuf-dependent value, see |past_profiler_events|.

+      const base::TimeDelta& profiling_phase_start,
+      const base::TimeDelta& profiling_phase_finish,
+      const ProfilerEventsSet& past_profiler_events) = 0;
 
   // The observer should not expect any more calls to |ReceivedProfilerData()|
   // (without re-registering).  This is sent either when data from all processes
   // has been gathered, or when the request times out.
   virtual void FinishedReceivingProfilerData() {}
 
  protected:
   TrackingSynchronizerObserver() {}
   virtual ~TrackingSynchronizerObserver() {}
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TrackingSynchronizerObserver);
 };
 
 }  // namespace metrics
 
 #endif  // COMPONENTS_METRICS_PROFILER_TRACKING_SYNCHRONIZER_OBSERVER_H_