[tracing] Add thread name to the pseudo stack of heap profiler

base/threading/thread_id_name_manager.cc

 // 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.
 
 #include "base/threading/thread_id_name_manager.h"
 
 #include <stdlib.h>
 #include <string.h>
 
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string_util.h"
+#include "base/trace_event/heap_profiler_allocation_context_tracker.h"
 
 namespace base {
 namespace {
 
 static const char kDefaultName[] = "";
 static std::string* g_default_name;
 
 }
 
 ThreadIdNameManager::ThreadIdNameManager()
@@ skipping 20 matching lines @@
 void ThreadIdNameManager::RegisterThread(PlatformThreadHandle::Handle handle,
                                          PlatformThreadId id) {
   AutoLock locked(lock_);
   thread_id_to_handle_[id] = handle;
   thread_handle_to_interned_name_[handle] =
       name_to_interned_name_[kDefaultName];
 }
 
 void ThreadIdNameManager::SetName(PlatformThreadId id,
                                   const std::string& name) {
-  AutoLock locked(lock_);
-  NameToInternedNameMap::iterator iter = name_to_interned_name_.find(name);
   std::string* leaked_str = NULL;
-  if (iter != name_to_interned_name_.end()) {
-    leaked_str = iter->second;
-  } else {
-    leaked_str = new std::string(name);
-    name_to_interned_name_[name] = leaked_str;
+  {
+    AutoLock locked(lock_);
+    NameToInternedNameMap::iterator iter = name_to_interned_name_.find(name);
+    if (iter != name_to_interned_name_.end()) {
+      leaked_str = iter->second;
+    } else {
+      leaked_str = new std::string(name);
+      name_to_interned_name_[name] = leaked_str;
+    }
+
+    ThreadIdToHandleMap::iterator 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 registered.
+    if (id_to_handle_iter == thread_id_to_handle_.end()) {
+      main_process_name_ = leaked_str;
+      main_process_id_ = id;
+      return;
+    }
+    thread_handle_to_interned_name_[id_to_handle_iter->second] = leaked_str;
   }
 
-  ThreadIdToHandleMap::iterator 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 registered.
-  if (id_to_handle_iter == thread_id_to_handle_.end()) {
-    main_process_name_ = leaked_str;
-    main_process_id_ = id;
-    return;
-  }
-  thread_handle_to_interned_name_[id_to_handle_iter->second] = leaked_str;
+  // Add the leaked thread name to heap profiler context tracker. The name added

[petrcermak, 2016/03/29 13:07:46]

Shouldn't you also do this for the main thread?

[ssid, 2016/03/29 18:19:11]

replied below.

+  // is valid for the lifetime of the process.
+  trace_event::AllocationContextTracker::SetCurrentThreadName(

[danakj, 2016/03/29 00:47:36]

Why not call GetName() when you need the name?

[ssid, 2016/03/29 18:19:11]

I cannot call GetName from AllocationTracker because it could cause a deadlock
at RegisterThread.
AllocationTracker gets initialized at first allocation in the thread (where I
should try to call GetName). The first allocation more often happens in
RegisterThread.
So, calling GetName tries to hold same lock as RegisterThread.

[danakj, 2016/03/29 20:12:41]

Sorry I'm not understanding this, but why is it a deadlock? If RegisterThread
and GetName happen at the same time, then one will wait for the other and then
continue. Deadlocks occur when 2 locks are held in different orders and one lock
ends up waiting on the other lock which waits on the first lock.

I only see 1 lock in your explanation tho, which would just provide single
access at a time?

It sounded like you're worried about RegisterThread and GetName happening from
the same thread at the same time? One does not call the other, right? Register
should happen before GetName for any given thread.

[Primiano Tucci (use gerrit), 2016/03/31 15:48:28]

Yeah that was my initial proposal, unfortunately won't work for this very
special case (more below).
I think what he means here is: deadlock by re-entrancy.
The magic bit here is that if he needs the name in the heap profiler code under
malloc hook. So if he tries to use GetName() he'd end up in the current
situation:

(some code) -> malloc -> TIDNameMgr::GetName().
Now, this would work most of the times, but won't work in the case where (some
code) == (some code in TIDNamemgr which has is holding the lock).
In other words, if the first malloc seen on one thread is due to some method
ThreadIdNameManager (which unfortunately seems to be the case), GetName would
re-entrantly try to re-acquire the lock. The lock is irrelevant to be honest,
the real problem is that he'd re-entrantly call within ThreadIdNameManager,
which should not happen even if it was lock-free, as re-entrant code is bad.

I think this is the reason why he's opting for a push-model.
I agree this is less nice from a layering viewpoint, but not sure is there any
other way around this (I'd love if there was a way to not touch base code
outside of trace_event).
Unless one calls malloc and the malloc hook calls GetName, which is the catch
here :)

+      leaked_str->c_str());
 }
 
 const char* ThreadIdNameManager::GetName(PlatformThreadId id) {
   AutoLock locked(lock_);
 
   if (id == main_process_id_)
     return main_process_name_->c_str();
 
   ThreadIdToHandleMap::iterator id_to_handle_iter =
       thread_id_to_handle_.find(id);
@@ skipping 19 matching lines @@
   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