[memory-infra] Add support for queueing memory dump requests in the browser process

content/browser/tracing/tracing_controller_impl.cc

 // Copyright (c) 2013 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 "content/browser/tracing/tracing_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/cpu.h"
 #include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/json/string_escape.h"
@@ skipping 543 matching lines @@
     if (it != pending_trace_log_status_filters_.end()) {
       BrowserThread::PostTask(
           BrowserThread::UI, FROM_HERE,
           base::Bind(&TracingControllerImpl::OnTraceLogStatusReply,
                      base::Unretained(this),
                      base::RetainedRef(trace_message_filter),
                      base::trace_event::TraceLogStatus()));
     }
   }
   if (pending_memory_dump_ack_count_ > 0) {
+    DCHECK(!queued_memory_dump_requests_.empty());
     TraceMessageFilterSet::const_iterator it =
         pending_memory_dump_filters_.find(trace_message_filter);
     if (it != pending_memory_dump_filters_.end()) {
       BrowserThread::PostTask(
           BrowserThread::UI, FROM_HERE,
           base::Bind(&TracingControllerImpl::OnProcessMemoryDumpResponse,
                      base::Unretained(this),
                      base::RetainedRef(trace_message_filter),
-                     pending_memory_dump_guid_, false /* success */));
+                     queued_memory_dump_requests_.front().args.dump_guid,
+                     false /* success */));
     }
   }
   trace_message_filters_.erase(trace_message_filter);
 }
 
 void TracingControllerImpl::AddTracingAgent(const std::string& agent_name) {
 #if defined(OS_CHROMEOS)
   auto debug_daemon =
       chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
   if (agent_name == debug_daemon->GetTracingAgentName()) {
@@ skipping 330 matching lines @@
 void TracingControllerImpl::RequestGlobalMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const base::trace_event::MemoryDumpCallback& callback) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&TracingControllerImpl::RequestGlobalMemoryDump,
                    base::Unretained(this), args, callback));
     return;
   }
-  // Abort if another dump is already in progress.
-  if (pending_memory_dump_guid_) {
-    VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix
-            << " (" << args.dump_guid << ") aborted because another dump ("
-            << pending_memory_dump_guid_ << ") is in progress";
-    if (!callback.is_null())
-      callback.Run(args.dump_guid, false /* success */);
+
+  bool another_dump_already_in_progress = !queued_memory_dump_requests_.empty();

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

I really love this perfectly touching the 80th col without wrapping :)

[petrcermak, 2016/06/24 10:49:57]

Ack :-)

+
+  // If this is a periodic memory dump request without a callback and there

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

remove the "without a callback" part. It is not true and if it was true would be
wrong. I think realistically most of them have a callback

[petrcermak, 2016/06/24 10:49:57]

Done.

+  // already is another request in the queue with the same level of detail,
+  // there's no point in enqueuing this request.
+  if (another_dump_already_in_progress) {

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

why not if (another... && dump_type == ). Would reduce the indentation level of
the code below and increase its readability.

[petrcermak, 2016/06/24 10:49:57]

Done.

+    if (args.dump_type ==
+        base::trace_event::MemoryDumpType::PERIODIC_INTERVAL) {
+      const auto requested_level_of_detail = args.level_of_detail;

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

not sure this variable buys you anything really

[petrcermak, 2016/06/24 10:49:57]

I removed it.

+      for (const auto& request : queued_memory_dump_requests_) {
+        if (request.args.level_of_detail == requested_level_of_detail) {
+          VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix << " ("
+                  << base::trace_event::MemoryDumpTypeToString(args.dump_type)
+                  << ") skipped because another dump request with the same "
+                     "level of detail ("
+                  << base::trace_event::MemoryDumpLevelOfDetailToString(
+                         requested_level_of_detail)
+                  << ") is already in the queue";
+          if (!callback.is_null())
+            callback.Run(args.dump_guid, false /* success */);
+          return;
+        }
+      }
+    }
+  }
+
+  queued_memory_dump_requests_.emplace_back(args, callback);
+
+  // If another dump is already in progress, this dump will automatically be
+  // scheduled when the other dump finishes.
+  if (another_dump_already_in_progress)
     return;
-  }
+
+  PerformGlobalMemoryDump();
+}
+
+void TracingControllerImpl::PerformGlobalMemoryDump() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!queued_memory_dump_requests_.empty());
+  const base::trace_event::MemoryDumpRequestArgs& args =
+      queued_memory_dump_requests_.front().args;
 
   // Count myself (local trace) in pending_memory_dump_ack_count_, acked by
   // OnBrowserProcessMemoryDumpDone().
   pending_memory_dump_ack_count_ = trace_message_filters_.size() + 1;
   pending_memory_dump_filters_.clear();
-  pending_memory_dump_guid_ = args.dump_guid;
-  pending_memory_dump_callback_ = callback;
   failed_memory_dump_count_ = 0;
 
   MemoryDumpManagerDelegate::CreateProcessDump(
       args, base::Bind(&TracingControllerImpl::OnBrowserProcessMemoryDumpDone,
                        base::Unretained(this)));
 
   // If there are no child processes we are just done.
   if (pending_memory_dump_ack_count_ == 1)
     return;
 
   pending_memory_dump_filters_ = trace_message_filters_;
 
   for (const scoped_refptr<TraceMessageFilter>& tmf : trace_message_filters_)
     tmf->SendProcessMemoryDumpRequest(args);
 }
 
+TracingControllerImpl::QueuedMemoryDumpRequest::QueuedMemoryDumpRequest(
+    const base::trace_event::MemoryDumpRequestArgs& args,
+    const base::trace_event::MemoryDumpCallback& callback)
+    : args(args), callback(callback) {}
+
+TracingControllerImpl::QueuedMemoryDumpRequest::~QueuedMemoryDumpRequest() {}
+
 uint64_t TracingControllerImpl::GetTracingProcessId() const {
   return ChildProcessHost::kBrowserTracingProcessId;
 }
 
 void TracingControllerImpl::AddTraceMessageFilterObserver(
     TraceMessageFilterObserver* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   trace_message_filter_observers_.AddObserver(observer);
 
   for (auto& filter : trace_message_filters_)
@@ skipping 19 matching lines @@
         base::Bind(&TracingControllerImpl::OnProcessMemoryDumpResponse,
                    base::Unretained(this),
                    base::RetainedRef(trace_message_filter), dump_guid,
                    success));
     return;
   }
 
   TraceMessageFilterSet::iterator it =
       pending_memory_dump_filters_.find(trace_message_filter);
 
-  if (pending_memory_dump_guid_ != dump_guid ||
+  DCHECK(!queued_memory_dump_requests_.empty());
+  if (queued_memory_dump_requests_.front().args.dump_guid != dump_guid ||
       it == pending_memory_dump_filters_.end()) {
     DLOG(WARNING) << "Received unexpected memory dump response: " << dump_guid;
     return;
   }
 
   DCHECK_GT(pending_memory_dump_ack_count_, 0);
   --pending_memory_dump_ack_count_;
   pending_memory_dump_filters_.erase(it);
   if (!success) {
     ++failed_memory_dump_count_;
@@ skipping 13 matching lines @@
     VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix
             << " aborted on the current process";
   }
   FinalizeGlobalMemoryDumpIfAllProcessesReplied();
 }
 
 void TracingControllerImpl::FinalizeGlobalMemoryDumpIfAllProcessesReplied() {
   if (pending_memory_dump_ack_count_ > 0)
     return;
 
-  DCHECK_NE(0u, pending_memory_dump_guid_);
-  const bool global_success = failed_memory_dump_count_ == 0;
-  if (!pending_memory_dump_callback_.is_null()) {
-    pending_memory_dump_callback_.Run(pending_memory_dump_guid_,
-                                      global_success);
-    pending_memory_dump_callback_.Reset();
+  DCHECK(!queued_memory_dump_requests_.empty());
+  {

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

why this extra scope, seems unnecessary?

[petrcermak, 2016/06/24 10:49:57]

It's because |callback| is a reference to the front of the queue, which is
popped on line 1076. If there's no scope, someone could incorrectly use
|callback| after it's popped.

[Primiano Tucci (use gerrit), 2016/06/24 11:23:05]

ahh right. smart.

+    const auto& callback = queued_memory_dump_requests_.front().callback;
+    if (!callback.is_null()) {
+      const bool global_success = failed_memory_dump_count_ == 0;
+      callback.Run(queued_memory_dump_requests_.front().args.dump_guid,
+                   global_success);
+    }
   }
-  pending_memory_dump_guid_ = 0;
+  queued_memory_dump_requests_.pop_front();
+
+  // Schedule the next queued dump (if applicable).
+  if (!queued_memory_dump_requests_.empty()) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&TracingControllerImpl::PerformGlobalMemoryDump,
+                   base::Unretained(this)));
+  }
 }
 
 }  // namespace content