Delivering the FIRST_NONEMPTY_PAINT phase changing event to base/

base/tracked_objects.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 BASE_TRACKED_OBJECTS_H_
 #define BASE_TRACKED_OBJECTS_H_
 
 #include <map>
 #include <set>
 #include <stack>
@@ skipping 240 matching lines @@
   int birth_count_;
 
   DISALLOW_COPY_AND_ASSIGN(Births);
 };
 
 //------------------------------------------------------------------------------
 // Basic info summarizing multiple destructions of a tracked object with a
 // single birthplace (fixed Location).  Used both on specific threads, and also
 // in snapshots when integrating assembled data.
 
+struct DeathDataPhaseSnapshot;
+
 class BASE_EXPORT DeathData {
+  // Access to members of this class is never protected by the lock. The fields

[Alexei Svitkine (slow), 2015/04/08 15:55:39]

Nit: This comment should be above the class itself rather than above the public:
statement.

+  // are accessed in such a way that corruptions resulting from race conditions
+  // are not significant, and don't accumulate as a result of multiple accesses.
+  // All fields except sample_probability_count_ can be snapshotted, and
+  // snapshotting happens only from the main thread.
  public:
   // Default initializer.
   DeathData();
 
   // When deaths have not yet taken place, and we gather data from all the
   // threads, we create DeathData stats that tally the number of births without
   // a corresponding death.
   explicit DeathData(int count);
 
+  ~DeathData();
+
   // Update stats for a task destruction (death) that had a Run() time of
   // |duration|, and has had a queueing delay of |queue_duration|.
   void RecordDeath(const int32 queue_duration,
                    const int32 run_duration,
                    const uint32 random_number);
 
-  // Metrics accessors, used only for serialization and in tests.
+  // Metrics and past snapshots accessors, used only for serialization and in
+  // tests.
   int count() const;
   int32 run_duration_sum() const;
   int32 run_duration_max() const;
   int32 run_duration_sample() const;
   int32 queue_duration_sum() const;
   int32 queue_duration_max() const;
   int32 queue_duration_sample() const;
+  DeathDataPhaseSnapshot* last_phase_snapshot() const;
 
-  // Reset all tallies to zero. This is used as a hack on realtime data.
-  void Clear();
+  // Called when the current profiling phase, identified by |profiling_phase|,
+  // ends.
+  // Called only in the main thread.

[Alexei Svitkine (slow), 2015/04/08 15:55:39]

Hmm, base/ generally doesn't know about "main thread" - since *specific* threads
are a higher-level concept. Also, the tense of this sentence is describing
behavior outside of this class - which doesn't make sense.

The existing comments in this file discuss "death thread" and "birth thread" -
which make sense - since they're relative. I'm guessing those can both be
distinct from the "main thread". Is this object always created on the main
thread? If so, you can say "Should only be called from the same thread that the
object was created on.". If that's not correct, let me know and we can discuss
more.

[vadimt, 2015/04/08 20:32:34]

I added some comments.
The object gets created in the death thread for adding to the death map, and in
the snapshot thread, as a part for creating snapshots for Still_Alive thread.

+  void OnProfilingPhaseCompleted(int profiling_phase);
 
  private:
   // Members are ordered from most regularly read and updated, to least
   // frequently used.  This might help a bit with cache lines.
   // Number of runs seen (divisor for calculating averages).
+  // Can be incremented only from the death thread.
   int count_;
-  // Basic tallies, used to compute averages.
+
+  // Count used in determining probability of selecting exec/queue times from a
+  // recorded death as samples.
+  // Gets incremented only in the death thread, but can be set to 0 by
+  // OnProfilingPhaseCompleted() from the main thread.
+  int sample_probability_count_;
+
+  // Basic tallies, used to compute averages. Can be incremented only in the
+  // death thread.
   int32 run_duration_sum_;
   int32 queue_duration_sum_;
   // Max values, used by local visualization routines.  These are often read,
-  // but rarely updated.
+  // but rarely updated. The max values get assigned only in the death thread,
+  // but these fields can be set to 0 by OnProfilingPhaseCompleted() from the
+  // main thread.
   int32 run_duration_max_;
   int32 queue_duration_max_;
   // Samples, used by crowd sourcing gatherers.  These are almost never read,
-  // and rarely updated.
+  // and rarely updated. They can be modified only from the death thread.
   int32 run_duration_sample_;
   int32 queue_duration_sample_;
+
+  // Snapshot of this death data made at the last profiling phase completion, if
+  // any. DeathData owns the whole list starting with this pointer.
+  // Can be accessed only from the main thread.
+  DeathDataPhaseSnapshot* last_phase_snapshot_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeathData);
 };
 
 //------------------------------------------------------------------------------
 // A "snapshotted" representation of the DeathData class.
 
 struct BASE_EXPORT DeathDataSnapshot {
   DeathDataSnapshot();
   explicit DeathDataSnapshot(const DeathData& death_data);
   ~DeathDataSnapshot();
 
+  // Calculates delta between this snapshot and an earlier snapshot of the same
+  // task |older| and assigns it back to this object.
+  void CalculateDelta(const DeathDataSnapshot& older);
+
   int count;
   int32 run_duration_sum;
   int32 run_duration_max;
   int32 run_duration_sample;
   int32 queue_duration_sum;
   int32 queue_duration_max;
   int32 queue_duration_sample;
 };
 
 //------------------------------------------------------------------------------
+// A "snapshotted" representation of the DeathData for a particular profiling
+// phase.
+
+struct DeathDataPhaseSnapshot {
+  DeathDataPhaseSnapshot(int profiling_phase, const DeathData& death_data);
+
+  // Profiling phase at which completion this snapshot was taken.
+  int profiling_phase;
+  // Death data snapshot.
+  DeathDataSnapshot death_data;
+  // Pointer to a snapshot from the previous phase. This object doesn't own the
+  // pointed object.

[Alexei Svitkine (slow), 2015/04/08 15:55:39]

But who does?

It seems in some cases, the object does - i.e. through the linked list chain on
DeathData.

This seems quite messy. How about having DeathData just keep a ScopedVector of
DeathDataPhaseSnapshot snapshots so that the ownership is clear - and no manual
iterating through the linked lists to delete these is needed?

[vadimt, 2015/04/08 20:32:34]

Having list nodes that don't own 'next' is a very common practice (the list
itself is owned by some parent structure); not sure we can call this "messy".

I’m not a big fan of scoped_vector. Vector has higher memory overhead than a
list, and we shouldn't ignore this, given that we have thousands of DeathDatas.
(Also, I like an elegant loop in ThreadData::SnapshotExecutedTasks, which starts
at the current snapshot (with is not in the snapshotted list, and would not be
in the vector), and goes on to the list of old snapshotted elements. This loop
would look heavier if we used a vector, since we'd have to special case the
current snapshot.)

I changed comments; hoping that the perception of “mess” is less now.

[Alexei Svitkine (slow), 2015/04/08 20:54:26]

Sorry, let me clarify - I don't mind that the "prev" pointer doesn't own the
data. My concern is that it's not consistent - because in some cases this
structure is indeed used for ownership.

But I agree that we should be careful about performance. Can you clarify (not in
the code, just in the review) when exactly DeathData is destroyed? If the events
are relatively rare (i.e. snapshotting and destruction), it seems better to opt
for code clarity than raw performance. On the other hand, if it's frequent, of
course performance is a bigger concern.

My impression was that snapshotting and destruction of DeathData would be rare.
Is that assumption correct or am I wrong?

[vadimt, 2015/04/08 21:27:17]

I still fail to see the difference between the patterns I use and the general
list pattern, where there is a root object (List), pointing to the first element
(Node), which points ... etc. The whole list is owned by the root object.
Here, List == DeathData, and Node == DeathDataPhaseSnapshot, without any
deviations, hacks and tricks.

So, your "concern is that it's not consistent" is equally applicable to the
general List pattern. However, our misunderstanding is perhaps because you think
that I deviate from the classic List pattern? You phrase "because in some cases
this structure is indeed used for ownership" seems to suggest this. Could you
point to where this happens?

DeatData is destroyed at the process' exit, and phase change/snapshottings are
rare.

However, my main concern regarding the vector is it's memory, not CPU, overhead.
You know, 2 values (count, capacity), + allocated memory (of 'capacity' size).
It's more expensive than just the list, both for 0 and non-0 list sizes.

[Alexei Svitkine (slow), 2015/04/08 21:57:49]

My original comment was because you had a comment mentioning that this object
doesn't own the prev pointer. Yet now you say that it does own it (and indeed
this seems to match the dtor of DeathData's use of this - where it's used to
delete the chain). I don't have a problem with the pattern per-se, but I do want
the ownership model in the code to be very clear.

If the snapshot indeed owns its prev chain, then it should be done using
constructs that indicate this - i.e. using a scoped_ptr. If it doesn't own it,
then a different construct (e.g. scoped vector) should be used for managing
ownership (in parallel to the list). From looking at the code, it seems to me
(correct me if I'm wrong) that in *some* cases it owns it (i.e. for the list in
ThreadData) - but in other cases it doesn't (e.g. when the struct is copied
around?) - that's the issue I have - the inconsistency.

Now, perhaps you are right that the memory overhead of a scoped_vector is too
much - and thinking about this, I guess I agree that it probably is, since this
is once per task type and there are a lot of types of tasks..

Ideally, I'd like to have it either always own it, or never own it - but if
there really is no good way to do this and stay performant - then we should at
least add better comments above where the root of this owning list is declared
about it.

[vadimt, 2015/04/08 23:31:24]

Hmmm;
DeathDataPhaseSnapshot is the node in the list, and, according to the list
pattern, doesn't own its 'prev'. Always.

DeathData is the root of the list, and (according to the same pattern) owns the
whole list of DeathDataPhaseSnapshot's. Always.

So, yes, I'm correcting you. Everything is consistent.

Now, regarding comments:
The comment before DeathDataPhaseSnapshot says "Used as an element of the list
of phase snapshots owned by DeathData." This indicates not-ownership of prev.

The comment before DeathData::last_phase_snapshot_ says "DeathData owns the
whole list starting with this pointer."

Did you somehow mix DeathData and DeathDataSnapshot while reading code? I'm
trying to understand why we are seeing different things...

[Alexei Svitkine (slow), 2015/04/09 15:39:05]

I admit I missed the updated comments you added - probably because I was still
looking at the old snapshot (which is where this comment is on) and not at the
new one.

The list root owning the whole list makes sense - though I guess I wasn't
thinking of the DeathData as the root of the list (I was thinking of
last_phase_snapshot_). I guess DeathData is just too decomposed for my
preference - doing multiple things at the same time at a low level - i.e. if its
runtime data was one struct and there was another struct/class member that was
the root of the list (that encapsulated the ownership), I would find it cleaner.

But I agree that it has a cost of complexity - as there's already a lot of
classes in this module and adding more just makes it harder to understand (I
already have a hard time reasoning about the existing ones because there are so
many. :\). So I won't insist on more classes here, even though I think it would
make things a bit cleaner in this instance - at the cost of increasing
complexity overall as I said. :\

So I guess I'm OK with this now that I've read your updated comments.

+  DeathDataPhaseSnapshot* prev;
+};
+
+//------------------------------------------------------------------------------
 // A temporary collection of data that can be sorted and summarized.  It is
 // gathered (carefully) from many threads.  Instances are held in arrays and
 // processed, filtered, and rendered.
 // The source of this data was collected on many threads, and is asynchronously
 // changing.  The data in this instance is not asynchronously changing.
 
 struct BASE_EXPORT TaskSnapshot {
   TaskSnapshot();
-  TaskSnapshot(const BirthOnThread& birth,
-               const DeathData& death_data,
+  TaskSnapshot(const BirthOnThreadSnapshot& birth,
+               const DeathDataSnapshot& death_data,
                const std::string& death_thread_name);
   ~TaskSnapshot();
 
   BirthOnThreadSnapshot birth;
+  // Delta between death data for a thread for a certain profiling phase and the
+  // snapshot for the pervious phase, if any. Otherwise, just a snapshot.
   DeathDataSnapshot death_data;
   std::string death_thread_name;
 };
 
 //------------------------------------------------------------------------------
 // For each thread, we have a ThreadData that stores all tracking info generated
 // on this thread.  This prevents the need for locking as data accumulates.
 // We use ThreadLocalStorage to quickly identfy the current ThreadData context.
 // We also have a linked list of ThreadData instances, and that list is used to
 // harvest data from all existing instances.
@@ skipping 32 matching lines @@
   // only used by the message loop, which has a well defined thread name.
   static void InitializeThreadContext(const std::string& suggested_name);
 
   // Using Thread Local Store, find the current instance for collecting data.
   // If an instance does not exist, construct one (and remember it for use on
   // this thread.
   // This may return NULL if the system is disabled for any reason.
   static ThreadData* Get();
 
   // Fills |process_data_snapshot| with phased snapshots of all profiling
-  // phases, including the current one.
-  static void Snapshot(ProcessDataSnapshot* process_data_snapshot);
+  // phases, including the current one, identified by |current_profiling_phase|.
+  // |current_profiling_phase| is necessary because a child process can start
+  // after several phase-changing events, so it needs to receive the current
+  // phase number from the browser process to fill the correct entry for the
+  // current phase in the |process_data_snapshot| map.
+  static void Snapshot(int current_profiling_phase,
+                       ProcessDataSnapshot* process_data_snapshot);
+
+  // Called when the current profiling phase, identified by |profiling_phase|,
+  // ends.
+  // |profiling_phase| is necessary because a child process can start after
+  // several phase-changing events, so it needs to receive the phase number from
+  // the browser process to fill the correct entry in the
+  // completed_phases_snapshots_ map.
+  static void OnProfilingPhaseCompleted(int profiling_phase);
 
   // Finds (or creates) a place to count births from the given location in this
   // thread, and increment that tally.
   // TallyABirthIfActive will returns NULL if the birth cannot be tallied.
   static Births* TallyABirthIfActive(const Location& location);
 
   // Records the end of a timed run of an object.  The |completed_task| contains
   // a pointer to a Births, the time_posted, and a delayed_start_time if any.
   // The |start_of_run| indicates when we started to perform the run of the
   // task.  The delayed_start_time is non-null for tasks that were posted as
   // delayed tasks, and it indicates when the task should have run (i.e., when
   // it should have posted out of the timer queue, and into the work queue.
   // The |end_of_run| was just obtained by a call to Now() (just after the task
   // finished). It is provided as an argument to help with testing.
   static void TallyRunOnNamedThreadIfTracking(
       const base::TrackingInfo& completed_task,
       const TaskStopwatch& stopwatch);
 
   // Record the end of a timed run of an object.  The |birth| is the record for
   // the instance, the |time_posted| records that instant, which is presumed to
   // be when the task was posted into a queue to run on a worker thread.
   // The |start_of_run| is when the worker thread started to perform the run of
   // the task.
   // The |end_of_run| was just obtained by a call to Now() (just after the task
   // finished).
-  static void TallyRunOnWorkerThreadIfTracking(const Births* birth,
+  static void TallyRunOnWorkerThreadIfTracking(const Births* births,
                                                const TrackedTime& time_posted,
                                                const TaskStopwatch& stopwatch);
 
   // Record the end of execution in region, generally corresponding to a scope
   // being exited.
-  static void TallyRunInAScopedRegionIfTracking(const Births* birth,
+  static void TallyRunInAScopedRegionIfTracking(const Births* births,
                                                 const TaskStopwatch& stopwatch);
 
   const std::string& thread_name() const { return thread_name_; }
 
   // Initializes all statics if needed (this initialization call should be made
   // while we are single threaded). Returns false if unable to initialize.
   static bool Initialize();
 
   // Sets internal status_.
   // If |status| is false, then status_ is set to DEACTIVATED.
@@ skipping 51 matching lines @@
   // TODO(jar): Make this a friend in DEBUG only, so that the optimizer has a
   // better change of optimizing (inlining? etc.) private methods (knowing that
   // there will be no need for an external entry point).
   friend class TrackedObjectsTest;
   FRIEND_TEST_ALL_PREFIXES(TrackedObjectsTest, MinimalStartupShutdown);
   FRIEND_TEST_ALL_PREFIXES(TrackedObjectsTest, TinyStartupShutdown);
   FRIEND_TEST_ALL_PREFIXES(TrackedObjectsTest, ParentChildTest);
 
   typedef std::map<const BirthOnThread*, int> BirthCountMap;
 
+  typedef std::vector<std::pair<const Births*, DeathDataPhaseSnapshot>>
+      DeathsSnapshot;
+
   // Worker thread construction creates a name since there is none.
   explicit ThreadData(int thread_number);
 
   // Message loop based construction should provide a name.
   explicit ThreadData(const std::string& suggested_name);
 
   ~ThreadData();
 
   // Push this instance to the head of all_thread_data_list_head_, linking it to
   // the previous head.  This is performed after each construction, and leaves
   // the instance permanently on that list.
   void PushToHeadOfList();
 
   // (Thread safe) Get start of list of all ThreadData instances using the lock.
   static ThreadData* first();
 
   // Iterate through the null terminated list of ThreadData instances.
   ThreadData* next() const;
 
 
   // In this thread's data, record a new birth.
   Births* TallyABirth(const Location& location);
 
   // Find a place to record a death on this thread.
-  void TallyADeath(const Births& birth,
+  void TallyADeath(const Births& births,
                    int32 queue_duration,
                    const TaskStopwatch& stopwatch);
 
-  // Snapshot (under a lock) the profiled data for the tasks in each ThreadData
-  // instance.  Also updates the |birth_counts| tally for each task to keep
-  // track of the number of living instances of the task.
-  static void SnapshotAllExecutedTasks(
-      ProcessDataPhaseSnapshot* process_data_phase,
+  // Snapshots (under a lock) the profiled data for the tasks for this thread
+  // and writes all of the executed tasks' data -- i.e. the data for all
+  // profiling phases (including the current one: |current_profiling_phase|) for
+  // the tasks with with entries in the death_map_ -- into
+  // |return_of_snapshot_hrenina|. Also updates the |birth_counts| tally for
+  // each task to keep track of the number of living instances of the task --
+  // that is, each task maps to the number of births for the task that have not
+  // yet been balanced by a death.
+  void SnapshotExecutedTasks(
+      int current_profiling_phase,
+      PhasedProcessDataSnapshotMap* phased_process_data_snapshots,
       BirthCountMap* birth_counts);
 
-  // Fills |process_data_phase| with all the recursive results in our process.
-  static void SnapshotCurrentPhase(
-      ProcessDataPhaseSnapshot* process_data_phase);
-
-  // Snapshots (under a lock) the profiled data for the tasks for this thread
-  // and writes all of the executed tasks' data -- i.e. the data for the tasks
-  // with with entries in the death_map_ -- into |process_data_phase|.  Also
-  // updates the |birth_counts| tally for each task to keep track of the number
-  // of living instances of the task -- that is, each task maps to the number of
-  // births for the task that have not yet been balanced by a death.
-  void SnapshotExecutedTasks(ProcessDataPhaseSnapshot* process_data_phase,
-                             BirthCountMap* birth_counts);
-
   // Using our lock, make a copy of the specified maps.  This call may be made
   // on  non-local threads, which necessitate the use of the lock to prevent
   // the map(s) from being reallocated while they are copied.
-  void SnapshotMaps(BirthMap* birth_map,
-                    DeathMap* death_map,
+  void SnapshotMaps(int profiling_phase,
+                    BirthMap* birth_map,
+                    DeathsSnapshot* deaths,
                     ParentChildSet* parent_child_set);
 
+  // Called for this thread when the current profiling phase, identified by
+  // |profiling_phase|, ends.
+  void OnProfilingPhaseCompletionOnThread(int profiling_phase);
+
   // This method is called by the TLS system when a thread terminates.
   // The argument may be NULL if this thread has never tracked a birth or death.
   static void OnThreadTermination(void* thread_data);
 
   // This method should be called when a worker thread terminates, so that we
   // can save all the thread data into a cache of reusable ThreadData instances.
   void OnThreadTerminationCleanup();
 
   // Cleans up data structures, and returns statics to near pristine (mostly
   // uninitialized) state.  If there is any chance that other threads are still
@@ skipping 222 matching lines @@
   ProcessDataSnapshot();
   ~ProcessDataSnapshot();
 
   PhasedProcessDataSnapshotMap phased_process_data_snapshots;
   base::ProcessId process_id;
 };
 
 }  // namespace tracked_objects
 
 #endif  // BASE_TRACKED_OBJECTS_H_