Avoid any possibility of an Alloc during TLS thread teardown

base/tracked_objects.cc

Index: base/tracked_objects.cc
===================================================================
--- base/tracked_objects.cc	(revision 110300)
+++ base/tracked_objects.cc	(working copy)
@@ -11,6 +11,7 @@
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
+#include "base/port.h"
 
 using base::TimeDelta;
 
@@ -147,22 +148,17 @@
 ThreadData::ThreadData(const std::string& suggested_name)
     : incarnation_count_for_pool_(-1),
       next_(NULL),
-      is_a_worker_thread_(false) {
+      worker_thread_number_(0) {
   DCHECK_GE(suggested_name.size(), 0u);
   thread_name_ = suggested_name;
   PushToHeadOfList();  // Which sets real incarnation_count_for_pool_.
 }
 
-ThreadData::ThreadData()
+ThreadData::ThreadData(size_t thread_number)
     : incarnation_count_for_pool_(-1),
       next_(NULL),
-      is_a_worker_thread_(true) {
-  int thread_number;
-  {
-    base::AutoLock lock(*list_lock_.Pointer());
-    thread_number = ++thread_number_counter_;
-  }
-  base::StringAppendF(&thread_name_, "WorkerThread-%d", thread_number);
+      worker_thread_number_(thread_number) {
+  base::StringAppendF(&thread_name_, "WorkerThread-%"PRIuS, thread_number);
   PushToHeadOfList();  // Which sets real incarnation_count_for_pool_.
 }
 
@@ -198,19 +194,25 @@
 
   // We must be a worker thread, since we didn't pre-register.
   ThreadData* worker_thread_data = NULL;
+  size_t thread_number = 0;
   {
     base::AutoLock lock(*list_lock_.Pointer());
-    if (unregistered_thread_data_pool_ &&
-        !unregistered_thread_data_pool_->empty()) {
+    if (!unregistered_thread_data_pool_)
+      unregistered_thread_data_pool_ = new ThreadDataPool;
+    if (!unregistered_thread_data_pool_->empty()) {
       worker_thread_data =
         const_cast<ThreadData*>(unregistered_thread_data_pool_->top());
       unregistered_thread_data_pool_->pop();

[ramant (doing other things), 2011/11/17 22:59:36]

nit: Should we consider adding a DCHECK for worker_thread_data? Can it be NULL?

[jar (doing other things), 2011/11/18 01:06:42]

Line 205 would access lower memory if the instance were null... so we
effectively have a CHECK() call (at no extra charge).

On 2011/11/17 22:59:36, ramant wrote:

+    } else {
+      thread_number = ++thread_number_counter_;
+      unregistered_thread_data_pool_->reserve(thread_number);
     }
   }
 
   // If we can't find a previously used instance, then we have to create one.
   if (!worker_thread_data)
-    worker_thread_data = new ThreadData();
+    worker_thread_data = new ThreadData(thread_number);

[ramant (doing other things), 2011/11/17 22:59:36]

nit: can thread_number be zero? If not should we consider adding a DCHECK?
thanks

[jar (doing other things), 2011/11/18 01:06:42]

Line 215 checks that via the DCHECK().  We're loading thread number in... and it
verifies that it was indeed non-zero.

+  DCHECK_GT(worker_thread_data->worker_thread_number_, 0u);
 
   tls_index_.Set(worker_thread_data);
   return worker_thread_data;
@@ -226,18 +228,12 @@
 }
 
 void ThreadData::OnThreadTerminationCleanup() const {
-  if (!is_a_worker_thread_)
+  if (!worker_thread_number_)
     return;
   base::AutoLock lock(*list_lock_.Pointer());
   if (incarnation_counter_ != incarnation_count_for_pool_)
     return;  // ThreadData was constructed in an earlier unit test.
-
-  // Handle case where we are in unit tests, and have become UNINITIALIZED.
-  // In that case, the pool might be NULL.  We really should detect this via the
-  // incarnation_counter_, but this call is rarely made, so we can afford to
-  // code defensively.
-  if (!unregistered_thread_data_pool_)
-    unregistered_thread_data_pool_ = new ThreadDataPool;
+  // The following will never have to do an allocation.
   unregistered_thread_data_pool_->push(this);

[ramant (doing other things), 2011/11/17 22:59:36]

nit: Can we add a DCHECK for unregistered_thread_data_pool_?

[jar (doing other things), 2011/11/18 01:06:42]

If unregistered_thread_data_pool_ is null, then we'll access memory near zero,
and segv anyway... so the DCHECK seems redundant (it is almost like we have a
CHECK on this line ;-) )

On 2011/11/17 22:59:36, ramant wrote:

 }
 
@@ -562,6 +558,49 @@
 }
 
 //------------------------------------------------------------------------------
+// Small partial implementation of a stack that never has to allocate during a
+// push() operation, because it is always prepared to accept the maximum number
+// of ThreadData instances (all the worker thread related instances).
+
+ThreadData::ThreadDataPool::ThreadDataPool() : empty_slot_(0) {};
+ThreadData::ThreadDataPool::~ThreadDataPool() {};
+
+bool ThreadData::ThreadDataPool::empty() const { return empty_slot_ == 0; }
+
+void ThreadData::ThreadDataPool::reserve(size_t largest_worker_pool_number) {
+  // Worker pool numbers start at 1, and exclude 0, so the number is exactly
+  // the least size needed.
+  // Due to asynchronous construction of worker-pool numbers (and associated
+  // ThreadData), we might not hear about the numbers sequentially.
+  if (largest_worker_pool_number > stack_.size())
+    stack_.resize(largest_worker_pool_number);
+}
+
+const ThreadData* ThreadData::ThreadDataPool::top() const {
+  if (empty_slot_ > 0)
+    return stack_[empty_slot_ - 1];
+  NOTREACHED();
+  return NULL;
+}
+
+void ThreadData::ThreadDataPool::push(const ThreadData* thread_data) {
+  if (empty_slot_ < stack_.size()) {

[ramant (doing other things), 2011/11/17 22:59:36]

nit: is there a value to check if thread_data->worker_thread_number_ is not
greater than stack_.size()? thanks.

[jar (doing other things), 2011/11/18 01:06:42]

If that violation takes place, then I'll fall through to line 592 (and DCHECK). 
IF this happens in release mode, then I'd "gracefully" discard the argument,
rather than pushing it, and I'd leak.  I was trying to be extra careful to even
check for that case...  but most stack implementations wouldn't have bothered to
do so.  In addition, I don't think that *all* of the worker threads usually get
shut down, so there is almost always some extra space in the stack... and we
should be far away from it.

Is that convincing??

+    stack_[empty_slot_] = thread_data;
+    ++empty_slot_;
+    return;
+  }
+  NOTREACHED();
+}
+
+void ThreadData::ThreadDataPool::pop() {
+  if (empty_slot_ > 0) {
+    --empty_slot_;
+    return;
+  }
+  NOTREACHED();
+}
+
+//------------------------------------------------------------------------------
 // Individual 3-tuple of birth (place and thread) along with death thread, and
 // the accumulated stats for instances (DeathData).