memory-infra: port peak detection logic to MemoryPeakDetector

base/trace_event/memory_peak_detector.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_peak_detector.h"
 
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/sys_info.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
+#include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/memory_dump_provider_info.h"
+#include "base/trace_event/trace_event.h"
 
 namespace base {
 namespace trace_event {
 
 // static
 MemoryPeakDetector* MemoryPeakDetector::GetInstance() {
   static MemoryPeakDetector* instance = new MemoryPeakDetector();
   return instance;
 }
 
 MemoryPeakDetector::MemoryPeakDetector()
     : generation_(0),
       state_(NOT_INITIALIZED),
-      polling_interval_ms_(0),
       poll_tasks_count_for_testing_(0) {}
 
 MemoryPeakDetector::~MemoryPeakDetector() {
   // This is hit only in tests, in which case the test is expected to TearDown()
   // cleanly and not leave the peak detector running.
   DCHECK_EQ(NOT_INITIALIZED, state_);
 }
 
 void MemoryPeakDetector::Setup(
     const GetDumpProvidersFunction& get_dump_providers_function,
     const scoped_refptr<SequencedTaskRunner>& task_runner,
     const OnPeakDetectedCallback& on_peak_detected_callback) {
   DCHECK(!get_dump_providers_function.is_null());
   DCHECK(task_runner);
   DCHECK(!on_peak_detected_callback.is_null());
   DCHECK(state_ == NOT_INITIALIZED || state_ == DISABLED);
   DCHECK(dump_providers_.empty());
   get_dump_providers_function_ = get_dump_providers_function;
   task_runner_ = task_runner;
   on_peak_detected_callback_ = on_peak_detected_callback;
   state_ = DISABLED;
+  config_ = {};
+  ResetPollHistory();
+
+  // Set threshold to 1% of total system memory.
+  static_threshold_bytes_ =
+      static_cast<uint64_t>(SysInfo::AmountOfPhysicalMemory()) / 100;
 }
 
 void MemoryPeakDetector::TearDown() {
   if (task_runner_) {
     task_runner_->PostTask(
         FROM_HERE,
         Bind(&MemoryPeakDetector::TearDownInternal, Unretained(this)));
   }
   task_runner_ = nullptr;
 }
 
-void MemoryPeakDetector::Start() {
-  task_runner_->PostTask(
-      FROM_HERE, Bind(&MemoryPeakDetector::StartInternal, Unretained(this)));
+void MemoryPeakDetector::Start(MemoryPeakDetector::Config config) {
+  if (!config.polling_interval_ms) {
+    NOTREACHED();
+    return;
+  }
+  task_runner_->PostTask(FROM_HERE, Bind(&MemoryPeakDetector::StartInternal,
+                                         Unretained(this), config));
 }
 
 void MemoryPeakDetector::Stop() {
   task_runner_->PostTask(
       FROM_HERE, Bind(&MemoryPeakDetector::StopInternal, Unretained(this)));
 }
 
+void MemoryPeakDetector::Clear() {
+  if (!task_runner_)
+    return;  // Can be called before Setup().
+  task_runner_->PostTask(
+      FROM_HERE, Bind(&MemoryPeakDetector::ResetPollHistory, Unretained(this)));
+}
+
 void MemoryPeakDetector::NotifyMemoryDumpProvidersChanged() {
-  // It is possible to call this before the first Setup() call, in which case
-  // we want to just make this a noop. The next Start() will fetch the MDP list.
   if (!task_runner_)
-    return;
+    return;  // Can be called before Setup().
   task_runner_->PostTask(
       FROM_HERE,
       Bind(&MemoryPeakDetector::ReloadDumpProvidersAndStartPollingIfNeeded,
            Unretained(this)));
 }
 
-void MemoryPeakDetector::StartInternal() {
+void MemoryPeakDetector::StartInternal(MemoryPeakDetector::Config config) {
   DCHECK_EQ(DISABLED, state_);
   state_ = ENABLED;

[ssid, 2017/04/05 21:23:46]

DCHECK_GT(polling_interval_ms, 0)
DCHECK_GT(min_time_between_peaks_ms, 0)

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

This one is already done in ::Start()
I added an extra one for safety in PollMemoryAndDetectPeak, just before the
PostDelayedTask
Nope, this one can be zero (I use it in tests). There is no reason why one
shouldn't be able to say "no throttling"

[ssid, 2017/04/10 20:55:06]

Acknowledged.

-  polling_interval_ms_ = 1;  // TODO(primiano): temporary until next CL.
+  config_ = config;
+  ResetPollHistory();

[ssid, 2017/04/05 21:26:46]

Should we test if Start() Stop() Start() happened, then the history was cleared
between Stop and Start?

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

Done. See RestartClearsState test

 
-  // If there are any dump providers available, NotifyMemoryDumpProvidersChanged
-  // will fetch them and start the polling. Otherwise this will remain in the
-  // ENABLED state and the actual polling will start on the next call to
+  // If there are any dump providers available,
+  // NotifyMemoryDumpProvidersChanged will fetch them and start the polling.
+  // Otherwise this will remain in the ENABLED state and the actual polling
+  // will start on the next call to
   // ReloadDumpProvidersAndStartPollingIfNeeded().
-  // Depending on the sandbox model, it is possible that no polling-capable dump
-  // providers will be ever available.
+  // Depending on the sandbox model, it is possible that no polling-capable
+  // dump providers will be ever available.
   ReloadDumpProvidersAndStartPollingIfNeeded();
 }
 
 void MemoryPeakDetector::StopInternal() {
   DCHECK_NE(NOT_INITIALIZED, state_);
   state_ = DISABLED;
   ++generation_;
   dump_providers_.clear();
 }
 
@@ skipping 23 matching lines @@
                            Bind(&MemoryPeakDetector::PollMemoryAndDetectPeak,
                                 Unretained(this), ++generation_));
   } else if (state_ == RUNNING && dump_providers_.empty()) {
     // Will cause the next PollMemoryAndDetectPeak() task to early return.
     state_ = ENABLED;
     ++generation_;
   }
 }
 
 void MemoryPeakDetector::PollMemoryAndDetectPeak(uint32_t expected_generation) {
-  if (state_ != RUNNING || expected_generation != generation_)
+  if (state_ != RUNNING || generation_ != expected_generation)
     return;
 
   // We should never end up in a situation where state_ == RUNNING but all dump
   // providers are gone.
   DCHECK(!dump_providers_.empty());
 
   poll_tasks_count_for_testing_++;
-  uint64_t memory_total = 0;
+  uint64_t polled_mem_bytes = 0;
   for (const scoped_refptr<MemoryDumpProviderInfo>& mdp_info :
        dump_providers_) {
     DCHECK(mdp_info->options.is_fast_polling_supported);
     uint64_t value = 0;
     mdp_info->dump_provider->PollFastMemoryTotal(&value);
-    memory_total += value;
+    polled_mem_bytes += value;
   }
-  ignore_result(memory_total);  // TODO(primiano): temporary until next CL.
+  if (config_.enable_verbose_poll_tracing) {
+    TRACE_COUNTER1(MemoryDumpManager::kTraceCategory, "PolledMemoryKB",
+                   polled_mem_bytes / 1024);

[ssid, 2017/04/05 21:23:46]

MB is easier to read in TV.

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

Ahh I see, good point. done

+  }
 
-  // TODO(primiano): Move actual peak detection logic from the
-  // MemoryDumpScheduler in next CLs.
+  // Peak detection logic. Design doc: https://goo.gl/0kOU4A .
+  bool is_peak = false;
+  if (skip_polls_ > 0) {
+    skip_polls_--;
+  } else if (last_dump_memory_total_ == 0) {
+    last_dump_memory_total_ = polled_mem_bytes;
+  } else if (polled_mem_bytes > 0) {
+    int64_t diff_from_last_dump = polled_mem_bytes - last_dump_memory_total_;
+
+    DCHECK_GT(static_threshold_bytes_, 0u);
+    is_peak = static_threshold_bytes_ &&

[ssid, 2017/04/05 21:23:46]

why this condition after dcheck above? should we not trust
SysInfo::AmountOfPhysicalMemory()? If so, we could have a default value when
this is assigned.

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

I expect AmountOfPhysicalMemory can be 0 in tests (asan bots are known to cheat
on this) but not in actual production code. If that happens I think the safest
thing is just ignorning the static threshold. 

+              (diff_from_last_dump > static_threshold_bytes_);
+
+    if (!is_peak)
+      is_peak = DetectPeakUsingSlidingWindowStddev(polled_mem_bytes);
+  }
+
+  if (is_peak) {
+    TRACE_EVENT_INSTANT1(MemoryDumpManager::kTraceCategory,
+                         "Peak memory dump detected", TRACE_EVENT_SCOPE_PROCESS,

[ssid, 2017/04/05 21:23:46]

"Peak memory detected"?
Also can we keep this in MB? It is just hard to read the values in the trace in
Kb. 2.4GB will show up as 2516582.

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

Done.

+                         "PolledMemoryKB", polled_mem_bytes / 1024);
+    on_peak_detected_callback_.Run();
+    ResetPollHistory();
+    last_dump_memory_total_ = polled_mem_bytes;
+  }
 
   SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       Bind(&MemoryPeakDetector::PollMemoryAndDetectPeak, Unretained(this),
            expected_generation),
-      TimeDelta::FromMilliseconds(polling_interval_ms_));
+      TimeDelta::FromMilliseconds(config_.polling_interval_ms));
+}
+
+bool MemoryPeakDetector::DetectPeakUsingSlidingWindowStddev(
+    uint64_t polled_mem_bytes) {
+  DCHECK(polled_mem_bytes);
+  samples_bytes_[samples_index_] = polled_mem_bytes;
+  samples_index_ = (samples_index_ + 1) % kSlidingWindowNumSamples;
+  uint64_t mean = 0;
+  for (uint32_t i = 0; i < kSlidingWindowNumSamples; ++i) {
+    if (samples_bytes_[i] == 0)
+      return false;  // Not enough samples to detect peaks.
+    mean += samples_bytes_[i];
+  }
+  mean /= kSlidingWindowNumSamples;
+  uint64_t variance = 0;

[ssid, 2017/04/05 21:23:46]

Either make this double or make the sample kb.
A sample of 1GB change will cause
2^30 * 2^30 variance.

If we have 4 such ups and downs over the last 50 samples, then we can easily hit
2^64.
Back to the previous discussion, maybe double is just better to avoid such
confusion.

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

Oh excellent point.
Yeah agreed. My obsession for keeping integer arith was becoming too painful
here.
switched to floats.

+  for (uint32_t i = 0; i < kSlidingWindowNumSamples; ++i) {
+    const uint64_t deviation = samples_bytes_[i] - mean;
+    variance += deviation * deviation;
+  }
+  variance /= kSlidingWindowNumSamples;
+
+  // If stddev is less than 0.2% then we consider that the process is inactive.
+  if (variance < (mean / 500) * (mean / 500))
+    return false;
+
+  // (mean + 3.69 * stddev) corresponds to a value that is higher than current
+  // sample with 99.99% probability.
+  const uint64_t cur_sample_deviation = polled_mem_bytes - mean;
+  return cur_sample_deviation * cur_sample_deviation > (3.69 * 3.69 * variance);
+}
+
+void MemoryPeakDetector::ResetPollHistory() {
+  skip_polls_ = 0;
+  last_dump_memory_total_ = 0;

[ssid, 2017/04/05 21:23:46]

hm shouldn't this be set to the current value instead?
If browser triggers a dump and renderer is polling, it should be notified that
the dump was triggered at current value instead of 0.
Also, if we sometime get to a point where we use peak and periodic dumps
together, then it is better to track when the last dump was triggered.
So, the existing code sets this to previous sample if it exists.

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

Does it make a difference in practice? If I set this to 0, the next poll will
take the polled value as initial.
So we really lose the increase beween the reset and the next poll, which is like
1 ms. 
Or is there asomething more I am not thinking about?
Not sure I understand this. Are you suggesting that Clear() sholuld take an
argument? THat feels a bit of a weird api.
Also, as above, this makes a difference only for one sample ? Or am I missing
something?

[ssid, 2017/04/10 20:55:06]

The issue is: If we detect peak in browser and renderer (as happens in Android),
let's say browser detected a peak at t1. The renderer process peak detector gets
notified that dump was triggered at t1. Renderer last_memory_total = 0;. If we
specified the min time between dumps as 2 seconds in trace config, then after 2
seconds renderer peak detector will set value of last total = current value,
since we set it after skip_polls are done. What we should do here is set the
value to the current total when notify is called.

I suggested setting the value here since we should always remember to do this
reset in the next poll. It is just clean to set it to previous value in the
notify call.

[Primiano Tucci (use gerrit), 2017/04/11 11:12:09]

Okay I changes this, but still not sure this makes a huge difference.
The only difference would be if in the last 2 seconds the renderer had a jump >
the static threshold.
The stddev is unaffected in practice, because we are just talking about keeping
an old sample (Which will last 1ms) in the window or not.

W.r.t. the case you mentioned above, I wonder if clearing the history is really
the best thing to do, or whether we should just fill the skip_polls_ and keep
monitoring memory.
Or maybe we should probably have one instance of peak detector per process (all
of them in the browser process). Let's rediscuss this in future, there is no
rush now. For the moment will do what you say as doesn't make a huge difference
anyways.

Anyhow, don't want to get this blocked on this decision that we can improve
later. Let's get this landed and we can improve subsequently, but I can unblock
the rest of the work in the meantime.

+  memset(samples_bytes_, 0, sizeof(samples_bytes_));
+  samples_index_ = 0;
+  if (config_.polling_interval_ms > 0) {

[ssid, 2017/04/05 21:23:46]

Maybe we should check for state_ == running or enabled. Since the comment on
config_ says valid only if state_ is this.
Also if enabled/running,
last_dump_memory_total = samples_[sample_index];

[Primiano Tucci (use gerrit), 2017/04/06 20:07:10]

No I use this also to clearup during Setup().
Does it make a difference in practice? other than getting an extra sample for
the next turn?
If I do this, then it becomes awkward because I have to clearup separately
samples_ at Setup (while here I am using this also for clear at Setup)

[ssid, 2017/04/10 20:55:06]

hm that is true. Maybe we could just do it in next poll.

[Primiano Tucci (use gerrit), 2017/04/11 11:12:09]

I just added a bool bool keep_last_sample here, so we can either clear all or
keep the last value.

+    skip_polls_ =
+        (config_.min_time_between_peaks_ms + config_.polling_interval_ms - 1) /
+        config_.polling_interval_ms;
+  }
+}
+
+void MemoryPeakDetector::SetStaticThresholdForTesting(
+    uint64_t static_threshold_bytes) {
+  DCHECK_EQ(DISABLED, state_);
+  static_threshold_bytes_ = static_threshold_bytes;
 }
 
 }  // namespace trace_event
 }  // namespace base