Move Thread Name Mapping into ThreadFunc.

base/threading/thread_id_name_manager.cc

Index: base/threading/thread_id_name_manager.cc
diff --git a/base/threading/thread_id_name_manager.cc b/base/threading/thread_id_name_manager.cc
index 5351ba714c3fa82340496465ef3efa0850e4da48..2d0588d0097647ac14a8d8658cb5c96361a6f20e 100644
--- a/base/threading/thread_id_name_manager.cc
+++ b/base/threading/thread_id_name_manager.cc
@@ -17,8 +17,10 @@ namespace {
 static const char kDefaultName[] = "";
 static std::string* g_default_name;
 
-typedef std::map<PlatformThreadId, std::string*>::iterator
-    ThreadIdToInternedNameIterator;
+typedef std::map<PlatformThreadId, PlatformThreadHandle*>::iterator

[jar (doing other things), 2013/05/22 19:37:56]

nit: My preference is to typedef the actual map type, probably as a private
typedef in the header file, and then use that to both declare the slots, and use
it to spec an iterator (and not bother to have a typedef for the iterator.

[dsinclair, 2013/05/22 20:22:31]

Done.

+    ThreadIdToHandleIterator;
+typedef std::map<PlatformThreadHandle*, std::string*>::iterator
+    ThreadHandleToInternedNameIterator;
 typedef std::map<std::string, std::string*>::iterator
     NameToInternedNameIterator;
 }
@@ -42,6 +44,12 @@ const char* ThreadIdNameManager::GetDefaultInternedString() {
   return g_default_name->c_str();
 }
 
+void ThreadIdNameManager::RegisterThread(PlatformThreadHandle* handle,
+                                         PlatformThreadId id) {
+  AutoLock locked(lock_);
+  thread_id_to_handle_[id] = handle;
+}
+
 void ThreadIdNameManager::SetName(PlatformThreadId id, const char* name) {
   std::string str_name(name);
 
@@ -54,26 +62,59 @@ void ThreadIdNameManager::SetName(PlatformThreadId id, const char* name) {
     leaked_str = new std::string(str_name);
     name_to_interned_name_[str_name] = leaked_str;
   }
-  thread_id_to_interned_name_[id] = leaked_str;
+
+  ThreadIdToHandleIterator id_to_handle_iter = thread_id_to_handle_.find(id);
+
+  // The main thread of a process will not be created as a Thread object which
+  // means there is no PlatformThreadHandler registerd.
+  if (id_to_handle_iter == thread_id_to_handle_.end()) {
+    main_process_name_ = leaked_str;
+    main_process_id_ = id;

[jar (doing other things), 2013/05/22 19:37:56]

Assuming there are no misunderstanding (and races), we should only set this
value exactly once, correct?  Can we CHECK() on this?

Should we also add this id to the thread_id_to_handle_ map?  ...and to the
handle_to_interned_name_ map?

[dsinclair, 2013/05/22 20:22:31]

It could be set multiple times. I think in practice we only call
PlatformThread::SetName once, but there is nothing that stops us from calling it
over and over again.

I can't put the main process information into the maps because I don't have a
PlatformThreadHandle for the main process. The two maps both use the Handle
(thread_id -> handle, handle -> name).

+    return;
+  }
+  thread_handle_to_interned_name_[id_to_handle_iter->second] = leaked_str;
 }
 
 const char* ThreadIdNameManager::GetName(PlatformThreadId id) {
   AutoLock locked(lock_);
-  ThreadIdToInternedNameIterator iter = thread_id_to_interned_name_.find(id);
-  // A platform thread may not have a name set, so return blank.
-  if (iter == thread_id_to_interned_name_.end())
+
+  if (id == main_process_id_)
+    return main_process_name_->c_str();

[jar (doing other things), 2013/05/22 19:37:56]

If we had put it in the map.... we could just use the general code.  Was there a
reason not to?

[dsinclair, 2013/05/22 20:22:31]

I can't put it into the map without a PlatformThreadHandle, as far as I can
tell, there is no handle for the main thread?

+
+  ThreadIdToHandleIterator id_to_handle_iter =
+      thread_id_to_handle_.find(id);
+  if (id_to_handle_iter == thread_id_to_handle_.end())
+    return name_to_interned_name_[kDefaultName]->c_str();
+
+  ThreadHandleToInternedNameIterator handle_to_name_iter =
+      thread_handle_to_interned_name_.find(id_to_handle_iter->second);
+  if (handle_to_name_iter == thread_handle_to_interned_name_.end())
     return name_to_interned_name_[kDefaultName]->c_str();
-  return iter->second->c_str();
+
+  return handle_to_name_iter->second->c_str();
 }
 
-void ThreadIdNameManager::RemoveName(PlatformThreadId id) {
+void ThreadIdNameManager::RemoveName(PlatformThreadHandle* handle,
+                                     PlatformThreadId id) {
+  if (*handle == base::kNullThreadHandle)

[jar (doing other things), 2013/05/22 21:36:48]

I see you deleted this (presumably because it can never happen).  What are the
cases where line 101 fires?

[dsinclair, 2013/05/23 18:21:53]

I dropped it originally because base::kNullThreadHandle doesn't exist anymore.

I've updated the RegisterThread code so that we always provide kDefaultName into
the handle to name map. This removes the need to special case not finding it in
GetName and keeps the remove code simpler.

+    return;
   if (id == kInvalidThreadId)
     return;
 
   AutoLock locked(lock_);
-  ThreadIdToInternedNameIterator iter = thread_id_to_interned_name_.find(id);
-  DCHECK((iter != thread_id_to_interned_name_.end()));
-  thread_id_to_interned_name_.erase(iter);
+  ThreadHandleToInternedNameIterator handle_to_name_iter =
+      thread_handle_to_interned_name_.find(handle);
+  DCHECK(handle_to_name_iter != thread_handle_to_interned_name_.end());
+  thread_handle_to_interned_name_.erase(handle_to_name_iter);
+
+  ThreadIdToHandleIterator id_to_handle_iter = thread_id_to_handle_.find(id);
+  DCHECK((id_to_handle_iter!= thread_id_to_handle_.end()));
+  // The given |id| may have been re-used by the system. Make sure the
+  // mapping points to the provided |handle| before removal.
+  if (id_to_handle_iter->second != handle)
+    return;
+
+  thread_id_to_handle_.erase(id_to_handle_iter);
 }
 
 }  // namespace base