Reuse ThreadData instances associated with terminated named threads.

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 <stdint.h>
 
 #include <map>
@@ skipping 41 matching lines @@
 //
 // First off, when the instance is created, the FROM_HERE macro is expanded
 // to specify the birth place (file, line, function) where the instance was
 // created.  That data is used to create a transient Location instance
 // encapsulating the above triple of information.  The strings (like __FILE__)
 // are passed around by reference, with the assumption that they are static, and
 // will never go away.  This ensures that the strings can be dealt with as atoms
 // with great efficiency (i.e., copying of strings is never needed, and
 // comparisons for equality can be based on pointer comparisons).
 //
-// Next, a Births instance is created for use ONLY on the thread where this
-// instance was created.  That Births instance records (in a base class
-// BirthOnThread) references to the static data provided in a Location instance,
-// as well as a pointer specifying the thread on which the birth takes place.
-// Hence there is at most one Births instance for each Location on each thread.
-// The derived Births class contains slots for recording statistics about all
-// instances born at the same location.  Statistics currently include only the
-// count of instances constructed.
+// Next, a Births instance is constructed or found. A Births instance records
+// (in a base class BirthOnThread) references to the static data provided in a
+// Location instance, as well as a pointer to the ThreadData bound to the thread
+// on which the birth takes place (see discussion on ThreadData below). There is
+// at most one Births instance for each Location / ThreadData pair. The derived
+// Births class contains slots for recording statistics about all instances born
+// at the same location. Statistics currently include only the count of
+// instances constructed.
 //
 // Since the base class BirthOnThread contains only constant data, it can be
-// freely accessed by any thread at any time (i.e., only the statistic needs to
-// be handled carefully, and stats are updated exclusively on the birth thread).
+// freely accessed by any thread at any time. The statistics must be handled
+// more carefully; they are updated exclusively by the single thread to which
+// the ThreadData is bound at a given time.
 //
 // For Tasks, having now either constructed or found the Births instance
 // described above, a pointer to the Births instance is then recorded into the
-// PendingTask structure in MessageLoop.  This fact alone is very useful in
-// debugging, when there is a question of where an instance came from.  In
-// addition, the birth time is also recorded and used to later evaluate the
-// lifetime duration of the whole Task.  As a result of the above embedding, we
-// can find out a Task's location of birth, and thread of birth, without using
-// any locks, as all that data is constant across the life of the process.
+// PendingTask structure.  This fact alone is very useful in debugging, when
+// there is a question of where an instance came from.  In addition, the birth
+// time is also recorded and used to later evaluate the lifetime duration of the
+// whole Task.  As a result of the above embedding, we can find out a Task's
+// location of birth, and name of birth thread, without using any locks, as all
+// that data is constant across the life of the process.
 //
 // The above work *could* also be done for any other object as well by calling
 // TallyABirthIfActive() and TallyRunOnNamedThreadIfTracking() as appropriate.
 //
-// The amount of memory used in the above data structures depends on how many
-// threads there are, and how many Locations of construction there are.
-// Fortunately, we don't use memory that is the product of those two counts, but
-// rather we only need one Births instance for each thread that constructs an
-// instance at a Location.  In many cases, instances are only created on one
-// thread, so the memory utilization is actually fairly restrained.
+// The maxium amount of memory used in the above data structures is the product

[Nico, 2016/11/24 01:42:04]

typo maxium

Also, the changes to this paragraph are pretty funny :-D ("fortunately we don't
use memory proportional to the product because…" -> "we use memory proportional
to the product, but…" – but from what I understand you're just rewording the lhs
and this doesn't actually change?)

[fdoray, 2016/11/29 16:10:33]

I'm saying the same thing as before, with fewer words (except that I changed
"threads" to "ThreadData instances" to highlight the fact that repetitively
creating/destroying a thread with the same name doesn't have an impact on the
memory used by tracked_objects).

+// of the number of ThreadData instances and the number of Locations.
+// Fortunately, Locations are often created on a single thread and the memory
+// utilization is actually fairly restrained.
 //
 // Lastly, when an instance is deleted, the final tallies of statistics are
 // carefully accumulated.  That tallying writes into slots (members) in a
-// collection of DeathData instances.  For each birth place Location that is
-// destroyed on a thread, there is a DeathData instance to record the additional
-// death count, as well as accumulate the run-time and queue-time durations for
-// the instance as it is destroyed (dies).  By maintaining a single place to
-// aggregate this running sum *only* for the given thread, we avoid the need to
-// lock such DeathData instances. (i.e., these accumulated stats in a DeathData
-// instance are exclusively updated by the singular owning thread).
+// collection of DeathData instances.  For each Births / death ThreadData pair,
+// there is a DeathData instance to record the additional death count, as well
+// as accumulate the run-time and queue-time durations for the instance as it is

[Nico, 2016/11/24 01:42:04]

"as well as to accumulate"? (lhs is the same as rhs, but I found the sentence
hard to parse. then again, not a native speaker)

[fdoray, 2016/11/29 16:10:33]

Done.

+// destroyed (dies). Since a ThreadData is bound to at most one thread at a
+// time, there is no need to lock such DeathData instances. (i.e., these
+// accumulated stats in a DeathData instance are exclusively updated by the
+// singular owning thread).
 //
-// With the above life cycle description complete, the major remaining detail
-// is explaining how each thread maintains a list of DeathData instances, and
-// of Births instances, and is able to avoid additional (redundant/unnecessary)
-// allocations.
+// With the above life cycle description complete, the major remaining detail is
+// explaining how existing Births and DeathData instances are found to avoid
+// redundant allocations.
 //
-// Each thread maintains a list of data items specific to that thread in a
-// ThreadData instance (for that specific thread only).  The two critical items
-// are lists of DeathData and Births instances.  These lists are maintained in
-// STL maps, which are indexed by Location.  As noted earlier, we can compare
-// locations very efficiently as we consider the underlying data (file,
-// function, line) to be atoms, and hence pointer comparison is used rather than
-// (slow) string comparisons.
+// A ThreadData instance maintains maps of Births and DeathData instances. The
+// Births map is indexed by Location and the DeathData map is indexed by
+// Births*. As noted earlier, we can compare Locations very efficiently as we
+// consider the underlying data (file, function, line) to be atoms, and hence
+// pointer comparison is used rather than (slow) string comparisons.
 //
-// To provide a mechanism for iterating over all "known threads," which means
-// threads that have recorded a birth or a death, we create a singly linked list
-// of ThreadData instances.  Each such instance maintains a pointer to the next
-// one.  A static member of ThreadData provides a pointer to the first item on
-// this global list, and access via that all_thread_data_list_head_ item
-// requires the use of the list_lock_.
+// The first time that a thread calls ThreadData::InitializeThreadContext() or
+// ThreadData::Get(), a ThreadData instance is bound to it and stored in TLS. If
+// a ThreadData bound to a terminated thread with the same sanitized name (i.e.
+// name without trailing digits) as the current thread is available, it is
+// reused. Otherwise, a new ThreadData instance is instantiated. Since a
+// ThreadData is bound to at most one thread at a time, there is no need to
+// acquire a lock to access its maps. Over time, a ThreadData may be bound to
+// different threads that share the same sanitized name.
+//
+// We maintain a list of all ThreadData instances for the current process. Each
+// ThreadData instance has a pointer to the next one. A static member of
+// ThreadData provides a pointer to the first item on this global list, and
+// access via that all_thread_data_list_head_ item requires the use of the
+// list_lock_.

[Nico, 2016/11/24 01:42:04]

Thanks for the comprehensive comment update!

[fdoray, 2016/11/29 16:10:33]

Acknowledged.

+//
 // When new ThreadData instances is added to the global list, it is pre-pended,

[Nico, 2016/11/24 01:42:04]

(s/is added/are added/. yes, not your code.)

[fdoray, 2016/11/29 16:10:33]

Done.

 // which ensures that any prior acquisition of the list is valid (i.e., the
 // holder can iterate over it without fear of it changing, or the necessity of
 // using an additional lock.  Iterations are actually pretty rare (used
 // primarily for cleanup, or snapshotting data for display), so this lock has
 // very little global performance impact.
 //
 // The above description tries to define the high performance (run time)
 // portions of these classes.  After gathering statistics, calls instigated
 // by visiting about:profiler will assemble and aggregate data for display.  The
 // following data structures are used for producing such displays.  They are
@@ skipping 29 matching lines @@
 // atomically collecting all data, so we could have count that does not, for
 // example, match with the number of durations we accumulated).  The advantage
 // to having fast (non-atomic) updates of the data outweighs the minimal risk of
 // a singular corrupt statistic snapshot (only the snapshot could be corrupt,
 // not the underlying and ongoing statistic).  In contrast, pointer data that
 // is accessed during snapshotting is completely invariant, and hence is
 // perfectly acquired (i.e., no potential corruption, and no risk of a bad
 // memory reference).
 //
 // TODO(jar): We can implement a Snapshot system that *tries* to grab the
-// snapshots on the source threads *when* they have MessageLoops available
-// (worker threads don't have message loops generally, and hence gathering from
-// them will continue to be asynchronous).  We had an implementation of this in
-// the past, but the difficulty is dealing with message loops being terminated.
-// We can *try* to spam the available threads via some task runner to
-// achieve this feat, and it *might* be valuable when we are collecting data
+// snapshots on the source threads *when* they have SingleThreadTaskRunners
+// available (worker threads don't have SingleThreadTaskRunners, and hence
+// gathering from them will continue to be asynchronous).  We had an
+// implementation of this in the past, but the difficulty is dealing with
+// threads being terminated. We can *try* to post a task to threads that have a
+// SingleThreadTaskRunner and check if that succeeds (will fail if the thread
+// has been terminated). This *might* be valuable when we are collecting data
 // for upload via UMA (where correctness of data may be more significant than
 // for a single screen of about:profiler).
 //
 // TODO(jar): We need to store DataCollections, and provide facilities for
 // taking the difference between two gathered DataCollections.  For now, we're
 // just adding a hack that Reset()s to zero all counts and stats.  This is also
 // done in a slightly thread-unsafe fashion, as the resetting is done
 // asynchronously relative to ongoing updates (but all data is 32 bit in size).
 // For basic profiling, this will work "most of the time," and should be
 // sufficient... but storing away DataCollections is the "right way" to do this.
@@ skipping 30 matching lines @@
 
 //------------------------------------------------------------------------------
 // A "snapshotted" representation of the BirthOnThread class.
 
 struct BASE_EXPORT BirthOnThreadSnapshot {
   BirthOnThreadSnapshot();
   explicit BirthOnThreadSnapshot(const BirthOnThread& birth);
   ~BirthOnThreadSnapshot();
 
   LocationSnapshot location;
   std::string thread_name;

[Nico, 2016/11/24 01:42:04]

This should probably now be called sanitized_name or at least grow a comment
explaining that it's not actually the thread_name any more?

https://cs.chromium.org/chromium/src/chrome/browser/task_profiler/task_profil...
wasn't changed – is that code fine with receiving sanitized names now? Probably
a progression? (maybe something checks if the names are identical and if so
assumes a task was born and died on the same thread – that wouldn't work
anymore. didn't check if that happens though)

[fdoray, 2016/11/29 16:10:33]

Done.

The receiver of the json generated by task_profiler_data_serializer.cc replaces
trailing digits with *.  If names without trailing digits are provided, this
will be a no-op. I plan to clean the javascript code in a separate CL.

Code that replaces trailing digits with *:
https://cs.chromium.org/chromium/src/chrome/browser/resources/profiler/profil...
/
This behavior is controlled by an hidden check box that is checked by default
(i.e. behavior is always enabled):
https://cs.chromium.org/chromium/src/chrome/browser/resources/profiler/profil...

I plan to clean the Javascript/HTML code in a separate CL.

 };
 
 //------------------------------------------------------------------------------
 // A class for accumulating counts of births (without bothering with a map<>).
 
 class BASE_EXPORT Births: public BirthOnThread {
  public:
   Births(const Location& location, const ThreadData& current);
 
   int birth_count() const;
@@ skipping 203 matching lines @@
     DEACTIVATED,           // No longer recording profiling.
     PROFILING_ACTIVE,      // Recording profiles.
     STATUS_LAST = PROFILING_ACTIVE
   };
 
   typedef base::hash_map<Location, Births*, Location::Hash> BirthMap;
   typedef std::map<const Births*, DeathData> DeathMap;
 
   // Initialize the current thread context with a new instance of ThreadData.
   // This is used by all threads that have names, and should be explicitly
-  // set *before* any births on the threads have taken place.  It is generally
-  // only used by the message loop, which has a well defined thread name.
-  static void InitializeThreadContext(const std::string& suggested_name);
+  // set *before* any births on the threads have taken place.
+  static void InitializeThreadContext(const std::string& thread_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, identified by |current_profiling_phase|.
   // |current_profiling_phase| is necessary because a child process can start
@@ skipping 37 matching lines @@
   // finished).
   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* births,
                                                 const TaskStopwatch& stopwatch);
 
-  const std::string& thread_name() const { return thread_name_; }
+  const std::string& sanitized_thread_name() const {
+    return sanitized_thread_name_;
+  }
 
   // Initializes all statics if needed (this initialization call should be made
   // while we are single threaded).
   static void EnsureTlsInitialization();
 
   // Sets internal status_.
   // If |status| is false, then status_ is set to DEACTIVATED.
   // If |status| is true, then status_ is set to PROFILING_ACTIVE.
   static void InitializeAndSetTrackingStatus(Status status);
 
@@ skipping 28 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(TrackedObjectsTest, MinimalStartupShutdown);
   FRIEND_TEST_ALL_PREFIXES(TrackedObjectsTest, TinyStartupShutdown);
 
   // Type for an alternate timer function (testing only).
   typedef unsigned int NowFunction();
 
   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);
-
+  explicit ThreadData(const std::string& sanitized_thread_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();
 
@@ skipping 43 matching lines @@
   // uninitialized) state.  If there is any chance that other threads are still
   // using the data structures, then the |leak| argument should be passed in as
   // true, and the data structures (birth maps, death maps, ThreadData
   // insntances, etc.) will be leaked and not deleted.  If you have joined all
   // threads since the time that InitializeAndSetTrackingStatus() was called,
   // then you can pass in a |leak| value of false, and this function will
   // delete recursively all data structures, starting with the list of
   // ThreadData instances.
   static void ShutdownSingleThreadedCleanup(bool leak);
 
+  // Returns a ThreadData instance for a thread whose sanitized name is
+  // |sanitized_thread_name|. The returned instance may have been extracted from
+  // the list of retired ThreadData instances or newly allocated.
+  static ThreadData* GetRetiredOrCreateThreadData(
+      const std::string& sanitized_thread_name);
+
   // When non-null, this specifies an external function that supplies monotone
   // increasing time functcion.
   static NowFunction* now_function_for_testing_;
 
   // We use thread local store to identify which ThreadData to interact with.
   static base::ThreadLocalStorage::StaticSlot tls_index_;
 
-  // List of ThreadData instances for use with worker threads.  When a worker
-  // thread is done (terminated), we push it onto this list.  When a new worker
-  // thread is created, we first try to re-use a ThreadData instance from the
-  // list, and if none are available, construct a new one.
-  // This is only accessed while list_lock_ is held.
-  static ThreadData* first_retired_worker_;
+  // Linked list of ThreadData instances that were associated with threads that
+  // have been terminated and that have not been associated with a new thread
+  // since then. This is only accessed while |list_lock_| is held.
+  static ThreadData* first_retired_thread_data_;
 
   // Link to the most recently created instance (starts a null terminated list).
   // The list is traversed by about:profiler when it needs to snapshot data.
   // This is only accessed while list_lock_ is held.
   static ThreadData* all_thread_data_list_head_;
 
-  // The next available worker thread number.  This should only be accessed when
-  // the list_lock_ is held.
-  static int worker_thread_data_creation_count_;
-
   // The number of times TLS has called us back to cleanup a ThreadData
   // instance.  This is only accessed while list_lock_ is held.
   static int cleanup_count_;
 
   // Incarnation sequence number, indicating how many times (during unittests)
   // we've either transitioned out of UNINITIALIZED, or into that state.  This
   // value is only accessed while the list_lock_ is held.
   static int incarnation_counter_;
 
   // Protection for access to all_thread_data_list_head_, and to
   // unregistered_thread_data_pool_.  This lock is leaked at shutdown.
   // The lock is very infrequently used, so we can afford to just make a lazy
   // instance and be safe.
   static base::LazyInstance<base::Lock>::Leaky list_lock_;
 
   // We set status_ to SHUTDOWN when we shut down the tracking service.
   static base::subtle::Atomic32 status_;
 
   // Link to next instance (null terminated list).  Used to globally track all
   // registered instances (corresponds to all registered threads where we keep
   // data).
   ThreadData* next_;
 
-  // Pointer to another ThreadData instance for a Worker-Thread that has been
-  // retired (its thread was terminated).  This value is non-NULL only for a
-  // retired ThreadData associated with a Worker-Thread.
-  ThreadData* next_retired_worker_;
+  // Pointer to another retired ThreadData instance. This value is nullptr if
+  // this is associated with an active thread.
+  ThreadData* next_retired_thread_data_;
 
-  // The name of the thread that is being recorded.  If this thread has no
-  // message_loop, then this is a worker thread, with a sequence number postfix.
-  std::string thread_name_;
-
-  // Indicate if this is a worker thread, and the ThreadData contexts should be
-  // stored in the unregistered_thread_data_pool_ when not in use.
-  // Value is zero when it is not a worker thread.  Value is a positive integer
-  // corresponding to the created thread name if it is a worker thread.
-  int worker_thread_number_;
+  // The name of the thread that is being recorded, with all trailing digits
+  // replaced with a single "*" character.
+  const std::string sanitized_thread_name_;
 
   // A map used on each thread to keep track of Births on this thread.
   // This map should only be accessed on the thread it was constructed on.
   // When a snapshot is needed, this structure can be locked in place for the
   // duration of the snapshotting activity.
   BirthMap birth_map_;
 
   // Similar to birth_map_, this records informations about death of tracked
   // instances (i.e., when a tracked instance was destroyed on this thread).
   // It is locked before changing, and hence other threads may access it by
@@ skipping 110 matching lines @@
   ProcessDataSnapshot(const ProcessDataSnapshot& other);
   ~ProcessDataSnapshot();
 
   PhasedProcessDataSnapshotMap phased_snapshots;
   base::ProcessId process_id;
 };
 
 }  // namespace tracked_objects
 
 #endif  // BASE_TRACKED_OBJECTS_H_