[tracing] MemoryDumpManager uses TraceConfig to set the periodic dump timers

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 "base/atomic_sequence_num.h"
 #include "base/command_line.h"
@@ skipping 20 matching lines @@
 
 #if defined(OS_WIN)
 #include "base/trace_event/winheap_dump_provider_win.h"
 #endif
 
 namespace base {
 namespace trace_event {
 
 namespace {
 
-// Throttle mmaps at a rate of once every kHeavyMmapsDumpsRate standard dumps.
-const int kHeavyDumpsRate = 8;  // 250 ms * 8 = 2000 ms.
-const int kDumpIntervalMs = 250;
 const int kTraceEventNumArgs = 1;
 const char* kTraceEventArgNames[] = {"dumps"};
 const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
 
 StaticAtomicSequenceNumber g_next_guid;
 uint32 g_periodic_dumps_count = 0;
+uint32 g_shortest_dump_interval_ms;
 MemoryDumpManager* g_instance_for_testing = nullptr;
 
 void RequestPeriodicGlobalDump() {
-  MemoryDumpArgs::LevelOfDetail dump_level_of_detail =
-      g_periodic_dumps_count == 0 ? MemoryDumpArgs::LevelOfDetail::HIGH
-                                  : MemoryDumpArgs::LevelOfDetail::LOW;
-  if (++g_periodic_dumps_count == kHeavyDumpsRate)
-    g_periodic_dumps_count = 0;
+  MemoryDumpArgs dump_args = {MemoryDumpArgs::LevelOfDetail::LOW};
+  TraceConfig trace_config = TraceLog::GetInstance()->GetCurrentTraceConfig();
 
-  MemoryDumpArgs dump_args = {dump_level_of_detail};
+  // Assign the highest dump args among the periodic dump triggers whose
+  // interval end at current time.
+  for (const TraceConfig::MemoryDumpTriggerConfig& config :

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

All this seems way too complex. I don't like the idea of re-reading and copying
the TraceConfig at each dump. I think you here want to do something way simpler,
which is:

Just add a g_heavy_dumps_rate and set it below when you parse the traceconfig.
In this method just check 
if (g_heavy_dumps_rate > 0 && )g_periodic_dumps_count % g_heavy_dumps_rate) ==
0)
  do heavy dump
else 
  do light dump

+       trace_config.memory_dump_config()) {
+    if (g_periodic_dumps_count %
+            (config.periodic_interval_ms / g_shortest_dump_interval_ms) ==
+        0) {
+      if (dump_args.level_of_detail < config.level_of_detail)
+        dump_args.level_of_detail = config.level_of_detail;
+    }
+  }
+  g_periodic_dumps_count++;
+
   MemoryDumpManager::GetInstance()->RequestGlobalDump(
       MemoryDumpType::PERIODIC_INTERVAL, dump_args);
 }
 
 }  // namespace
 
-// TODO(ssid): Start using TraceConfig and remove this.
 // static
 const char* const MemoryDumpManager::kTraceCategory =
     TRACE_DISABLED_BY_DEFAULT("memory-infra");
 
 // static
 const int MemoryDumpManager::kMaxConsecutiveFailuresCount = 3;
 
 // static
 const uint64 MemoryDumpManager::kInvalidTracingProcessId = 0;
 
@@ skipping 310 matching lines @@
   // invoking the callback while holding the lock_.
   task_runner->PostTask(FROM_HERE,
                         Bind(callback, dump_guid, false /* success */));
 }
 
 void MemoryDumpManager::OnTraceLogEnabled() {
   // TODO(primiano): at this point we query  TraceLog::GetCurrentCategoryFilter
   // to figure out (and cache) which dumpers should be enabled or not.
   // For the moment piggy back everything on the generic "memory" category.
   bool enabled;
   TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

You don't need this check anymore right?

 
   // Initialize the TraceLog for the current thread. This is to avoid that the
   // TraceLog memory dump provider is registered lazily in the PostTask() below
   // while the |lock_| is taken;
   TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
 
   AutoLock lock(lock_);
 
   // There is no point starting the tracing without a delegate.
   if (!enabled || !delegate_) {
     // Disable all the providers.
     for (auto it = dump_providers_.begin(); it != dump_providers_.end(); ++it)
       it->disabled = true;
     return;
   }
 
   session_state_ = new MemoryDumpSessionState();
   for (auto it = dump_providers_.begin(); it != dump_providers_.end(); ++it) {
     it->disabled = false;
     it->consecutive_failures = 0;
   }
 
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 
   // TODO(primiano): This is a temporary hack to disable periodic memory dumps
   // when running memory benchmarks until they can be enabled/disabled in
   // base::trace_event::TraceConfig. See https://goo.gl/5Hj3o0.
   // The same mechanism should be used to disable periodic dumps in tests.
-  if (delegate_->IsCoordinatorProcess() &&
-      !CommandLine::ForCurrentProcess()->HasSwitch(
-          "enable-memory-benchmarking") &&
-      !disable_periodic_dumps_for_testing_) {
-    g_periodic_dumps_count = 0;
-    periodic_dump_timer_.Start(FROM_HERE,
-                               TimeDelta::FromMilliseconds(kDumpIntervalMs),
-                               base::Bind(&RequestPeriodicGlobalDump));
+  if (!delegate_->IsCoordinatorProcess() ||
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          "enable-memory-benchmarking") ||
+      disable_periodic_dumps_for_testing_) {
+    return;
   }
+
+  g_periodic_dumps_count = 0;
+  const TraceConfig trace_config =
+      TraceLog::GetInstance()->GetCurrentTraceConfig();
+  const TraceConfig::MemoryDumpConfig& config_list =
+      trace_config.memory_dump_config();
+  if (!config_list.size())

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

if (config_list.empty()) ?

+    return;
+
+  g_shortest_dump_interval_ms = std::numeric_limits<uint32>::max();
+  for (const TraceConfig::MemoryDumpTriggerConfig& config : config_list) {
+    if (g_shortest_dump_interval_ms > config.periodic_interval_ms)
+      g_shortest_dump_interval_ms = config.periodic_interval_ms;
+  }
+  DCHECK_NE(g_shortest_dump_interval_ms, std::numeric_limits<uint32>::max());
+
+  // It does not make sense to have dumps at varying intervals.

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

Is not that it doesnt' make sense.
The deal is that we don't want to support it, which is different.
Please adjust the comment... and the code according to the comment above.

+  for (const TraceConfig::MemoryDumpTriggerConfig& config : config_list)
+    DCHECK_EQ(0u, config.periodic_interval_ms % g_shortest_dump_interval_ms);
+
+  periodic_dump_timer_.Start(
+      FROM_HERE, TimeDelta::FromMilliseconds(g_shortest_dump_interval_ms),
+      base::Bind(&RequestPeriodicGlobalDump));
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
   AutoLock lock(lock_);
   periodic_dump_timer_.Stop();
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
   session_state_ = nullptr;
 }
 
 uint64 MemoryDumpManager::GetTracingProcessId() const {
@@ skipping 29 matching lines @@
       next_dump_provider(next_dump_provider),
       callback(callback),
       task_runner(MessageLoop::current()->task_runner()) {
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
 }
 
 }  // namespace trace_event
 }  // namespace base