TraceEvent fixes for thread name changes

base/debug/trace_event.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/debug/trace_event.h"
 
 #include <algorithm>
 
 #if defined(OS_WIN)
 #include "base/debug/trace_event_win.h"
 #endif
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/lazy_instance.h"
+#include "base/memory/singleton.h"
 #include "base/process_util.h"
 #include "base/stringprintf.h"
+#include "base/threading/platform_thread.h"
 #include "base/threading/thread_local.h"
 #include "base/utf_string_conversions.h"
 #include "base/stl_util.h"
 #include "base/time.h"
 
 class DeleteTraceLogForTesting {
  public:
   static void Delete() {
     Singleton<base::debug::TraceLog,
               StaticMemorySingletonTraits<base::debug::TraceLog> >::OnExit(0);
@@ skipping 20 matching lines @@
     false }
 };
 const TraceCategory* const g_category_already_shutdown =
     &g_categories[0];
 const TraceCategory* const g_category_categories_exhausted =
     &g_categories[1];
 const TraceCategory* const g_category_metadata =
     &g_categories[2];
 int g_category_index = 3; // skip initial 3 categories
 
-// Flag to indicate whether we captured the current thread name
-LazyInstance<ThreadLocalBoolean,
-             LeakyLazyInstanceTraits<ThreadLocalBoolean> >
-    g_current_thread_name_captured(LINKER_INITIALIZED);
+// The most-recently captured name of the current thread
+LazyInstance<ThreadLocalPointer<char>,
+             LeakyLazyInstanceTraits<ThreadLocalPointer<char> > >
+    g_current_thread_name(LINKER_INITIALIZED);
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 //
 // TraceValue
 //
 ////////////////////////////////////////////////////////////////////////////////
 
 void TraceValue::AppendAsJSON(std::string* out) const {
@@ skipping 435 matching lines @@
   int ret_begin_id = -1;
   {
     AutoLock lock(lock_);
     if (!category->enabled)
       return -1;
     if (logged_events_.size() >= kTraceEventBufferSize)
       return -1;
 
     PlatformThreadId thread_id = PlatformThread::CurrentId();
 
-    // Record the name of the calling thread, if not done already.
-    if (!g_current_thread_name_captured.Pointer()->Get()) {
-      g_current_thread_name_captured.Pointer()->Set(true);
-      const char* cur_name = PlatformThread::GetName();
+    const char* cur_name = PlatformThread::GetName();
+    if (cur_name && *cur_name &&

[jbates, 2011/11/08 18:44:38]

Since the steady state is (cur_name == g_current_thread_name.Get().Get()), it
looks like we can avoid two branches here by changing this to:
if (cur_name != g_current_thread_name.Get().Get() &&
    cur_name && *cur_name) {

[nduca, 2011/11/09 04:02:24]

If we do that, we might want to explain the logic, or use some temp var names.
Its a bit hard to read that way.

[joth, 2011/11/09 13:17:58]

Done.

[joth, 2011/11/09 13:17:58]

Done.

+        cur_name != g_current_thread_name.Get().Get()) {
+      // Benign const cast.
+      g_current_thread_name.Get().Set(const_cast<char*>(cur_name));
       base::hash_map<PlatformThreadId, std::string>::iterator existing_name =
           thread_names_.find(thread_id);
       if (existing_name == thread_names_.end()) {
         // This is a new thread id, and a new name.
-        thread_names_[thread_id] = cur_name ? cur_name : "";
-      } else if(cur_name != NULL) {
+        thread_names_[thread_id] = cur_name;
+      } else {
         // This is a thread id that we've seen before, but potentially with a
         // new name.
-        std::vector<std::string> existing_names;
+        std::vector<base::StringPiece> existing_names;
         Tokenize(existing_name->second, std::string(","), &existing_names);
         bool found = std::find(existing_names.begin(),
                                existing_names.end(),
                                cur_name) != existing_names.end();
         if (!found) {
-          existing_names.push_back(cur_name);
-          thread_names_[thread_id] =
-              JoinString(existing_names, ',');
+          existing_name->second.push_back(',');
+          existing_name->second.append(cur_name);
         }
       }
     }
 
     if (threshold_begin_id > -1) {
       DCHECK(phase == base::debug::TRACE_EVENT_PHASE_END);
       size_t begin_i = static_cast<size_t>(threshold_begin_id);
       // Return now if there has been a flush since the begin event was posted.
       if (begin_i >= logged_events_.size())
         return -1;
@@ skipping 120 matching lines @@
         NULL, 0, NULL, 0,
         p_data_->threshold_begin_id, p_data_->threshold,
         TraceLog::EVENT_FLAG_NONE);
   }
 }
 
 }  // namespace internal
 
 }  // namespace debug
 }  // namespace base