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

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 <utility>
 
 #include "base/allocator/features.h"
@@ skipping 266 matching lines @@
       new MemoryDumpProviderInfo(mdp, name, std::move(task_runner), options,
                                  whitelisted_for_background_mode);
 
   {
     AutoLock lock(lock_);
     bool already_registered = !dump_providers_.insert(mdpinfo).second;
     // This actually happens in some tests which don't have a clean tear-down
     // 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_)
     mdp->OnHeapProfilingEnabled(true);
 }
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   UnregisterDumpProviderInternal(mdp, false /* delete_async */);
 }
 
@@ skipping 18 matching lines @@
   }
 
   if (mdp_iter == dump_providers_.end())
     return;  // Not registered / already unregistered.
 
   if (take_mdp_ownership_and_delete_async) {
     // The MDP will be deleted whenever the MDPInfo struct will, that is either:
     // - 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_)) {
     // If you hit this DCHECK, your dump provider has a bug.
     // Unregistration of a MemoryDumpProvider is safe only if:
     // - The MDP has specified a sequenced task runner affinity AND the
     //   unregistration happens on the same task runner. So that the MDP cannot
     //   unregister and be in the middle of a OnMemoryDump() at the same time.
     // - The MDP has NOT specified a task runner affinity and its ownership is
     //   transferred via UnregisterAndDeleteDumpProviderSoon().
     // In all the other cases, it is not possible to guarantee that the
     // unregistration will not race with OnMemoryDump() calls.
     DCHECK((*mdp_iter)->task_runner &&
            (*mdp_iter)->task_runner->RunsTasksOnCurrentThread())
         << "MemoryDumpProvider \"" << (*mdp_iter)->name << "\" attempted to "
         << "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()
   // to just skip it, without actually invoking the |mdp|, which might be
   // destroyed by the caller soon after this method returns.
   (*mdp_iter)->disabled = true;
   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,
     const MemoryDumpCallback& callback) {
   // Bail out immediately if tracing is not enabled at all or if the dump mode
   // is not allowed.
   if (!UNLIKELY(subtle::NoBarrier_Load(&memory_tracing_enabled_)) ||
       !IsDumpModeAllowed(level_of_detail)) {
     VLOG(1) << kLogPrefix << " failed because " << kTraceCategory
             << " tracing category is not enabled or the requested dump mode is "
@@ skipping 233 matching lines @@
                                                                   args);
     bool dump_successful = mdpinfo->dump_provider->OnMemoryDump(args, pmd);
     mdpinfo->consecutive_failures =
         dump_successful ? 0 : mdpinfo->consecutive_failures + 1;
   }
 
   pmd_async_state->pending_dump_providers.pop_back();
   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) {
   HEAP_PROFILER_SCOPED_IGNORE;
   DCHECK(pmd_async_state->pending_dump_providers.empty());
   const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
   if (!pmd_async_state->callback_task_runner->BelongsToCurrentThread()) {
     scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
         pmd_async_state->callback_task_runner;
     callback_task_runner->PostTask(
@@ skipping 93 matching lines @@
 
   {
     AutoLock lock(lock_);
 
     DCHECK(delegate_);  // At this point we must have a delegate.
     session_state_ = session_state;
 
     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
     // when running memory benchmarks until telemetry uses TraceConfig to
     // enable/disable periodic dumps. See crbug.com/529184 .
     if (!is_coordinator_ ||
         CommandLine::ForCurrentProcess()->HasSwitch(
             "enable-memory-benchmarking")) {
       return;
     }
@@ skipping 13 matching lines @@
     AutoLock lock(lock_);
     dump_thread = std::move(dump_thread_);
     session_state_ = nullptr;
   }
 
   // Thread stops are blocking and must be performed outside of the |lock_|
   // or will deadlock (e.g., if SetupNextMemoryDump() tries to acquire it).
   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) {
   AutoLock lock(lock_);
   if (!session_state_)
     return false;
   return session_state_->memory_dump_config().allowed_dump_modes.count(
              dump_mode) != 0;
 }
 
@@ skipping 130 matching lines @@
   if (heavy_dump_rate_ > 0 && periodic_dumps_count_ % heavy_dump_rate_ == 0)
     level_of_detail = MemoryDumpLevelOfDetail::DETAILED;
   ++periodic_dumps_count_;
 
   MemoryDumpManager::GetInstance()->RequestGlobalDump(
       MemoryDumpType::PERIODIC_INTERVAL, level_of_detail);
 }
 
 }  // namespace trace_event
 }  // namespace base