Avoid any possibility of an Alloc during TLS thread teardown

base/tracked_objects.cc

 // Copyright (c) 2011 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.
 
 #include "base/tracked_objects.h"
 
 #include <math.h>
 
 #include "base/format_macros.h"
 #include "base/message_loop.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
+#include "base/port.h"
 
 using base::TimeDelta;
 
 namespace tracked_objects {
 
 namespace {
 // Flag to compile out almost all of the task tracking code.
 static const bool kTrackAllTaskObjects = true;
 
 // When ThreadData is first initialized, should we start in an ACTIVE state to
@@ skipping 116 matching lines @@
 base::LazyInstance<base::Lock,
                    base::LeakyLazyInstanceTraits<base::Lock> >
     ThreadData::list_lock_ = LAZY_INSTANCE_INITIALIZER;
 
 // static
 ThreadData::Status ThreadData::status_ = ThreadData::UNINITIALIZED;
 
 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_.
 }
 
 ThreadData::~ThreadData() {}
 
 void ThreadData::PushToHeadOfList() {
   DCHECK(!next_);
   base::AutoLock lock(*list_lock_.Pointer());
   incarnation_count_for_pool_ = incarnation_counter_;
   next_ = all_thread_data_list_head_;
@@ skipping 15 matching lines @@
 // static
 ThreadData* ThreadData::Get() {
   if (!tls_index_.initialized())
     return NULL;  // For unittests only.
   ThreadData* registered = reinterpret_cast<ThreadData*>(tls_index_.Get());
   if (registered)
     return registered;
 
   // 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;
 }
 
 // static
 void ThreadData::OnThreadTermination(void* thread_data) {
   if (!kTrackAllTaskObjects)
     return;  // Not compiled in.
   if (!thread_data)
     return;
   reinterpret_cast<ThreadData*>(thread_data)->OnThreadTerminationCleanup();
 }
 
 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:

 }
 
 // static
 base::DictionaryValue* ThreadData::ToValue() {
   DataCollector collected_data;  // Gather data.
   collected_data.AddListOfLivingObjects();  // Add births that are still alive.
   base::ListValue* list = collected_data.ToValue();
   base::DictionaryValue* dictionary = new base::DictionaryValue();
   dictionary->Set("list", list);
   return dictionary;
@@ skipping 303 matching lines @@
     for (BirthMap::iterator it = next_thread_data->birth_map_.begin();
          next_thread_data->birth_map_.end() != it; ++it)
       delete it->second;  // Delete the Birth Records.
     next_thread_data->birth_map_.clear();
     next_thread_data->death_map_.clear();
     delete next_thread_data;  // Includes all Death Records.
   }
 }
 
 //------------------------------------------------------------------------------
+// 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).
 
 Snapshot::Snapshot(const BirthOnThread& birth_on_thread,
                    const ThreadData& death_thread,
                    const DeathData& death_data)
     : birth_(&birth_on_thread),
       death_thread_(&death_thread),
       death_data_(death_data) {
 }
@@ skipping 80 matching lines @@
 
 base::ListValue* DataCollector::ToValue() const {
   base::ListValue* list = new base::ListValue;
   for (size_t i = 0; i < collection_.size(); ++i) {
     list->Append(collection_[i].ToValue());
   }
   return list;
 }
 
 }  // namespace tracked_objects