Delivering the FIRST_NONEMPTY_PAINT phase changing event to base/

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 "base/process/process_handle.h"
@@ skipping 11 matching lines @@
 namespace metrics {
 
 // Set of profiling events, in no guaranteed order. Implemented as a vector
 // because we don't need to have an efficient .find() on it, so vector<> is more
 // efficient.
 typedef std::vector<ProfilerEventProto::ProfilerEvent> ProfilerEvents;
 
 // Observer for notifications from the TrackingSynchronizer class.
 class TrackingSynchronizerObserver {
  public:
-  // TODO(vadimt): Consider isherman@ idea: I'd change the phase_start and
-  // phase_finish from TimeDeltas to TimeTicks.  And I'd omit the |past_events|
-  // list -- either in favor of a single ProfilerEvent that corresponds to the
-  // phase, or a method on the TrackingSynchronizer that can translate a
-  // profiling_phase to a ProfilerEvent.
-
   // 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 |phase_start| and
   // |phase_finish|. All profiling phases prior to the reported one have already
   // completed, and each completion was associated with an instance of
   // ProfilerEventProto::ProfilerEvent. |past_events| contains events associated
   // with completions of phases prior to the reported one.
   // The observer should assume there might be more data coming until
   // FinishedReceivingData() is called.
   virtual void ReceivedProfilerData(
       const tracked_objects::ProcessDataPhaseSnapshot& process_data_phase,
       base::ProcessId process_id,
       content::ProcessType process_type,
       int profiling_phase,
       base::TimeDelta phase_start,
       base::TimeDelta phase_finish,
       const ProfilerEvents& past_events) = 0;

[Ilya Sherman, 2015/04/07 01:15:30]

I'd still prefer to pass TimeTicks in place of TimeDeltas, and I'd still prefer
to omit the past_events list.

[vadimt, 2015/04/07 21:44:14]

IMHO, time deltas make sense. phase_start and _end are relative times (i.e.
deltas between the phase event and the start of recording), hence, we use
TimeDelta. This makes the code easier to comprehend.

Speaking of past_events: as you can see, the whole accounting for phases and
events is done by one central component: tracking synchronizer. It has 2
possible consumers of the information: metrics component and the profiler tab.
It makes the phase-related information equally available to both consumers. If
metrics component didn’t exist (say, on a new hypothetical platform), the
profiler tab would still have access to phasing information from the
synchronizer. It would be strange to move the accounting to one of the
consumers, i.e. metrics component, creating strange asymmetry between consumers.
Hence, the accounting is done by the central component - tracking synchroniser,
metrics component is NOT notified about phase change events (only about end
results of multiphased snapshots), and tracking synchronizer reports the whole
set of information, including past events, to consumers.

[Ilya Sherman, 2015/04/07 22:58:35]

My claim is that relative times only make sense for the metrics component, and
not for the profiler UI.  I'd prefer to use absolute timestamps and let clients
process those timestamps as they wish.
I agree that the tracking synchronizer is currently the code that is responsible
for keeping track of the basics of phases.  However, I'm also seeing the needs
of the metrics uploader, which are distinct from the needs of the profiler tab,
leak into the internals of the tracking synchronizer.  I'd prefer that the
tracking synchronizer be responsible for the minimum set of things that are
actually shared between the two clients, and that everything that's only needed
for the uploader actually be handled by the uploader.

[vadimt, 2015/04/08 01:09:50]

Done with the TimeTicks part.

On past_events: I believe we should make it possible (in future) for the
chrome://profiler to be able to filter by before/after a given phase. The
current data passed to it doesn’t seem unreasonable, since it supports exactly
this kind of query. This doesn’t seem like an obvious leaking of the UMA-related
details. The current protocol may in fact help to avoid duplication between the
tab and UMA code.
Of course, there are some uncertainties about the future, and it may turn out
that the current protocol will become inadequate. But we don’t know, and I’m not
sure that we have to change the current implementation, perhaps to better,
perhaps to worse (from the hypothetical future needs’ point of view), right now.
So, the claim is twofold: (1) the current protocol doesn’t look leaky to me; (2)
we don’t know the future.
This is my motivation for having exactly this protocol.
YMMV.

 
   // 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_