[tracing] Move memory-infra dumps to dedicated thread

base/trace_event/memory_dump_manager.cc

Index: base/trace_event/memory_dump_manager.cc
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index d939eea3cef93ec13f76f2cbd59b31f90a7c8231..133ee13efd777e7b7eb01c69283dd963b1d39646 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -11,6 +11,7 @@
 #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"
@@ -282,7 +283,8 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
   {
     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",
@@ -331,10 +333,21 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
     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
@@ -343,9 +356,9 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
       // 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)
@@ -407,12 +420,12 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
 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;
   }
 
@@ -483,6 +496,9 @@ void MemoryDumpManager::OnTraceLogEnabled() {
                                        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) {
@@ -532,9 +548,16 @@ void MemoryDumpManager::OnTraceLogEnabled() {
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
-  AutoLock lock(lock_);
-  periodic_dump_timer_.Stop();
   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_);
+  dump_thread_.reset();
   session_state_ = nullptr;
 }
 
@@ -559,19 +582,22 @@ 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() {
 }