Reuse ThreadData instances associated with terminated named threads.

base/tracked_objects.cc

Index: base/tracked_objects.cc
diff --git a/base/tracked_objects.cc b/base/tracked_objects.cc
index 675c9b89e6747ff288902e78ca280f006045281b..0e089071b2de81007e888c75053c216a0bdd1c29 100644
--- a/base/tracked_objects.cc
+++ b/base/tracked_objects.cc
@@ -4,6 +4,7 @@
 
 #include "base/tracked_objects.h"
 
+#include <ctype.h>
 #include <limits.h>
 #include <stdlib.h>
 
@@ -14,7 +15,6 @@
 #include "base/debug/leak_annotations.h"
 #include "base/logging.h"
 #include "base/process/process_handle.h"
-#include "base/strings/stringprintf.h"
 #include "base/third_party/valgrind/memcheck.h"
 #include "base/threading/worker_pool.h"
 #include "base/tracking_info.h"
@@ -74,6 +74,22 @@ inline bool IsProfilerTimingEnabled() {
   return current_timing_enabled == ENABLED_TIMING;
 }
 
+// Sanitize a thread name by replacing trailing sequence of digits with "*".
+// Examples:
+// 1. "BrowserBlockingWorker1/23857" => "BrowserBlockingWorker1/*"
+// 2. "Chrome_IOThread" => "Chrome_IOThread"
+std::string SanitizeThreadName(const std::string& thread_name) {
+  size_t i = thread_name.length();
+
+  while (i > 0 && isdigit(thread_name[i - 1]))
+    --i;
+
+  if (i == thread_name.length())
+    return thread_name;
+
+  return thread_name.substr(0, i) + '*';
+}
+
 }  // namespace
 
 //------------------------------------------------------------------------------
@@ -252,8 +268,7 @@ BirthOnThreadSnapshot::BirthOnThreadSnapshot() {
 
 BirthOnThreadSnapshot::BirthOnThreadSnapshot(const BirthOnThread& birth)
     : location(birth.location()),
-      thread_name(birth.birth_thread()->thread_name()) {
-}
+      thread_name(birth.birth_thread()->sanitized_thread_name()) {}
 
 BirthOnThreadSnapshot::~BirthOnThreadSnapshot() {
 }
@@ -285,9 +300,6 @@ ThreadData::NowFunction* ThreadData::now_function_for_testing_ = NULL;
 base::ThreadLocalStorage::StaticSlot ThreadData::tls_index_ = TLS_INITIALIZER;
 
 // static
-int ThreadData::worker_thread_data_creation_count_ = 0;
-
-// static
 int ThreadData::cleanup_count_ = 0;
 
 // static
@@ -297,7 +309,7 @@ int ThreadData::incarnation_counter_ = 0;
 ThreadData* ThreadData::all_thread_data_list_head_ = NULL;
 
 // static
-ThreadData* ThreadData::first_retired_worker_ = NULL;
+ThreadData* ThreadData::first_retired_thread_data_ = NULL;
 
 // static
 base::LazyInstance<base::Lock>::Leaky
@@ -306,25 +318,12 @@ base::LazyInstance<base::Lock>::Leaky
 // static
 base::subtle::Atomic32 ThreadData::status_ = ThreadData::UNINITIALIZED;
 
-ThreadData::ThreadData(const std::string& suggested_name)
+ThreadData::ThreadData(const std::string& sanitized_thread_name)
     : next_(NULL),
-      next_retired_worker_(NULL),
-      worker_thread_number_(0),
+      next_retired_thread_data_(NULL),
+      sanitized_thread_name_(sanitized_thread_name),
       incarnation_count_for_pool_(-1),
       current_stopwatch_(NULL) {
-  DCHECK_GE(suggested_name.size(), 0u);
-  thread_name_ = suggested_name;
-  PushToHeadOfList();  // Which sets real incarnation_count_for_pool_.
-}
-
-ThreadData::ThreadData(int thread_number)
-    : next_(NULL),
-      next_retired_worker_(NULL),
-      worker_thread_number_(thread_number),
-      incarnation_count_for_pool_(-1),
-      current_stopwatch_(NULL) {
-  CHECK_GT(thread_number, 0);
-  base::StringAppendF(&thread_name_, "WorkerThread-%d", thread_number);
   PushToHeadOfList();  // Which sets real incarnation_count_for_pool_.

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

maybe DCHECK(sanitized_thread_name.empty() ||
!isdigit(sanitized_thread_name.back()))?

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

Done.

 }
 
@@ -355,7 +354,7 @@ ThreadData* ThreadData::first() {
 ThreadData* ThreadData::next() const { return next_; }
 
 // static
-void ThreadData::InitializeThreadContext(const std::string& suggested_name) {
+void ThreadData::InitializeThreadContext(const std::string& thread_name) {
   if (base::WorkerPool::RunsTasksOnCurrentThread())
     return;
   EnsureTlsInitialization();
@@ -363,7 +362,8 @@ void ThreadData::InitializeThreadContext(const std::string& suggested_name) {
       reinterpret_cast<ThreadData*>(tls_index_.Get());
   if (current_thread_data)
     return;  // Browser tests instigate this.
-  current_thread_data = new ThreadData(suggested_name);
+  current_thread_data =
+      GetRetiredOrCreateThreadData(SanitizeThreadName(thread_name));
   tls_index_.Set(current_thread_data);
 }
 
@@ -376,26 +376,8 @@ ThreadData* ThreadData::Get() {
     return registered;
 
   // We must be a worker thread, since we didn't pre-register.
-  ThreadData* worker_thread_data = NULL;
-  int worker_thread_number = 0;
-  {
-    base::AutoLock lock(*list_lock_.Pointer());
-    if (first_retired_worker_) {
-      worker_thread_data = first_retired_worker_;
-      first_retired_worker_ = first_retired_worker_->next_retired_worker_;
-      worker_thread_data->next_retired_worker_ = NULL;
-    } else {
-      worker_thread_number = ++worker_thread_data_creation_count_;
-    }
-  }
-
-  // If we can't find a previously used instance, then we have to create one.
-  if (!worker_thread_data) {
-    DCHECK_GT(worker_thread_number, 0);
-    worker_thread_data = new ThreadData(worker_thread_number);
-  }
-  DCHECK_GT(worker_thread_data->worker_thread_number_, 0);
-
+  ThreadData* worker_thread_data =
+      GetRetiredOrCreateThreadData("WorkerThread-*");

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

How much will things explode if someone one day decides to create a named thread
called "WorkerThread"?

(Can't think of anything better than a string-y ID type for threads either
though)

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

I added a DCHECK in InitializeThreadContext() to prevent that.

   tls_index_.Set(worker_thread_data);
   return worker_thread_data;
 }
@@ -409,21 +391,21 @@ void ThreadData::OnThreadTermination(void* thread_data) {
 }
 
 void ThreadData::OnThreadTerminationCleanup() {
+  // We must NOT do any allocations during this callback. There is a chance that
+  // the allocator is no longer active on this thread.
+
   // The list_lock_ was created when we registered the callback, so it won't be
   // allocated here despite the lazy reference.
   base::AutoLock lock(*list_lock_.Pointer());
   if (incarnation_counter_ != incarnation_count_for_pool_)
     return;  // ThreadData was constructed in an earlier unit test.
   ++cleanup_count_;
-  // Only worker threads need to be retired and reused.
-  if (!worker_thread_number_) {
-    return;
-  }
-  // We must NOT do any allocations during this callback.
-  // Using the simple linked lists avoids all allocations.
-  DCHECK_EQ(this->next_retired_worker_, reinterpret_cast<ThreadData*>(NULL));
-  this->next_retired_worker_ = first_retired_worker_;
-  first_retired_worker_ = this;
+
+  // Add this ThreadData to a retired list so that it can be reused by a thread
+  // with the same name sanitized name in the future.
+  DCHECK(!next_retired_thread_data_);

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

Why is this dcheck true?

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

See comment on member declaration. I also added a comment here.

+  next_retired_thread_data_ = first_retired_thread_data_;
+  first_retired_thread_data_ = this;
 }
 
 // static
@@ -635,7 +617,7 @@ void ThreadData::SnapshotExecutedTasks(
       if (death_data.count > 0) {
         (*phased_snapshots)[phase->profiling_phase].tasks.push_back(
             TaskSnapshot(BirthOnThreadSnapshot(*death.first), death_data,
-                         thread_name()));
+                         sanitized_thread_name()));
       }
     }
   }
@@ -746,8 +728,6 @@ TrackedTime ThreadData::Now() {
 // static
 void ThreadData::EnsureCleanupWasCalled(int major_threads_shutdown_count) {
   base::AutoLock lock(*list_lock_.Pointer());
-  if (worker_thread_data_creation_count_ == 0)
-    return;  // We haven't really run much, and couldn't have leaked.
 
   // TODO(jar): until this is working on XP, don't run the real test.
 #if 0
@@ -772,16 +752,14 @@ void ThreadData::ShutdownSingleThreadedCleanup(bool leak) {
     all_thread_data_list_head_ = NULL;
     ++incarnation_counter_;
     // To be clean, break apart the retired worker list (though we leak them).
-    while (first_retired_worker_) {
-      ThreadData* worker = first_retired_worker_;
-      CHECK_GT(worker->worker_thread_number_, 0);
-      first_retired_worker_ = worker->next_retired_worker_;
-      worker->next_retired_worker_ = NULL;
+    while (first_retired_thread_data_) {
+      ThreadData* thread_data = first_retired_thread_data_;
+      first_retired_thread_data_ = thread_data->next_retired_thread_data_;
+      thread_data->next_retired_thread_data_ = nullptr;
     }
   }
 
   // Put most global static back in pristine shape.
-  worker_thread_data_creation_count_ = 0;
   cleanup_count_ = 0;
   tls_index_.Set(NULL);
   // Almost UNINITIALIZED.
@@ -813,6 +791,42 @@ void ThreadData::ShutdownSingleThreadedCleanup(bool leak) {
   }
 }
 
+// static
+ThreadData* ThreadData::GetRetiredOrCreateThreadData(
+    const std::string& sanitized_thread_name) {
+  {
+    base::AutoLock lock(*list_lock_.Pointer());
+    ThreadData** previous_thread_data_next_retired_thread_data_pointer =

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

this name is both long too long for a local and confusing (not clear to me what
"previous", "next" and "pointer" refer to). I'd just call this "pcursor" or
"pcurrent" or something like that, and s/current_thread_data/cursor/ (or
current, or...), and with all the characters you save you can consider putting
in a link to something like
http://grisha.org/blog/2013/04/02/linus-on-understanding-pointers/ if someone
hasn't seen this -- but I think it's pretty well-known)

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

Done.

+        &first_retired_thread_data_;
+    ThreadData* current_thread_data = first_retired_thread_data_;
+
+    // Assuming that there aren't more than a few tens of retired ThreadData

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

I'm a bit nervous about having to do potentially unbounded work every time a
thread is created. After some looking, I couldn't find any uma metrics that'd
alert us if this function ended up taking a lot of time. What's the plan for
making sure that this really is "few tens" in practice going forward, and not
just now?

(XXX)

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

I added a UMA histogram to keep track of the time spent in this function. I will
register to be notified if it ever changes. I also thought about keeping track
of the number of traversed ThreadData instances, but I decided that time would
be more actionable (100 ms spent in this function is clearly bad while it's hard
to say if traversing 100 ThreadData instances is bad).

+    // instances, this lookup should be quick compared to the thread creation
+    // time. Retired ThreadData instances cannot be stored in a map because
+    // insertions are done from OnThreadTerminationCleanup() where allocations
+    // are not allowed.
+    //
+    // Note: Test processes may have more than a few tens or retired ThreadData

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

s/or/of/

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

Done.

+    // instances.
+    while (current_thread_data) {
+      if (current_thread_data->sanitized_thread_name() ==
+          sanitized_thread_name) {
+        DCHECK_EQ(*previous_thread_data_next_retired_thread_data_pointer,
+                  current_thread_data);
+        *previous_thread_data_next_retired_thread_data_pointer =
+            current_thread_data->next_retired_thread_data_;
+        current_thread_data->next_retired_thread_data_ = nullptr;
+        return current_thread_data;
+      }
+      previous_thread_data_next_retired_thread_data_pointer =
+          &current_thread_data->next_retired_thread_data_;
+      current_thread_data = current_thread_data->next_retired_thread_data_;
+    }
+  }
+
+  return new ThreadData(sanitized_thread_name);
+}
+
 //------------------------------------------------------------------------------
 TaskStopwatch::TaskStopwatch()
     : wallclock_duration_ms_(0),