[tracing] Implement polling in MemoryDumpManager

base/trace_event/memory_dump_scheduler.cc

Index: base/trace_event/memory_dump_scheduler.cc
diff --git a/base/trace_event/memory_dump_scheduler.cc b/base/trace_event/memory_dump_scheduler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1baa6ef45c4e2fbffe1e83d52739e68efc27804b
--- /dev/null
+++ b/base/trace_event/memory_dump_scheduler.cc
@@ -0,0 +1,246 @@
+// 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"
+#include "build/build_config.h"
+
+namespace base {
+namespace trace_event {
+
+namespace {
+const uint32_t kMemoryTotalsPollingInterval = 25;
+uint32_t g_polling_interval_for_testing = 0;
+}  // namespace
+
+MemoryDumpScheduler::MemoryDumpScheduler(
+    MemoryDumpManager* mdm,
+    scoped_refptr<SingleThreadTaskRunner> polling_task_runner)
+    : periodic_dump_scheduler_(mdm),
+      peak_dump_scheduler_(mdm, polling_task_runner) {}
+
+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_dump_scheduler_.is_setup());
+    peak_dump_scheduler_.AddTrigger(level_of_detail, min_time_between_dumps_ms);
+  } else if (trigger_type == MemoryDumpType::PERIODIC_INTERVAL) {
+    DCHECK(!peak_dump_scheduler_.is_setup());
+    periodic_dump_scheduler_.AddTrigger(level_of_detail,
+                                        min_time_between_dumps_ms);
+  }
+}
+
+void MemoryDumpScheduler::NotifyPeriodicTriggerSupported() {
+  periodic_dump_scheduler_.NotifySupported();
+}
+
+void MemoryDumpScheduler::NotifyPollingSupported() {
+  peak_dump_scheduler_.NotifySupported();
+}
+
+void MemoryDumpScheduler::DisableAllTriggers() {
+  periodic_dump_scheduler_.Disable();
+  peak_dump_scheduler_.Disable();
+}
+
+// static
+void MemoryDumpScheduler::SetPollingIntervalForTesting(uint32_t interval) {
+  g_polling_interval_for_testing = interval;
+}
+
+bool MemoryDumpScheduler::IsPeriodicTimerRunningForTesting() {
+  return periodic_dump_scheduler_.IsTimerRunning();
+}
+
+MemoryDumpScheduler::PeriodicDumpScheduler::PeriodicDumpScheduler(
+    MemoryDumpManager* mdm)
+    : mdm_(mdm),
+      is_setup_(false),
+      dump_count_(0),
+      min_timer_period_ms_(std::numeric_limits<uint32_t>::max()),
+      light_dumps_rate_(0),
+      heavy_dumps_rate_(0),
+      light_dump_period_ms(0),
+      heavy_dump_period_ms(0) {}
+
+MemoryDumpScheduler::PeriodicDumpScheduler::~PeriodicDumpScheduler() {
+  DCHECK(!IsTimerRunning());
+}
+
+void MemoryDumpScheduler::PeriodicDumpScheduler::AddTrigger(
+    MemoryDumpLevelOfDetail level_of_detail,
+    uint32_t min_time_between_dumps_ms) {
+  is_setup_ = true;
+  DCHECK_NE(0u, min_time_between_dumps_ms);
+  switch (level_of_detail) {
+    case MemoryDumpLevelOfDetail::BACKGROUND:
+      break;
+    case MemoryDumpLevelOfDetail::LIGHT:
+      DCHECK_EQ(0u, light_dump_period_ms);
+      light_dump_period_ms = min_time_between_dumps_ms;
+      break;
+    case MemoryDumpLevelOfDetail::DETAILED:
+      DCHECK_EQ(0u, heavy_dump_period_ms);
+      heavy_dump_period_ms = min_time_between_dumps_ms;
+      break;
+  }
+  min_timer_period_ms_ =
+      std::min(min_timer_period_ms_, min_time_between_dumps_ms);
+}
+
+void MemoryDumpScheduler::PeriodicDumpScheduler::NotifySupported() {
+  if (!is_setup_)
+    return;
+  DCHECK_EQ(0u, light_dump_period_ms % min_timer_period_ms_);

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

I'd move these checks to AddTrigger, so the stacktrace will shop up at the time
we read them form the config, and not only later when we enable this.
here you can leave just a check min_timer_period > 0, or similar

[ssid, 2017/02/14 02:05:40]

Um, we will be doing the check multiple times when we can do it at the end. But
its okay.
  

+  light_dumps_rate_ = light_dump_period_ms / min_timer_period_ms_;
+  DCHECK_EQ(0u, heavy_dump_period_ms % min_timer_period_ms_);
+  heavy_dumps_rate_ = heavy_dump_period_ms / min_timer_period_ms_;
+
+  dump_count_ = 0;
+  timer_.Start(FROM_HERE, TimeDelta::FromMilliseconds(min_timer_period_ms_),
+               Bind(&MemoryDumpScheduler::PeriodicDumpScheduler::
+                        RequestPeriodicGlobalDump,
+                    Unretained(this)));
+}
+
+void MemoryDumpScheduler::PeriodicDumpScheduler::Disable() {
+  if (timer_.IsRunning())
+    timer_.Stop();
+}
+
+void MemoryDumpScheduler::PeriodicDumpScheduler::RequestPeriodicGlobalDump() {
+  MemoryDumpLevelOfDetail level_of_detail = MemoryDumpLevelOfDetail::BACKGROUND;
+  if (light_dumps_rate_ > 0 && dump_count_ % light_dumps_rate_ == 0)
+    level_of_detail = MemoryDumpLevelOfDetail::LIGHT;
+  if (heavy_dumps_rate_ > 0 && dump_count_ % heavy_dumps_rate_ == 0)
+    level_of_detail = MemoryDumpLevelOfDetail::DETAILED;
+  ++dump_count_;
+
+  mdm_->RequestGlobalDump(MemoryDumpType::PERIODIC_INTERVAL, level_of_detail);
+}
+
+MemoryDumpScheduler::PeakDumpScheduler::PeakDumpScheduler(
+    MemoryDumpManager* mdm,
+    scoped_refptr<SingleThreadTaskRunner> polling_task_runner)
+    : mdm_(mdm),
+      is_setup_(0),
+      dump_mode_(MemoryDumpLevelOfDetail::FIRST),
+      polling_task_runner_(polling_task_runner),
+      polling_interval_(g_polling_interval_for_testing
+                            ? g_polling_interval_for_testing
+                            : kMemoryTotalsPollingInterval),
+      min_polls_between_dumps_(0),
+      num_polls_from_last_dump_(0),
+      last_dump_memory_total_(0) {
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) || \
+    defined(OS_ANDROID)
+  // Set threshold to 1% of total system memory.
+  SystemMemoryInfoKB meminfo;
+  bool res = GetSystemMemoryInfo(&meminfo);
+  if (res)
+    memory_increase_threshold_ = (meminfo.total / 100) * 1024;
+#else
+  memory_increase_threshold_ = 50 * 1024 * 1024;  // 50MiB
+#endif
+}
+
+MemoryDumpScheduler::PeakDumpScheduler::~PeakDumpScheduler() {
+  DCHECK(!polling_task_runner_) << "Polling was not disabled safely";

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

remove the string comment, the code in the dcheck seems clear enough

[ssid, 2017/02/14 02:05:40]

Done.

+}
+
+void MemoryDumpScheduler::PeakDumpScheduler::AddTrigger(

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

shouldn't this method just be called Configure() or Initialize().
You can't support more than 1 trigger here really

[ssid, 2017/02/14 02:05:40]

Removed this method since this is a struct.

+    MemoryDumpLevelOfDetail level_of_detail,
+    uint32_t min_time_between_dumps_ms) {
+  DCHECK(!is_setup_);
+  DCHECK_NE(0u, min_time_between_dumps_ms);
+
+  dump_mode_ = level_of_detail;

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

just call the variable level_of_detail_ ? never been a huge fan of variables
that change name along the road

[ssid, 2017/02/14 02:05:40]

Done.

+  min_polls_between_dumps_ =
+      (min_time_between_dumps_ms + polling_interval_ - 1) / polling_interval_;
+  is_setup_ = true;

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

s/is_setup_/is_initialized_/ (or is_configured_)

[ssid, 2017/02/14 02:05:40]

Done.

+}
+
+void MemoryDumpScheduler::PeakDumpScheduler::NotifySupported() {
+  if (!is_setup_)
+    return;
+  num_polls_from_last_dump_ = 0;

[Primiano Tucci (use gerrit), 2017/02/03 20:08:20]

can you add some logic (either a check or a return) to prevent that  this
doesn't et accidentally called twice, firing 2x timers

[ssid, 2017/02/14 02:05:40]

Done.

+  last_dump_memory_total_ = 0;
+  polling_task_runner_->PostTask(
+      FROM_HERE,
+      Bind(&MemoryDumpScheduler::PeakDumpScheduler::PollMemoryOnPollingThread,
+           Unretained(this)));
+}
+
+void MemoryDumpScheduler::PeakDumpScheduler::Disable() {
+  if (ThreadTaskRunnerHandle::Get() != polling_task_runner_) {
+    if (polling_task_runner_->PostTask(
+            FROM_HERE, Bind(&MemoryDumpScheduler::PeakDumpScheduler::Disable,
+                            Unretained(this))))
+      return;
+  }
+  is_setup_ = false;
+  polling_task_runner_ = nullptr;
+}
+
+void MemoryDumpScheduler::PeakDumpScheduler::PollMemoryOnPollingThread() {
+  if (!is_setup_)
+    return;
+
+  uint64_t metric = 0;
+  bool res = mdm_->PollFastMemoryTotal(&metric);
+  DCHECK(res);
+  if (dump_mode_ == MemoryDumpLevelOfDetail::DETAILED) {
+    TRACE_COUNTER1(MemoryDumpManager::kTraceCategory, "MemoryMetricMB",

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

MemoryMetricMB -> PolledMemoryMB ?

[ssid, 2017/02/14 02:05:40]

Done.

+                   metric / 1024 / 1024);
+  }
+
+  if (ShouldTriggerDump(metric))
+    mdm_->RequestGlobalDump(MemoryDumpType::PEAK_MEMORY_USAGE, dump_mode_);
+
+  // TODO(ssid): Use RequestSchedulerCallback, crbug.com/607533.
+  ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

out of curiosity why reposting instead of using Time(is_repeating=true)? (no
need to change it here, can do in a follow up CL, if this make sense)

[ssid, 2017/02/14 02:05:40]

I did not use timer here because in future we plan to do smarter polling based
on background processes.

+      Bind(&MemoryDumpScheduler::PeakDumpScheduler::PollMemoryOnPollingThread,
+           Unretained(this)),
+      TimeDelta::FromMilliseconds(polling_interval_));
+}
+
+bool MemoryDumpScheduler::PeakDumpScheduler::ShouldTriggerDump(
+    uint64_t current_memory_total) {
+  if (current_memory_total == 0)
+    return false;
+
+  bool should_dump = false;
+  ++num_polls_from_last_dump_;

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

can you make all this in early-out style, without using should_dump? might be
easier to follow. e.g.,

++num_polls_from_last_dump_;
if (num_polls_from_last_dump_ < min_polls_between_dumps_) 
  return false;
...
if (last_dump_memory_total && increase_from_last_dump <
memory_increase_threshold_)
  return false;

TRACE_EVENT...
return true

[ssid, 2017/02/14 02:05:40]

The code would look like:
  if (polling_state_.last_dump_memory_total == 0) {
    polling_state_.last_dump_memory_total = current_memory_total;
    return true;
  }

  ++polling_state_.num_polls_from_last_dump;
  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;
  if (increase_from_last_dump < polling_state_.memory_increase_threshold)
    return false;

  polling_state_.last_dump_memory_total = current_memory_total;
  polling_state_.num_polls_from_last_dump = 0;
  return true;


I don't like the ides of returning true from 2 places and the fact that we have
to reset few values when returning true makes it weird. So keeping the current
pattern.
I can't do early out because of the fact that I want to return true for the
first poll and also set the last_total's value.

+  if (last_dump_memory_total_ == 0) {
+    // If it's first sample then trigger memory dump.
+    should_dump |= true;

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

well, really no need for the |= here :) just = true should be enough

[ssid, 2017/02/14 02:05:40]

Done.

+  } else if (min_polls_between_dumps_ > num_polls_from_last_dump_) {
+    return false;
+  }
+
+  int64_t increase_from_last_dump =
+      current_memory_total - last_dump_memory_total_;
+  should_dump |= increase_from_last_dump > memory_increase_threshold_;
+  if (should_dump) {
+    last_dump_memory_total_ = current_memory_total;
+    num_polls_from_last_dump_ = 0;
+    TRACE_EVENT_INSTANT1(MemoryDumpManager::kTraceCategory,
+                         "Peak memory dump Triggered",

[Primiano Tucci (use gerrit), 2017/02/03 20:08:19]

I'd move this above in Poll...OnThread, so this is emitted in the same function
of the other one.

[ssid, 2017/02/14 02:05:40]

Done.

+                         TRACE_EVENT_SCOPE_PROCESS, "total_usage_MB",
+                         current_memory_total / 1024 / 1024);
+  }
+  return should_dump;
+}
+
+}  // namespace trace_event
+}  // namespace base