[tracing] Implement polling 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 096eaf88487828c43ca9748182282eeed73df80a..d54352cb08b535a2aeacbdcb8bd3eabbedade343 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -25,6 +25,7 @@
 #include "base/trace_event/malloc_dump_provider.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/memory_dump_session_state.h"
+#include "base/trace_event/memory_dump_trigger.h"
 #include "base/trace_event/memory_infra_background_whitelist.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
@@ -128,7 +129,6 @@ void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
 
 MemoryDumpManager::MemoryDumpManager()
     : delegate_(nullptr),
-      is_coordinator_(false),
       memory_tracing_enabled_(0),
       tracing_process_id_(kInvalidTracingProcessId),
       dumper_registrations_ignored_for_testing_(false),
@@ -188,8 +188,8 @@ void MemoryDumpManager::Initialize(MemoryDumpManagerDelegate* delegate,
     DCHECK(delegate);
     DCHECK(!delegate_);
     delegate_ = delegate;
-    is_coordinator_ = is_coordinator;
     EnableHeapProfilingIfNeeded();
+    dump_trigger_.reset(new MemoryDumpTrigger(this, is_coordinator));
   }
 
 // Enable the core dump providers.
@@ -374,6 +374,15 @@ void MemoryDumpManager::RegisterPollingMDPOnDumpThread(
   DCHECK(!mdpinfo->task_runner);
   AutoLock lock(lock_);
   dump_providers_for_polling_.insert(mdpinfo);
+
+  // Setup polling when first polling supported provider is registered since
+  // OnTraceLogEnabled() would not have setup polling without any provider that
+  // supports polling.

[Primiano Tucci (use gerrit), 2017/01/18 16:16:06]

can you expand a bit here about why? To me feels a bit more natural that, at the
moment we start meminfra and read the config, we setup the polling.
Eventually the first polls will be no-ops because we don't have MDPs yet.
But I think the right layering here should be MDM saying "I am not ready for
polling yet" (using the bool retval of Poll maybe)

[ssid, 2017/01/20 23:07:27]

If I did that then we would be unnecessarily setting timer every 25ms and
polling nothing.
This is a major concern in Linux where we will never register dump providers
with polling in renderer. So, we shouldn't be start polling unless we have dump
providers. I changed it to Notify instead of SetupPeakTriggers. Does that look
better?

+  if (dump_providers_for_polling_.size() == 1) {
+    dump_trigger_->SetupPeakTriggers(
+        session_state_->memory_dump_config().triggers,
+        ThreadTaskRunnerHandle::Get());

[Primiano Tucci (use gerrit), 2017/01/18 16:16:06]

this one confuses me a bit: how do you know at this point if the mdconfig
contains only a peak trigger or several triggers?

[ssid, 2017/01/20 23:07:27]

So, now it does not matter. The scheduler knows if it should setup polling.

I did not want to read the config at two places (OnTraceLogEnabled and here) to
start polling. that is why I checked if poling config exists in scheduler
itself.

+  }
 }
 
 void MemoryDumpManager::UnregisterPollingMDPOnDumpThread(
@@ -382,6 +391,9 @@ void MemoryDumpManager::UnregisterPollingMDPOnDumpThread(
 
   AutoLock lock(lock_);
   dump_providers_for_polling_.erase(mdpinfo);
+  DCHECK(!dump_providers_for_polling_.empty())
+      << "All polling MDPs cannot be unregistered, since it will cause polling "
+         "without without any MDPs";
 }
 
 void MemoryDumpManager::RequestGlobalDump(
@@ -637,7 +649,17 @@ void MemoryDumpManager::InvokeOnMemoryDump(
   SetupNextMemoryDump(std::move(pmd_async_state));
 }
 
-void MemoryDumpManager::PollFastMemoryTotal(uint64_t* memory_total) {
+bool MemoryDumpManager::PollFastMemoryTotal(uint64_t* memory_total) {
+#if DCHECK_IS_ON()
+  {
+    AutoLock lock(lock_);
+    if (dump_thread_)
+      DCHECK(dump_thread_->task_runner()->BelongsToCurrentThread());
+  }
+#endif
+  if (dump_providers_for_polling_.empty())
+    return false;
+
   *memory_total = 0;
   // Note that we call PollFastMemoryTotal() even if the dump provider is
   // disabled (unregistered). This is to avoid taking lock while polling.
@@ -646,7 +668,7 @@ void MemoryDumpManager::PollFastMemoryTotal(uint64_t* memory_total) {
     mdpinfo->dump_provider->PollFastMemoryTotal(&value);
     *memory_total += value;
   }
-  return;
+  return true;
 }
 
 // static
@@ -753,6 +775,8 @@ void MemoryDumpManager::OnTraceLogEnabled() {
             session_state, &MemoryDumpSessionState::type_name_deduplicator));
   }
 
+  scoped_refptr<SingleThreadTaskRunner> polling_task_runner;
+  MemoryDumpTrigger* trigger = nullptr;
   {
     AutoLock lock(lock_);
 
@@ -767,21 +791,23 @@ void MemoryDumpManager::OnTraceLogEnabled() {
       if (mdpinfo->options.is_fast_polling_supported)
         dump_providers_for_polling_.insert(mdpinfo);
     }
+    if (!dump_providers_for_polling_.empty())
+      polling_task_runner = dump_thread_->task_runner();
 
     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;
-    }
+    trigger = dump_trigger_.get();
   }
 
+  // Setup peak triggers only if some polling supported provider was registered,
+  // else polling will be setup by RegisterPollingMDPOnDumpThread when first
+  // polling MDP registers.
+  if (polling_task_runner) {
+    trigger->SetupPeakTriggers(trace_config.memory_dump_config().triggers,
+                               polling_task_runner);
+  }
   // Enable periodic dumps if necessary.
-  periodic_dump_timer_.Start(trace_config.memory_dump_config().triggers);
+  trigger->SetupPeriodicTriggers(trace_config.memory_dump_config().triggers);
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
@@ -800,18 +826,20 @@ void MemoryDumpManager::OnTraceLogDisabled() {
 
   // 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();
 
-  // |dump_providers_for_polling_| must be cleared only after the dump thread is
-  // stopped (polling tasks are done).
+  // |dump_providers_for_polling_| must be cleared and triggers must be
+  // disabled only after the dump thread is stopped (polling tasks are done).
+  MemoryDumpTrigger* trigger = nullptr;
   {
     AutoLock lock(lock_);
     for (const auto& mdpinfo : dump_providers_for_polling_)
       mdpinfo->dump_provider->SuspendFastMemoryPolling();
     dump_providers_for_polling_.clear();
+    trigger = dump_trigger_.get();
   }
+  trigger->Disable();
 }
 
 bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {
@@ -885,80 +913,5 @@ ProcessMemoryDump* MemoryDumpManager::ProcessMemoryDumpAsyncState::
   return iter->second.get();
 }
 
-MemoryDumpManager::PeriodicGlobalDumpTimer::PeriodicGlobalDumpTimer() {}
-
-MemoryDumpManager::PeriodicGlobalDumpTimer::~PeriodicGlobalDumpTimer() {
-  Stop();
-}
-
-void MemoryDumpManager::PeriodicGlobalDumpTimer::Start(
-    const std::vector<TraceConfig::MemoryDumpConfig::Trigger>& triggers_list) {
-  if (triggers_list.empty())
-    return;
-
-  // At the moment the periodic support is limited to at most one periodic
-  // trigger per dump mode. All intervals should be an integer multiple of the
-  // smallest interval specified.
-  periodic_dumps_count_ = 0;
-  uint32_t min_timer_period_ms = std::numeric_limits<uint32_t>::max();
-  uint32_t light_dump_period_ms = 0;
-  uint32_t heavy_dump_period_ms = 0;
-  DCHECK_LE(triggers_list.size(), 3u);
-  auto* mdm = MemoryDumpManager::GetInstance();
-  for (const TraceConfig::MemoryDumpConfig::Trigger& config : triggers_list) {
-    DCHECK_NE(0u, config.min_time_between_dumps_ms);
-    DCHECK_EQ(MemoryDumpType::PERIODIC_INTERVAL, config.trigger_type)
-        << "Only periodic_interval triggers are suppported";
-    switch (config.level_of_detail) {
-      case MemoryDumpLevelOfDetail::BACKGROUND:
-        DCHECK(mdm->IsDumpModeAllowed(MemoryDumpLevelOfDetail::BACKGROUND));
-        break;
-      case MemoryDumpLevelOfDetail::LIGHT:
-        DCHECK_EQ(0u, light_dump_period_ms);
-        DCHECK(mdm->IsDumpModeAllowed(MemoryDumpLevelOfDetail::LIGHT));
-        light_dump_period_ms = config.min_time_between_dumps_ms;
-        break;
-      case MemoryDumpLevelOfDetail::DETAILED:
-        DCHECK_EQ(0u, heavy_dump_period_ms);
-        DCHECK(mdm->IsDumpModeAllowed(MemoryDumpLevelOfDetail::DETAILED));
-        heavy_dump_period_ms = config.min_time_between_dumps_ms;
-        break;
-    }
-    min_timer_period_ms =
-        std::min(min_timer_period_ms, config.min_time_between_dumps_ms);
-  }
-
-  DCHECK_EQ(0u, light_dump_period_ms % min_timer_period_ms);
-  light_dump_rate_ = light_dump_period_ms / min_timer_period_ms;
-  DCHECK_EQ(0u, heavy_dump_period_ms % min_timer_period_ms);
-  heavy_dump_rate_ = heavy_dump_period_ms / min_timer_period_ms;
-
-  timer_.Start(FROM_HERE, TimeDelta::FromMilliseconds(min_timer_period_ms),
-               base::Bind(&PeriodicGlobalDumpTimer::RequestPeriodicGlobalDump,
-                          base::Unretained(this)));
-}
-
-void MemoryDumpManager::PeriodicGlobalDumpTimer::Stop() {
-  if (IsRunning()) {
-    timer_.Stop();
-  }
-}
-
-bool MemoryDumpManager::PeriodicGlobalDumpTimer::IsRunning() {
-  return timer_.IsRunning();
-}
-
-void MemoryDumpManager::PeriodicGlobalDumpTimer::RequestPeriodicGlobalDump() {
-  MemoryDumpLevelOfDetail level_of_detail = MemoryDumpLevelOfDetail::BACKGROUND;
-  if (light_dump_rate_ > 0 && periodic_dumps_count_ % light_dump_rate_ == 0)
-    level_of_detail = MemoryDumpLevelOfDetail::LIGHT;
-  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