[memory-infra] MemoryDumpScheduler follows config for time between dumps

base/trace_event/memory_dump_scheduler.cc

 // Copyright 2017 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_scheduler.h"
 
 #include "base/process/process_metrics.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
@@ skipping 24 matching lines @@
 #endif
 }
 
 MemoryDumpScheduler::~MemoryDumpScheduler() {}
 
 void MemoryDumpScheduler::AddTrigger(MemoryDumpType trigger_type,
                                      MemoryDumpLevelOfDetail level_of_detail,
                                      uint32_t min_time_between_dumps_ms) {
   if (trigger_type == MemoryDumpType::PEAK_MEMORY_USAGE) {
     DCHECK(!periodic_state_.is_configured);
-    DCHECK(!polling_state_.is_configured);
+    DCHECK_EQ(PollingTriggerState::DISABLED, polling_state_.current_state);
     DCHECK_NE(0u, min_time_between_dumps_ms);
 
     polling_state_.level_of_detail = level_of_detail;
     polling_state_.min_polls_between_dumps =
         (min_time_between_dumps_ms + polling_state_.polling_interval_ms - 1) /
         polling_state_.polling_interval_ms;
-    polling_state_.is_configured = true;
+    polling_state_.current_state = PollingTriggerState::CONFIGURED;
   } else if (trigger_type == MemoryDumpType::PERIODIC_INTERVAL) {
-    DCHECK(!polling_state_.is_configured);
+    DCHECK_EQ(PollingTriggerState::DISABLED, polling_state_.current_state);
     periodic_state_.is_configured = true;
     DCHECK_NE(0u, min_time_between_dumps_ms);
     switch (level_of_detail) {
       case MemoryDumpLevelOfDetail::BACKGROUND:
         break;
       case MemoryDumpLevelOfDetail::LIGHT:
         DCHECK_EQ(0u, periodic_state_.light_dump_period_ms);
         periodic_state_.light_dump_period_ms = min_time_between_dumps_ms;
         break;
       case MemoryDumpLevelOfDetail::DETAILED:
@@ skipping 20 matching lines @@
                                      periodic_state_.min_timer_period_ms;
 
   periodic_state_.dump_count = 0;
   periodic_state_.timer.Start(
       FROM_HERE,
       TimeDelta::FromMilliseconds(periodic_state_.min_timer_period_ms),
       Bind(&MemoryDumpScheduler::RequestPeriodicGlobalDump, Unretained(this)));
 }
 
 void MemoryDumpScheduler::NotifyPollingSupported() {
-  if (!polling_state_.is_configured || polling_state_.is_polling_enabled)
+  if (polling_state_.current_state != PollingTriggerState::CONFIGURED)
     return;
-  polling_state_.is_polling_enabled = true;
-  for (uint32_t i = 0;
-       i < PollingTriggerState::kNumTotalsTrackedForPeakDetection; ++i) {
-    polling_state_.last_memory_totals_kb[i] = 0;
-  }
-  polling_state_.last_memory_totals_kb_index = 0;
-  polling_state_.num_polls_from_last_dump = 0;
-  polling_state_.last_dump_memory_total = 0;
+  polling_state_.current_state = PollingTriggerState::ENABLED;
+
+  polling_state_.ResetTotals();
   polling_state_.polling_task_runner->PostTask(
       FROM_HERE,
       Bind(&MemoryDumpScheduler::PollMemoryOnPollingThread, Unretained(this)));
 }
 
+void MemoryDumpScheduler::NotifyDumpTriggered() {
+  if (polling_state_.polling_task_runner &&
+      ThreadTaskRunnerHandle::Get() != polling_state_.polling_task_runner) {

[Primiano Tucci (use gerrit), 2017/03/10 10:35:39]

do you really want to compare the TaskRunnerHandles? That should be a pretty
rare need.
by looking at the code it seems to be you just want to use:

polling_state_.polling_task_runner.RunsTasksOnCurrentThread()

In general you should not rely on the fact that two calls to
ThreadTaskRunnerHandle::Get() o the same thread returns the same handle. task
runner handles are kind of proxy objects. 

Sorry I just realized that there is a similar instance below and I didn't spot
in the previous review.

[ssid, 2017/03/10 23:51:10]

Yes my bad. Fixed it.

+    polling_state_.polling_task_runner->PostTask(
+        FROM_HERE,
+        Bind(&MemoryDumpScheduler::NotifyDumpTriggered, Unretained(this)));

[Primiano Tucci (use gerrit), 2017/03/10 10:35:39]

Also I just realized that these Unretained aren't really safe. The MDScheduler
is reset OnTraceLogDisabled, so these post task might end up causing
Use-After-Free. I think you might need a weakptr here, or figure out some better
ownership semantic.

[ssid, 2017/03/10 14:57:37]

The reason why the unretained should be safe here is because of the thread.Stop
called by MDM before destroying the scheduler. There could an issue with the
task runner itself being null while posting, but that will again be handled by
MDM which takes lock while accessing scheduler, and does not call it after trace
log disabled takes away the scheduler. Only after the |dump_schefuler| is null
we call DisableTriggers which then makes the task runner null.

+    return;
+  }
+  if (polling_state_.current_state != PollingTriggerState::ENABLED)
+    return;
+
+  polling_state_.ResetTotals();
+}
+
 void MemoryDumpScheduler::DisableAllTriggers() {
   if (periodic_state_.timer.IsRunning())
     periodic_state_.timer.Stop();
   DisablePolling();
 }
 
 void MemoryDumpScheduler::DisablePolling() {
   if (ThreadTaskRunnerHandle::Get() != polling_state_.polling_task_runner) {
     if (polling_state_.polling_task_runner->PostTask(
             FROM_HERE,
             Bind(&MemoryDumpScheduler::DisablePolling, Unretained(this))))
       return;
   }
-  polling_state_.is_polling_enabled = false;
-  polling_state_.is_configured = false;
+  polling_state_.current_state = PollingTriggerState::DISABLED;
   polling_state_.polling_task_runner = nullptr;
 }
 
 // static
 void MemoryDumpScheduler::SetPollingIntervalForTesting(uint32_t interval) {
   g_polling_interval_ms_for_testing = interval;
 }
 
 bool MemoryDumpScheduler::IsPeriodicTimerRunningForTesting() {
   return periodic_state_.timer.IsRunning();
 }
 
 void MemoryDumpScheduler::RequestPeriodicGlobalDump() {
   MemoryDumpLevelOfDetail level_of_detail = MemoryDumpLevelOfDetail::BACKGROUND;
   if (periodic_state_.light_dumps_rate > 0 &&
       periodic_state_.dump_count % periodic_state_.light_dumps_rate == 0)
     level_of_detail = MemoryDumpLevelOfDetail::LIGHT;
   if (periodic_state_.heavy_dumps_rate > 0 &&
       periodic_state_.dump_count % periodic_state_.heavy_dumps_rate == 0)
     level_of_detail = MemoryDumpLevelOfDetail::DETAILED;
   ++periodic_state_.dump_count;
 
   mdm_->RequestGlobalDump(MemoryDumpType::PERIODIC_INTERVAL, level_of_detail);
 }
 
 void MemoryDumpScheduler::PollMemoryOnPollingThread() {
-  if (!polling_state_.is_configured)
+  if (polling_state_.current_state != PollingTriggerState::ENABLED)
     return;
 
   uint64_t polled_memory = 0;
   bool res = mdm_->PollFastMemoryTotal(&polled_memory);
   DCHECK(res);
   if (polling_state_.level_of_detail == MemoryDumpLevelOfDetail::DETAILED) {
     TRACE_COUNTER1(MemoryDumpManager::kTraceCategory, "PolledMemoryMB",
                    polled_memory / 1024 / 1024);
   }
 
@@ skipping 29 matching lines @@
   } else if (polling_state_.min_polls_between_dumps >
              polling_state_.num_polls_from_last_dump) {
     return false;
   }
 
   int64_t increase_from_last_dump =
       current_memory_total - polling_state_.last_dump_memory_total;
   should_dump |=
       increase_from_last_dump > polling_state_.memory_increase_threshold;
   should_dump |= IsCurrentSamplePeak(current_memory_total);
-  if (should_dump) {
-    polling_state_.last_dump_memory_total = current_memory_total;
-    polling_state_.num_polls_from_last_dump = 0;
-    for (uint32_t i = 0;
-         i < PollingTriggerState::kNumTotalsTrackedForPeakDetection; ++i) {
-      polling_state_.last_memory_totals_kb[i] = 0;
-    }
-    polling_state_.last_memory_totals_kb_index = 0;
-  }
+  if (should_dump)
+    polling_state_.ResetTotals();
   return should_dump;
 }
 
 bool MemoryDumpScheduler::IsCurrentSamplePeak(
     uint64_t current_memory_total_bytes) {
   uint64_t current_memory_total_kb = current_memory_total_bytes / 1024;
   polling_state_.last_memory_totals_kb_index =
       (polling_state_.last_memory_totals_kb_index + 1) %
       PollingTriggerState::kNumTotalsTrackedForPeakDetection;
   uint64_t mean = 0;
@@ skipping 40 matching lines @@
       heavy_dumps_rate(0),
       light_dump_period_ms(0),
       heavy_dump_period_ms(0) {}
 
 MemoryDumpScheduler::PeriodicTriggerState::~PeriodicTriggerState() {
   DCHECK(!timer.IsRunning());
 }
 
 MemoryDumpScheduler::PollingTriggerState::PollingTriggerState(
     scoped_refptr<SingleThreadTaskRunner> polling_task_runner)
-    : is_configured(false),
-      is_polling_enabled(false),
+    : current_state(DISABLED),
       level_of_detail(MemoryDumpLevelOfDetail::FIRST),
       polling_task_runner(polling_task_runner),
       polling_interval_ms(g_polling_interval_ms_for_testing
                               ? g_polling_interval_ms_for_testing
                               : kMemoryTotalsPollingInterval),
       min_polls_between_dumps(0),
       num_polls_from_last_dump(0),
       last_dump_memory_total(0),
       memory_increase_threshold(kDefaultMemoryIncreaseThreshold),
       last_memory_totals_kb_index(0) {}
 
 MemoryDumpScheduler::PollingTriggerState::~PollingTriggerState() {
   DCHECK(!polling_task_runner);
 }
 
+void MemoryDumpScheduler::PollingTriggerState::ResetTotals() {
+  if (last_memory_totals_kb_index) {
+    last_dump_memory_total =
+        last_memory_totals_kb[last_memory_totals_kb_index] * 1024;
+  }
+  num_polls_from_last_dump = 0;
+  for (uint32_t i = 0; i < kNumTotalsTrackedForPeakDetection; ++i)
+    last_memory_totals_kb[i] = 0;
+  last_memory_totals_kb_index = 0;
+}
+
 }  // namespace trace_event
 }  // namespace base