[tracing] Introduce BACKGROUND mode in MemoryInfra

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 b14d265f19ebbdc7945c141474957bece0aad403..26d8bc3e9c9f77325887c97b71fed33157989e41 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -16,6 +16,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/timer/timer.h"
 #include "base/trace_event/heap_profiler.h"
 #include "base/trace_event/heap_profiler_allocation_context_tracker.h"
 #include "base/trace_event/heap_profiler_stack_frame_deduplicator.h"
@@ -46,26 +47,14 @@ const char* kTraceEventArgNames[] = {"dumps"};
 const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
 
 StaticAtomicSequenceNumber g_next_guid;
-uint32_t g_periodic_dumps_count = 0;
-uint32_t g_heavy_dumps_rate = 0;
 MemoryDumpManager* g_instance_for_testing = nullptr;
 
-void RequestPeriodicGlobalDump() {
-  MemoryDumpLevelOfDetail level_of_detail;
-  if (g_heavy_dumps_rate == 0) {
-    level_of_detail = MemoryDumpLevelOfDetail::LIGHT;
-  } else {
-    level_of_detail = g_periodic_dumps_count == 0
-                          ? MemoryDumpLevelOfDetail::DETAILED
-                          : MemoryDumpLevelOfDetail::LIGHT;
-
-    if (++g_periodic_dumps_count == g_heavy_dumps_rate)
-      g_periodic_dumps_count = 0;
-  }
-
-  MemoryDumpManager::GetInstance()->RequestGlobalDump(
-      MemoryDumpType::PERIODIC_INTERVAL, level_of_detail);
-}
+// The names of dump providers whitelisted for background tracing. Dump
+// providers can be added here only if the background mode dump has very
+// less performance and memory overhead.
+const char* const kDumpProviderWhitelist[] = {
+    "WhitelistedTestDumpProvider",  // For testing.

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

I think a better approach is to:
- Make this an empty array (for the moment, with a TODO that you will fill it in
next CLs)
- Introduce a static private field in MDM (a pointer to array) called
background_dump_providers_whitelist. 
- Have the MDM ctor set dump_provider_whitelist = kDumpProviderWhitelist 
- Have an instance method
MDM::GetInstance()->SetBackgroundDumpProviderWhitelistForTesting(pointer to
array)
- Have the test set this to its own array.

Does it make sense?

[ssid, 2016/05/26 22:12:54]

Yeah the problem was that i cannot get the array size when i loop through it and
i had to store either size or a vector.
If the issue is just the test name, i have added an extra field in dump manager
for the testing case.

The other option would be to use BlinkGC as dump provider for unittests

+};
 
 // Callback wrapper to hook upon the completion of RequestGlobalDump() and
 // inject trace markers.
@@ -109,8 +98,90 @@ struct SessionStateConvertableProxy : public ConvertableToTraceFormat {
   GetterFunctPtr const getter_function;
 };
 
+// Checks if the dump provider is in the whitelist.
+bool IsDumpProviderInWhitelist(const char* name) {
+  for (size_t i = 0; i < arraysize(kDumpProviderWhitelist); ++i) {
+    if (strcmp(name, kDumpProviderWhitelist[i]) == 0)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
+// Sets up periodic memory dump timers to start global dump requests based on
+// the dump triggers from trace config.
+class MemoryDumpManager::PeriodicGlobalDumpInvokeHelper {

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

nit: I think this should go after the constants below
(MaxConsecutiveFailuresCount and friends)

[ssid, 2016/05/26 22:12:54]

Added to the last.

+ public:
+  PeriodicGlobalDumpInvokeHelper(
+      const std::vector<TraceConfig::MemoryDumpConfig::Trigger>& triggers_list);
+  ~PeriodicGlobalDumpInvokeHelper();
+
+  // Periodically called by the timer.
+  void RequestPeriodicGlobalDump();
+
+ private:
+  RepeatingTimer periodic_dump_timer_;
+  uint32_t periodic_dumps_count_;
+  uint32_t light_dump_rate_;
+  uint32_t heavy_dump_rate_;
+};
+
+MemoryDumpManager::PeriodicGlobalDumpInvokeHelper::
+    PeriodicGlobalDumpInvokeHelper(

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

In general the constructor should not do actions, mostly for sake of
readability.
If I see new DumpInvokeHelper() I don't expect that to do anything.
Can you just make the ctor of this trivial and turn this into a Start() method?

[ssid, 2016/05/26 22:12:54]

Makes sense.

+        const std::vector<TraceConfig::MemoryDumpConfig::Trigger>&
+            triggers_list)
+    : 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(triggers_list.size());
+  DCHECK_LE(triggers_list.size(), 3u);
+  for (const TraceConfig::MemoryDumpConfig::Trigger& config : triggers_list) {
+    if (config.level_of_detail == MemoryDumpLevelOfDetail::LIGHT) {
+      DCHECK_EQ(0u, light_dump_period_ms);
+      light_dump_period_ms = config.periodic_interval_ms;
+    } else if (config.level_of_detail == MemoryDumpLevelOfDetail::DETAILED) {
+      DCHECK_EQ(0u, heavy_dump_period_ms);
+      heavy_dump_period_ms = config.periodic_interval_ms;
+    }
+    min_timer_period_ms =
+        std::min(min_timer_period_ms, config.periodic_interval_ms);
+  }
+
+  // Do not start timer if period is 0 for any trigger.
+  if (!min_timer_period_ms)

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

Hmm this is a bit contra-intuitive. THis means that if you say:
heavy: 5, light: 0, both heavy and light will be disabled :/
What is the rationale of this?

[ssid, 2016/05/26 22:12:54]

Yeah, I was thinking it is useful to specify mode without period. But realized
it is useless.

+    return;
+
+  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_rate_ % min_timer_period_ms);
+  heavy_dump_rate_ = heavy_dump_period_ms / min_timer_period_ms;
+
+  periodic_dump_timer_.Start(
+      FROM_HERE, TimeDelta::FromMilliseconds(min_timer_period_ms),
+      base::Bind(&PeriodicGlobalDumpInvokeHelper::RequestPeriodicGlobalDump,
+                 base::Unretained(this)));
+}
+
+MemoryDumpManager::PeriodicGlobalDumpInvokeHelper::
+    ~PeriodicGlobalDumpInvokeHelper() {
+  periodic_dump_timer_.Stop();
+}
+
+void MemoryDumpManager::PeriodicGlobalDumpInvokeHelper::
+    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);
+}
+
 // static
 const char* const MemoryDumpManager::kTraceCategory =
     TRACE_DISABLED_BY_DEFAULT("memory-infra");
@@ -449,6 +520,15 @@ void MemoryDumpManager::SetupNextMemoryDump(
   MemoryDumpProviderInfo* mdpinfo =
       pmd_async_state->pending_dump_providers.back().get();
 
+  // If we are in background tracing, we should invoke only the whitelisted
+  // providers. Ignore other providers and continue.
+  if (pmd_async_state->req_args.level_of_detail ==
+          MemoryDumpLevelOfDetail::BACKGROUND &&
+      mdpinfo->disabled_for_background_mode) {
+    pmd_async_state->pending_dump_providers.pop_back();
+    return SetupNextMemoryDump(std::move(pmd_async_state));
+  }
+
   // If the dump provider did not specify a task runner affinity, dump on
   // |dump_thread_| which is already checked above for presence.
   SequencedTaskRunner* task_runner = mdpinfo->task_runner.get();
@@ -632,78 +712,64 @@ void MemoryDumpManager::OnTraceLogEnabled() {
     return;
   }
 
-  AutoLock lock(lock_);

[ssid, 2016/05/23 18:17:03]

This unnecessarily locks the whole function. I cannot call RequestGlobalDump
when already inside lock. So, restructued it to use lock only when necessary.

This assumes that the following does not need lock:
periodic_dump_helper_ - already accessed in OnTraceLogDisabled without lock. So,
not really useful to have a lock here. If there is a race between enabling and
disabling, then it could also happen that OnTraceLogDisabled can be called even
before OntraceLogEnabled and the timer runs for ever. So, this can't be avoided
anyway.

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

Where is the call to RequestGlobalDump? In the timer?
Nope, you are right. If OnTraceLogEnabled and Disabled can be called at the same
time this is the end and we should just go home :)

[ssid, 2016/05/26 22:12:54]

We currently do the first dump after 250ms of starting recording.
I was trying to see if triggering the first dump as soon as trace log is enabled
works. But it doesn't because the processes do not yet get the tracing enabled
message.
So, in that case it hit a deadlock since it was trying to lock twice. I made
this just to be safe in case we need to trigger dumps here in future.

-
-  DCHECK(delegate_);  // At this point we must have a delegate.
-  session_state_ = new MemoryDumpSessionState;
-
+  const TraceConfig trace_config =
+      TraceLog::GetInstance()->GetCurrentTraceConfig();
+  scoped_refptr<MemoryDumpSessionState> session_state =
+      new MemoryDumpSessionState;
   if (heap_profiling_enabled_) {
     // If heap profiling is enabled, the stack frame deduplicator and type name
     // deduplicator will be in use. Add a metadata events to write the frames
     // and type IDs.
-    session_state_->SetStackFrameDeduplicator(
+    session_state->SetStackFrameDeduplicator(
         WrapUnique(new StackFrameDeduplicator));
 
-    session_state_->SetTypeNameDeduplicator(
+    session_state->SetTypeNameDeduplicator(
         WrapUnique(new TypeNameDeduplicator));
 
     TRACE_EVENT_API_ADD_METADATA_EVENT(
         TraceLog::GetCategoryGroupEnabled("__metadata"), "stackFrames",
         "stackFrames",
-        WrapUnique(
-            new SessionStateConvertableProxy<StackFrameDeduplicator>(
-                session_state_,
-                &MemoryDumpSessionState::stack_frame_deduplicator)));
+        WrapUnique(new SessionStateConvertableProxy<StackFrameDeduplicator>(
+            session_state, &MemoryDumpSessionState::stack_frame_deduplicator)));
 
     TRACE_EVENT_API_ADD_METADATA_EVENT(
         TraceLog::GetCategoryGroupEnabled("__metadata"), "typeNames",
         "typeNames",
         WrapUnique(new SessionStateConvertableProxy<TypeNameDeduplicator>(
-            session_state_, &MemoryDumpSessionState::type_name_deduplicator)));
+            session_state, &MemoryDumpSessionState::type_name_deduplicator)));
   }
+  session_state->SetMemoryDumpConfig(trace_config.memory_dump_config());

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

nit: seems a bit more logical to have this after sessions_state = ... above on
line 718.
Or is there a reason to have this after the if(heap_profiling)

[ssid, 2016/05/26 22:12:54]

Done.

 
-  DCHECK(!dump_thread_);
-  dump_thread_ = std::move(dump_thread);
-  subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
+  {
+    AutoLock lock(lock_);
 
-  // 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;
+    DCHECK(delegate_);  // At this point we must have a delegate.
+    session_state_ = session_state;
+
+    DCHECK(!dump_thread_);
+    dump_thread_ = std::move(dump_thread);
+
+    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;
+    }
   }
 
   // Enable periodic dumps. At the moment the periodic support is limited to at
-  // most one low-detail periodic dump and at most one high-detail periodic
-  // dump. If both are specified the high-detail period must be an integer
-  // multiple of the low-level one.
-  g_periodic_dumps_count = 0;
-  const TraceConfig trace_config =
-      TraceLog::GetInstance()->GetCurrentTraceConfig();
-  session_state_->SetMemoryDumpConfig(trace_config.memory_dump_config());

[ssid, 2016/05/23 18:17:03]

I think this needs to be set before the check for is_coordinator. The renderer
process won't see the config otherwise.

+  // most one periodic trigger per dump mode. All intervals should be an integer
+  // multiple of the smallest interval specified.
   const std::vector<TraceConfig::MemoryDumpConfig::Trigger>& triggers_list =
       trace_config.memory_dump_config().triggers;
-  if (triggers_list.empty())
-    return;
-
-  uint32_t min_timer_period_ms = std::numeric_limits<uint32_t>::max();
-  uint32_t heavy_dump_period_ms = 0;
-  DCHECK_LE(triggers_list.size(), 2u);
-  for (const TraceConfig::MemoryDumpConfig::Trigger& config : triggers_list) {
-    DCHECK(config.periodic_interval_ms);
-    if (config.level_of_detail == MemoryDumpLevelOfDetail::DETAILED)
-      heavy_dump_period_ms = config.periodic_interval_ms;
-    min_timer_period_ms =
-        std::min(min_timer_period_ms, config.periodic_interval_ms);
+  if (!triggers_list.empty()) {

[Primiano Tucci (use gerrit), 2016/05/26 17:20:53]

maybe just always pass the triggers and to this check in the helper?
Also I think you can just pass trace_config.memory_dump_config().triggers as
argument, no need for the intermedia dir, especially if you introduce the start
method.

[ssid, 2016/05/26 22:12:54]

Done.

+    periodic_dump_helper_.reset(
+        new PeriodicGlobalDumpInvokeHelper(triggers_list));
   }
-  DCHECK_EQ(0u, heavy_dump_period_ms % min_timer_period_ms);
-  g_heavy_dumps_rate = heavy_dump_period_ms / min_timer_period_ms;
-
-  periodic_dump_timer_.Start(FROM_HERE,
-                             TimeDelta::FromMilliseconds(min_timer_period_ms),
-                             base::Bind(&RequestPeriodicGlobalDump));
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
@@ -720,7 +786,7 @@ 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();
+  periodic_dump_helper_.reset();
   if (dump_thread)
     dump_thread->Stop();
 }
@@ -739,7 +805,8 @@ MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
       task_runner(std::move(task_runner)),
       options(options),
       consecutive_failures(0),
-      disabled(false) {}
+      disabled(false),
+      disabled_for_background_mode(!IsDumpProviderInWhitelist(name)) {}
 
 MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {}