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

content/browser/tracing/tracing_controller_impl.cc

Index: content/browser/tracing/tracing_controller_impl.cc
diff --git a/content/browser/tracing/tracing_controller_impl.cc b/content/browser/tracing/tracing_controller_impl.cc
index 8380c45c44cd2924301ebd3b7e34f803daf23480..48ed1ad6115998f0ba80d84cc95dabb4fdc2d6e4 100644
--- a/content/browser/tracing/tracing_controller_impl.cc
+++ b/content/browser/tracing/tracing_controller_impl.cc
@@ -920,16 +920,21 @@ void TracingControllerImpl::RequestGlobalMemoryDump(
                    base::Unretained(this), args, callback));
     return;
   }
-  // Abort if another dump is already in progress.
+
+  // Enqueue request if another dump is already in progress.
   if (pending_memory_dump_guid_) {

[Primiano Tucci (use gerrit), 2016/06/16 08:37:23]

I think here I'd just if (!queued_memory_dump_requests.empty())

so there is only one state (the queue) to care about

[petrcermak, 2016/06/16 12:08:29]

Done.

-    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 */);
+    enqueued_memory_dump_requests_.emplace(args, callback);

[ssid, 2016/06/15 17:20:51]

For the case of only periodic dumps, if a heavy dump gets stalled for 2 seconds,
then we will accumulate 7 light dumps on the queue. This would start triggering
7 light dumps in a row and then come to the heavy dumps. If the page was loading
for 10 seconds, then we would see doing too many queued memory dumps at the end
which are not really useful to see.
Currently we would start making periodic dumps once the page loads finishes.

Is there a reason we should queue more than one dump?
Is there a reason to queue a light dump followed by a heavy dump?

I think
1. We should queue memory dumps only when it is explicitly_triggered 
2. This queue should have a maximum size of 1.
3. The logic could be if more request comes by, and if the request is heavier
than the current dump only then queue it. I mean: if we are currently doing a
heavy dump and a light dump request comes, there is no need to queue it since we
already have those details in the current dump. But if we are doing a heavy
dump, and a new heavy dump is requested then it should be queued because V8 gc
might have collected different set of stats after the dump provider was called.

essentially what i am trying to say is the code should look like:
if (pending_memory_dump_guid_ && current_level_of_detail <
requested_level_of_detail) {
  if (enqueued_memory_dump_request_ &&
enqueued_memory_dump_request_.level_of_detail < requested_level_of_detail) {
    enqueued_memory_dump_request_  = new_request;
  }
}

This also solves the current problem of heavy dumps going missing if the light
dump gets stalled. ie, if the 7th light dump takes too long, the heavy dump
might go missing because it gets ignored. This would ensure that the heavy dump
surely happens and the light dumps are unnecessary to see there.

[Primiano Tucci (use gerrit), 2016/06/16 08:37:23]

So, I agree with ssid that this can cause some storms if too many light dumps
get queued becuase the message loops are busy. However, I'd simplify A bit the
proposal.
What I'd do personally is: 
always enqueue the dump request, unless: the request is of type
PERIODIC_INTERVAL and the level of detail matches the top of the queue. (in
essence, deduplicate and don't queue identical periodic requests)
WDYT?

[petrcermak, 2016/06/16 12:08:31]

I think that the following would be the best approach: If the following
conditions both hold when requesting a dump, there's no point in queueing the
request:

  1. The request is for a periodic dump.
  2. There is another dump in the queue (potentially being performed at the
moment) which has the same level of detail.

[ssid, 2016/06/16 23:12:03]

If there is a sequence of events like this when a DETAILED dump is in progress:
1. request: periodic LIGHT dump
2. request: periodic DETAILED dump
3. request: explicit DETAILED dump
4. request: explicit DETAILED dump

Is there any use of making a LIGHT dump before actually making a DETAILED dump
when the DETAILED dump would anyway include all the details needed for light
dump?

Is there any reason to still perform a DETAILED dump after previous DETAILED
dump, and also multiple times when explicitly called?

If the only reason to dump multiple times is for running the success callback,
maybe we only need a queue for callbacks to run. And just a single dump in the
Queue (the most detailed one requested).
We could call the light dump request a success if the heavy dump was successful.

[petrcermak, 2016/06/17 08:36:44]

I'm fine with that, BUT that would require re-architecting other parts of the
pipeline. The problem is that the TracingControllerImpl is already given a GUID
for the dump it's supposed to create. Therefore, you can't call multiple
callbacks (after a single dump succeeds) and tell them they were all successful
because each of them is expecting a different GUID.

Another thing to think about: Sometimes you want a less detailed dump instead of
a more detailed one for reasons other than performance. For example, when you
request a BACKGROUND dump, you definitely do NOT want it to be a LIGHT or
DETAILED dump.

Either way, I think that this is something that we can address later. We don't
need to have a perfect solution straightaway.

[ssid, 2016/06/17 18:43:45]

Ah I see, then I am fine with this, if that is going to be a bigger change. I
was thinking it is a simple check. After all it these are rare cases anyway.
Agreed.

     return;
   }
 
+  PerformGlobalMemoryDump(args, callback);
+}
+
+void TracingControllerImpl::PerformGlobalMemoryDump(
+    const base::trace_event::MemoryDumpRequestArgs& args,

[Primiano Tucci (use gerrit), 2016/06/16 08:37:23]

Does this require any arg? Aren't this the front() of the queue?

[petrcermak, 2016/06/16 12:08:29]

They were not, but I changed it, so done.

+    const base::trace_event::MemoryDumpCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
   // Count myself (local trace) in pending_memory_dump_ack_count_, acked by
   // OnBrowserProcessMemoryDumpDone().
   pending_memory_dump_ack_count_ = trace_message_filters_.size() + 1;
@@ -952,6 +957,13 @@ void TracingControllerImpl::RequestGlobalMemoryDump(
     tmf->SendProcessMemoryDumpRequest(args);
 }
 
+TracingControllerImpl::GlobalMemoryDumpRequest::GlobalMemoryDumpRequest(
+    const base::trace_event::MemoryDumpRequestArgs& args,
+    const base::trace_event::MemoryDumpCallback& callback)
+    : args(args), callback(callback) {}
+
+TracingControllerImpl::GlobalMemoryDumpRequest::~GlobalMemoryDumpRequest() {}
+
 uint64_t TracingControllerImpl::GetTracingProcessId() const {
   return ChildProcessHost::kBrowserTracingProcessId;
 }
@@ -1033,6 +1045,14 @@ void TracingControllerImpl::FinalizeGlobalMemoryDumpIfAllProcessesReplied() {
     pending_memory_dump_callback_.Reset();
   }
   pending_memory_dump_guid_ = 0;
+
+  // Immediately perform another dump if it's been enqueued.
+  if (!enqueued_memory_dump_requests_.empty()) {

[petrcermak, 2016/06/15 16:19:11]

Maybe it would be better to post a task instead?

[Primiano Tucci (use gerrit), 2016/06/16 08:37:23]

Yeah that will avoid thinking about weird re-entrancy in the rare cases where
you have no child processes.

[petrcermak, 2016/06/16 12:08:31]

Done.

+    const auto args = enqueued_memory_dump_requests_.front().args;
+    const auto callback = enqueued_memory_dump_requests_.front().callback;
+    enqueued_memory_dump_requests_.pop();
+    PerformGlobalMemoryDump(args, callback);
+  }
 }
 
 }  // namespace content