[memory-infra] Add support for polling memory totals in MemoryDumpManager

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 5e34147063662d016ddb8ef003eb21928801a03d..b98cf3df545591bb1c10d9200c1364f3ba492526 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -284,6 +284,12 @@ void MemoryDumpManager::RegisterDumpProviderInternal(
     // path for RenderThreadImpl::Init().
     if (already_registered)
       return;
+
+    if (options.is_fast_polling_supported && dump_thread_) {

[Primiano Tucci (use gerrit), 2016/12/15 16:45:01]

Can you add a comment here saying:
// the list of polling MDPs is populated OnTraceLogEnabled(). This code deals
with the case of a MDP capable of fast polling that is registered after the
OnTraceLogEnabled().

[ssid, 2016/12/16 02:31:54]

Done.

+      dump_thread_->task_runner()->PostTask(
+          FROM_HERE, Bind(&MemoryDumpManager::RegisterPollingMDPOnDumpThread,
+                          Unretained(this), mdpinfo));
+    }
   }
 
   if (heap_profiling_enabled_)
@@ -322,6 +328,7 @@ void MemoryDumpManager::UnregisterDumpProviderInternal(
     // - At the end of this function, if no dump is in progress.
     // - Either in SetupNextMemoryDump() or InvokeOnMemoryDump() when MDPInfo is
     //   removed from |pending_dump_providers|.
+    // - When the provider is removed from |mdps_for_fast_polling_|.
     DCHECK(!(*mdp_iter)->owned_dump_provider);
     (*mdp_iter)->owned_dump_provider = std::move(owned_mdp);
   } else if (subtle::NoBarrier_Load(&memory_tracing_enabled_)) {
@@ -340,6 +347,12 @@ void MemoryDumpManager::UnregisterDumpProviderInternal(
         << "unregister itself in a racy way. Please file a crbug.";
   }
 
+  if ((*mdp_iter)->options.is_fast_polling_supported && dump_thread_) {
+    dump_thread_->task_runner()->PostTask(

[Primiano Tucci (use gerrit), 2016/12/15 16:45:01]

Add a DCHECK(take_mdp_ownership_and_delete_async) << "MemoryDumpProviders
capable of fast polling must NOT be thread bound and hence destroyed using
UnregisterAndDeleteDumpProviderSoon()"

[ssid, 2016/12/16 02:31:54]

Done.

+        FROM_HERE, Bind(&MemoryDumpManager::UnregisterPollingMDPOnDumpThread,
+                        Unretained(this), *mdp_iter));
+  }
+
   // The MDPInfo instance can still be referenced by the
   // |ProcessMemoryDumpAsyncState.pending_dump_providers|. For this reason
   // the MDPInfo is flagged as disabled. It will cause InvokeOnMemoryDump()
@@ -349,6 +362,23 @@ void MemoryDumpManager::UnregisterDumpProviderInternal(
   dump_providers_.erase(mdp_iter);
 }
 
+void MemoryDumpManager::RegisterPollingMDPOnDumpThread(
+    scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo> mdpinfo) {
+  DCHECK(!mdpinfo->task_runner);
+  mdpinfo->dump_provider->SetFastMemoryPollingEnabled(true);
+
+  AutoLock lock(lock_);
+  mdps_for_fast_polling_.insert(mdpinfo);
+}
+
+void MemoryDumpManager::UnregisterPollingMDPOnDumpThread(
+    scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo> mdpinfo) {
+  mdpinfo->dump_provider->SetFastMemoryPollingEnabled(false);
+
+  AutoLock lock(lock_);
+  mdps_for_fast_polling_.erase(mdpinfo);
+}
+
 void MemoryDumpManager::RequestGlobalDump(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail,
@@ -602,6 +632,24 @@ void MemoryDumpManager::InvokeOnMemoryDump(
   SetupNextMemoryDump(std::move(pmd_async_state));
 }
 
+void MemoryDumpManager::PollFastMemoryTotal(uint64_t* memory_total) {
+#if DCHECK_IS_ON()

[Primiano Tucci (use gerrit), 2016/12/15 16:45:01]

plz dcheck section and just add a comment.
There is a short time window where this DCHECK will crash, even if everything is
consistent, specifically, this sequence:
- OnTraceLogDisabled() is called
- It takes the lock and std::move(dump_thread_).
- Now dump_thread_ is nullptr, but the thread is still alive and MDM hasn't
requested it to stop yet.
- PolLFastMemoryTotal is called here, tries to access dump_thread_ and
segfaults.
- MDM continues the section outside of the lock in OnTraceLogDisabled, and
requests the thread (that in the meantime has been moved to a local variable) to
Stop()

 

[ssid, 2016/12/16 02:31:54]

I agree this is the case. But this is the part I should be worrying about in the
cl that actually polls. According to this CL, the polling could (theoretically)
only happen if dump_thread_ is present.
In my next cl i was planning to add a check here:
(if (dump_thread_) DCHECK())
since that condition does not make sense for this cl, i left it.
This section is useful for testing. but removed it now. In my next cl, i will
add it/ discuss if we need.

[Primiano Tucci (use gerrit), 2016/12/16 12:34:40]

Let's move this discussion to the next CL.
There is no real race here. The thread is consistently alive when you are here,
The only problem is that you shouldn't never look at the state of MDM fields_
from the dumper threads, because there are moments when the state of MDM doesn't
match the reality.

+  {
+    AutoLock lock(lock_);
+    DCHECK(dump_thread_->task_runner()->BelongsToCurrentThread());
+  }
+#endif
+  *memory_total = 0;
+  // Note that we call PollFastMemoryTotal even if the dump provider is disabled
+  // (unregistered). This is to avoid taking lock slowing this method.
+  for (const auto& mdpinfo : mdps_for_fast_polling_) {
+    uint64_t value = 0;
+    mdpinfo->dump_provider->PollFastMemoryTotal(&value);
+    *memory_total += value;
+  }
+  return;
+}
+
 // static
 void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
@@ -715,6 +763,14 @@ void MemoryDumpManager::OnTraceLogEnabled() {
     DCHECK(!dump_thread_);
     dump_thread_ = std::move(dump_thread);
 
+    mdps_for_fast_polling_.clear();
+    for (const auto& mdpinfo : dump_providers_) {
+      if (mdpinfo->options.is_fast_polling_supported) {
+        mdps_for_fast_polling_.insert(mdpinfo);
+        mdpinfo->dump_provider->SetFastMemoryPollingEnabled(true);

[Primiano Tucci (use gerrit), 2016/12/15 16:45:01]

I think we should do this call only when needed.
Otherwise doesn't make lot of sense to have this method in the first place

[ssid, 2016/12/16 02:31:54]

I just don't like the idea of session_state_ scanning the triggers once and the
PeriodicDumpTimer scanning it again. Wish I could put it all in one place.
Maybe move PeriodicDumpTimer and mdps_for_fast_polling_ into session_state_

[Primiano Tucci (use gerrit), 2016/12/16 12:34:40]

Ok the more I think to this the more I feel that the answer is:
get rid of SetFastMemoryPollingEnabled(true) and have the MDP lazily enable
(like you already do)
Let's turn this into a:

// Indicates that fast memory polling is not going to be used in the near future
and the MDP can tear down any resource kept around for fast memory polling.
void SuspendFastMemoryPolling()

Otherwise we are going to add complications everywhere for nothing.
Sorry for all the back and forth for this.

+      }
+    }
+
     subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 
     // TODO(primiano): This is a temporary hack to disable periodic memory dumps
@@ -748,6 +804,15 @@ void MemoryDumpManager::OnTraceLogDisabled() {
   periodic_dump_timer_.Stop();
   if (dump_thread)
     dump_thread->Stop();
+
+  // |mdps_for_fast_polling_| must be cleared only after the dump thread is
+  // stopped (polling tasks are done).
+  {
+    AutoLock lock(lock_);
+    for (const auto& mdpinfo : mdps_for_fast_polling_)
+      mdpinfo->dump_provider->SetFastMemoryPollingEnabled(false);
+    mdps_for_fast_polling_.clear();
+  }
 }
 
 bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {