[memory-infra] Implement peak detection logic

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
index 9b9933b1c986c976dcb107d70e17d118bd63c13b..0f65e094acda9a67458626a80e2169aa004f2ad1 100644
--- a/base/trace_event/memory_dump_scheduler.cc
+++ b/base/trace_event/memory_dump_scheduler.cc
@@ -4,6 +4,9 @@
 
 #include "base/trace_event/memory_dump_scheduler.h"
 
+#include <math.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"
@@ -15,15 +18,26 @@ namespace trace_event {
 namespace {
 // Threshold on increase in memory from last dump beyond which a new dump must
 // be triggered.
-int64_t kMemoryIncreaseThreshold = 50 * 1024 * 1024;  // 50MiB
+int64_t kDefaultMemoryIncreaseThreshold = 50 * 1024 * 1024;  // 50MiB
 const uint32_t kMemoryTotalsPollingInterval = 25;
 uint32_t g_polling_interval_ms_for_testing = 0;
+const unsigned kNumTotalsTrackedForPeakDetection = 50;
+
 }  // namespace
 
 MemoryDumpScheduler::MemoryDumpScheduler(
     MemoryDumpManager* mdm,
     scoped_refptr<SingleThreadTaskRunner> polling_task_runner)
-    : mdm_(mdm), polling_state_(polling_task_runner) {}
+    : mdm_(mdm), polling_state_(polling_task_runner) {
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) || \

[Primiano Tucci (use gerrit), 2017/03/09 18:31:27]

isn't easier to say which one you don't want?
Is this just a way to say !NACL, in which case just #if !defined(OS_NACL) :)

[ssid, 2017/03/09 19:32:44]

GetSystemMemoryInfo is defined under this same macro. So, using the same. I
might break tracing for BSD or some os.

+    defined(OS_ANDROID)
+  // Set threshold to 1% of total system memory.
+  SystemMemoryInfoKB meminfo;
+  bool res = GetSystemMemoryInfo(&meminfo);
+  if (res)
+    polling_state_.memory_increase_threshold = (meminfo.total / 100) * 1024;
+#endif
+}
 
 MemoryDumpScheduler::~MemoryDumpScheduler() {}
 
@@ -85,6 +99,7 @@ void MemoryDumpScheduler::NotifyPollingSupported() {
   if (!polling_state_.is_configured || polling_state_.is_polling_enabled)
     return;
   polling_state_.is_polling_enabled = true;
+  polling_state_.last_n_memory_totals.clear();
   polling_state_.num_polls_from_last_dump = 0;
   polling_state_.last_dump_memory_total = 0;
   polling_state_.polling_task_runner->PostTask(
@@ -180,14 +195,47 @@ bool MemoryDumpScheduler::ShouldTriggerDump(uint64_t current_memory_total) {
 
   int64_t increase_from_last_dump =
       current_memory_total - polling_state_.last_dump_memory_total;
-  should_dump |= increase_from_last_dump > kMemoryIncreaseThreshold;
+  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;
+    polling_state_.last_n_memory_totals.clear();
   }
   return should_dump;
 }
 
+bool MemoryDumpScheduler::IsCurrentSamplePeak(uint64_t current_memory_total) {
+  if (polling_state_.last_n_memory_totals.size() <
+      kNumTotalsTrackedForPeakDetection) {
+    polling_state_.last_n_memory_totals.push_back(current_memory_total);
+    return false;
+  } else if (polling_state_.last_n_memory_totals.size() >
+             kNumTotalsTrackedForPeakDetection) {
+    polling_state_.last_n_memory_totals.pop_front();
+  }
+
+  double mean = 0;

[Primiano Tucci (use gerrit), 2017/03/09 18:31:27]

never like too much to use floating point arith if we really don't need to.
Can you just use uint64_t for this?

[ssid, 2017/03/09 19:32:44]

Done. I liked double because i didn't have to worry about overflow.

[Primiano Tucci (use gerrit), 2017/03/10 10:19:32]

ah are definitely right. I didn't realize that you have anyways the 3.69 down
there. sorry for wasting your time :/
Anything you want to do works, if you want to go back to the fp math, you are
completely right, there are less things to worry about. I guess I just didn't
look well enough to the code before adding that comment. 

[ssid, 2017/03/10 19:56:10]

Weel now that i accounted for the overflow, it should be better than sqrt calls.
I guess i will just use the variance calculations.

+  for (uint64_t sample : polling_state_.last_n_memory_totals)
+    mean += sample;
+  mean = mean / kNumTotalsTrackedForPeakDetection;

[Primiano Tucci (use gerrit), 2017/03/09 18:31:27]

hmm this is not true until you get the initial kNum samples.
I think you want to keep a count in the loop for the non-0 alues and divide by
that (see my other comment about getting rid of th deque)

[ssid, 2017/03/09 19:32:44]

If I don't have N samples then the func returns false in the start. So, only
detect peak when we have enough samples. added a comment now.

[Primiano Tucci (use gerrit), 2017/03/10 10:19:32]

ah righ. ack

+  double stddev = 0;
+  for (uint64_t sample : polling_state_.last_n_memory_totals)
+    stddev += (sample - mean) * (sample - mean);
+  stddev = sqrt(stddev / kNumTotalsTrackedForPeakDetection);

[Primiano Tucci (use gerrit), 2017/03/09 18:31:27]

same here, would be great if instead of the actual stddev you kept the ^2
uint64_t value and adapt the logic to reason on the squared value (unless makes
anything too crazy).
On android I am not even sure we target the hardfp ABI, which means that we
might end up using some software fallback of this.

[ssid, 2017/03/09 19:32:44]

Done.

+
+  polling_state_.last_n_memory_totals.push_back(current_memory_total);
+
+  // if stddev is less than 0.2% then we consider that the process is inactive.
+  bool is_stddev_low = stddev < mean / 500;
+  if (is_stddev_low)
+    return false;
+  // 3.69 corresponds to a value that is higher than current sample with 99.99%
+  // probability.
+  return current_memory_total > (mean + 3.69 * stddev);
+}
+
 MemoryDumpScheduler::PeriodicTriggerState::PeriodicTriggerState()
     : is_configured(false),
       dump_count(0),
@@ -212,7 +260,8 @@ MemoryDumpScheduler::PollingTriggerState::PollingTriggerState(
                               : kMemoryTotalsPollingInterval),
       min_polls_between_dumps(0),
       num_polls_from_last_dump(0),
-      last_dump_memory_total(0) {}
+      last_dump_memory_total(0),
+      memory_increase_threshold(kDefaultMemoryIncreaseThreshold) {}
 
 MemoryDumpScheduler::PollingTriggerState::~PollingTriggerState() {
   DCHECK(!polling_task_runner);