[tracing] Move memory-infra dumps to dedicated thread

base/trace_event/memory_dump_manager.cc

 // Copyright 2015 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/trace_event/memory_dump_manager.h"
 
 #include <algorithm>
 
 #include "base/atomic_sequence_num.h"
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/threading/thread.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/memory_dump_session_state.h"
 #include "base/trace_event/memory_profiler_allocation_context.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "build/build_config.h"
 
 #if !defined(OS_NACL)
 #include "base/trace_event/process_memory_totals_dump_provider.h"
 #endif
@@ skipping 251 matching lines @@
 
 void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
                                           const MemoryDumpCallback& callback) {
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(kTraceCategory, "ProcessMemoryDump",
                                     TRACE_ID_MANGLE(args.dump_guid));
 
   scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
     pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
-        args, dump_providers_.begin(), session_state_, callback));
+        args, dump_providers_.begin(), session_state_, callback,
+        dump_thread_->task_runner()));
   }
 
   TRACE_EVENT_WITH_FLOW0(kTraceCategory, "MemoryDumpManager::CreateProcessDump",
                          TRACE_ID_MANGLE(args.dump_guid),
                          TRACE_EVENT_FLAG_FLOW_OUT);
 
   // Start the thread hop. |dump_providers_| are kept sorted by thread, so
   // ContinueAsyncProcessDump will hop at most once per thread (w.r.t. thread
   // affinity specified by the MemoryDumpProvider(s) in RegisterDumpProvider()).
   ContinueAsyncProcessDump(pmd_async_state.Pass());
@@ skipping 28 matching lines @@
   // DO NOT put any LOG() statement in the locked sections, as in some contexts
   // (GPU process) LOG() ends up performing PostTask/IPCs.
   MemoryDumpProvider* mdp;
   bool skip_dump = false;
   {
     AutoLock lock(lock_);
 
     auto mdp_info = pmd_async_state->next_dump_provider;
     mdp = mdp_info->dump_provider;
     dump_provider_name = mdp_info->name;
+
+    // If the dump provider did not specify a thread affinity, dump on the

[petrcermak, 2015/10/30 17:43:04]

nit: "dump_thread_" is a name so I'd drop the article (at the end of the line)

[Primiano Tucci (use gerrit), 2015/11/02 10:22:22]

Done.

+    // |dump_thread_|.
+    SingleThreadTaskRunner* task_runner = mdp_info->task_runner.get();
+    if (!task_runner)
+      task_runner = pmd_async_state->dump_thread_task_runner.get();
+
+    // The |dump_thread_| might have been Stop()-ed at this point (if tracing

[petrcermak, 2015/10/30 17:43:04]

ditto

[Primiano Tucci (use gerrit), 2015/11/02 10:22:22]

Done.

+    // was disabled in the meanwhile). In such case the PostTask() below will
+    // fail. The task_runner handle, however, should always be non-null.
+    DCHECK(task_runner);
+
     if (mdp_info->disabled || mdp_info->unregistered) {
       skip_dump = true;
-    } else if (mdp_info->task_runner &&
-               !mdp_info->task_runner->BelongsToCurrentThread()) {
+    } else if (!task_runner->BelongsToCurrentThread()) {
       // It's time to hop onto another thread.
 
       // Copy the callback + arguments just for the unlikley case in which
       // PostTask fails. In such case the Bind helper will destroy the
       // pmd_async_state and we must keep a copy of the fields to notify the
       // abort.
       MemoryDumpCallback callback = pmd_async_state->callback;
       scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
-          pmd_async_state->task_runner;
+          pmd_async_state->callback_task_runner;
 
-      const bool did_post_task = mdp_info->task_runner->PostTask(
+      const bool did_post_task = task_runner->PostTask(
           FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
                           Unretained(this), Passed(pmd_async_state.Pass())));
       if (did_post_task)
         return;
 
       // The thread is gone. At this point the best thing we can do is to
       // disable the dump provider and abort this dump.
       mdp_info->disabled = true;
       return AbortDumpLocked(callback, callback_task_runner, dump_guid);
     }
@@ skipping 41 matching lines @@
   if (finalize)
     return FinalizeDumpAndAddToTrace(pmd_async_state.Pass());
 
   ContinueAsyncProcessDump(pmd_async_state.Pass());
 }
 
 // static
 void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
   const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
-  if (!pmd_async_state->task_runner->BelongsToCurrentThread()) {
-    scoped_refptr<SingleThreadTaskRunner> task_runner =
-        pmd_async_state->task_runner;
-    task_runner->PostTask(FROM_HERE,
-                          Bind(&MemoryDumpManager::FinalizeDumpAndAddToTrace,
-                               Passed(pmd_async_state.Pass())));
+  if (!pmd_async_state->callback_task_runner->BelongsToCurrentThread()) {
+    scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
+        pmd_async_state->callback_task_runner;
+    callback_task_runner->PostTask(
+        FROM_HERE, Bind(&MemoryDumpManager::FinalizeDumpAndAddToTrace,
+                        Passed(pmd_async_state.Pass())));
     return;
   }
 
   TRACE_EVENT_WITH_FLOW0(kTraceCategory,
                          "MemoryDumpManager::FinalizeDumpAndAddToTrace",
                          TRACE_ID_MANGLE(dump_guid), TRACE_EVENT_FLAG_FLOW_IN);
 
   TracedValue* traced_value = new TracedValue();
   scoped_refptr<ConvertableToTraceFormat> event_value(traced_value);
   pmd_async_state->process_memory_dump.AsValueInto(traced_value);
@@ skipping 50 matching lines @@
   scoped_refptr<StackFrameDeduplicator> stack_frame_deduplicator = nullptr;
 
   if (heap_profiling_enabled_) {
     // If heap profiling is enabled, the stack frame deduplicator will be in
     // use. Add a metadata event to write its frames.
     stack_frame_deduplicator = new StackFrameDeduplicator;
     TRACE_EVENT_API_ADD_METADATA_EVENT("stackFrames", "stackFrames",
                                        stack_frame_deduplicator);
   }
 
+  dump_thread_.reset(new Thread("MemoryInfra"));
+  dump_thread_->Start();

[Ruud van Asseldonk, 2015/10/30 16:52:41]

Is |new Thread| or |dump_thread_->Start()| an expensive operation that spins up
a system thread? If so, can it be done outside of the lock?

[petrcermak, 2015/10/30 17:43:04]

It seems like that could create a race with OnTraceLogDisabled. How about
starting it first and then swapping it in (like Ruud suggested in
OnTraceLogDisabled).

[Primiano Tucci (use gerrit), 2015/11/02 10:22:23]

Makes sense, moved to a temporary ref_ptr before and doing only the swap in the
lock

+
   session_state_ = new MemoryDumpSessionState(stack_frame_deduplicator);
 
   for (auto it = dump_providers_.begin(); it != dump_providers_.end(); ++it) {
     it->disabled = false;
     it->consecutive_failures = 0;
   }
 
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 
   // TODO(primiano): This is a temporary hack to disable periodic memory dumps
@@ skipping 29 matching lines @@
   }
   DCHECK_EQ(0u, heavy_dump_period_ms % min_timer_period_ms);
   g_heavy_dumps_rate = heavy_dump_period_ms / min_timer_period_ms;
 
   periodic_dump_timer_.Start(FROM_HERE,
                              TimeDelta::FromMilliseconds(min_timer_period_ms),
                              base::Bind(&RequestPeriodicGlobalDump));
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
+  subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
+
+  // Thread stops are blocking and must be performed outside of the |lock_|
+  // or will deadlock (e.g., if ContinueAsyncProcessDump() tries to acquire it).
+  periodic_dump_timer_.Stop();
+  if (dump_thread_)

[Ruud van Asseldonk, 2015/10/30 16:52:41]

So |dump_thread_| is set inside the lock everywhere but read outside of it here.
Instead you can do

    scoped_refptr<SingleThreadTaskRunner> dump_thread = nullptr;
    {
      AutoLock lock(lock_);
      dump_thread_.swap(dump_thread);
      // stuff like before
    }
    if (dump_thread)
      dump_thread.Stop();

[petrcermak, 2015/10/30 17:43:04]

I agree that would be better. It's strange (and possibly dangerous) that you
suddenly access it unlocked here.

[Primiano Tucci (use gerrit), 2015/11/02 10:22:23]

So, tecnically is fine as it is accessed only in OnTraceLogEnabled/Disabled,
which are guaranteed to be serialized (locked) from the TraceLog.

But I agree that I shouldn't rely on this , so made the change as proposed, I
like it.

+    dump_thread_->Stop();
+
   AutoLock lock(lock_);
-  periodic_dump_timer_.Stop();
-  subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
+  dump_thread_.reset();
   session_state_ = nullptr;
 }
 
 uint64_t MemoryDumpManager::GetTracingProcessId() const {
   return delegate_->GetTracingProcessId();
 }
 
 MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
     MemoryDumpProvider* dump_provider,
     const char* name,
     const scoped_refptr<SingleThreadTaskRunner>& task_runner)
     : dump_provider(dump_provider),
       name(name),
       task_runner(task_runner),
       consecutive_failures(0),
       disabled(false),
       unregistered(false) {}
 
 MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {}
 
 bool MemoryDumpManager::MemoryDumpProviderInfo::operator<(
     const MemoryDumpProviderInfo& other) const {
   if (task_runner == other.task_runner)
     return dump_provider < other.dump_provider;
-  return task_runner < other.task_runner;
+  // Ensure that unbound providers (task_runner == nullptr) run always last.

[petrcermak, 2015/10/30 17:43:04]

supernit: "always run" sounds better than "run always" in my opinion

[Primiano Tucci (use gerrit), 2015/11/02 10:22:23]

Done.

+  return !(task_runner < other.task_runner);

[Ruud van Asseldonk, 2015/10/30 16:52:41]

You replaced < with >= here but we already know that the task runners are not
equal due to the conditional above, so I think just writing > here is more
readable.

[Primiano Tucci (use gerrit), 2015/11/02 10:22:23]

The reason why I did that is that operator< is already defined for TaskRunner.
If I want to write >, I have to define a > operator, too much extra boilerplate
:)

[Ruud van Asseldonk, 2015/11/02 11:00:37]

I see. You could call |.get()| to work around.

 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     MemoryDumpRequestArgs req_args,
     MemoryDumpProviderInfoSet::iterator next_dump_provider,
     const scoped_refptr<MemoryDumpSessionState>& session_state,
-    MemoryDumpCallback callback)
+    MemoryDumpCallback callback,
+    const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner)
     : process_memory_dump(session_state),
       req_args(req_args),
       next_dump_provider(next_dump_provider),
       callback(callback),
-      task_runner(MessageLoop::current()->task_runner()) {}
+      callback_task_runner(MessageLoop::current()->task_runner()),
+      dump_thread_task_runner(dump_thread_task_runner) {}
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
 }
 
 }  // namespace trace_event
 }  // namespace base