memory-infra: port peak detection logic to MemoryPeakDetector

base/trace_event/memory_peak_detector.h

 // 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.
 
 #ifndef BASE_TRACE_EVENT_MEMORY_PEAK_DETECTOR_H_
 #define BASE_TRACE_EVENT_MEMORY_PEAK_DETECTOR_H_
 
 #include <stdint.h>
 
 #include <memory>
 #include <vector>
 
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 
 namespace base {
 
 class SequencedTaskRunner;
 
 namespace trace_event {
 
 struct MemoryDumpProviderInfo;
 
+// Detects temporally local memory peaks. Peak detection is based on
+// continuously querying memory usage using MemoryDumpprovider(s) that support
+// fast polling (e.g., ProcessMetricsDumpProvider which under the hoods reads
+// /proc/PID/statm on Linux) and using a cobination of:
+// - An static threshold (currently 1% of total system memory).
+// - Sliding window stddev analysis.
+// Design doc: https://goo.gl/0kOU4A .
 // This class is NOT thread-safe, the caller has to ensure linearization of
 // the calls to the public methods. In any case, the public methods do NOT have
 // to be called from the |task_runner| on which the polling tasks run.
 class BASE_EXPORT MemoryPeakDetector {
  public:
   using OnPeakDetectedCallback = RepeatingClosure;
   using DumpProvidersList = std::vector<scoped_refptr<MemoryDumpProviderInfo>>;
   using GetDumpProvidersFunction = RepeatingCallback<void(DumpProvidersList*)>;
 
   enum State {
     NOT_INITIALIZED = 0,  // Before Setup()
     DISABLED,             // Before Start() or after Stop().
     ENABLED,              // After Start() but no dump_providers_ are available.
     RUNNING  // After Start(). The PollMemoryAndDetectPeak() task is scheduled.
   };
 
+  // Peak detector configuration, passed to Start().
+  struct Config {
+    uint32_t polling_interval_ms;
+    uint32_t min_time_between_peaks_ms;
+    bool enable_verbose_poll_tracing;

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

Nice to have some comment on the config members.

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

done

+  };
+
   static MemoryPeakDetector* GetInstance();
 
   // Configures the peak detector, binding the polling tasks on the given
   // thread. Setup() can be called several times, provided that: (1) Stop()
   // is called; (2a) the previous task_runner is flushed or (2b) the task_runner
   // remains the same.
   // GetDumpProvidersFunction: is the function that will be invoked to get
   //   an updated list of polling-capable dump providers. This is really just
   //   MemoryDumpManager::GetDumpProvidersForPolling, but this extra level of
   //   indirection allows easier testing.
   // SequencedTaskRunner: the task runner where PollMemoryAndDetectPeak() will
   //  be periodically called.
   // OnPeakDetectedCallback: a callback that will be invoked on the
   //   given task runner when a memory peak is detected.
   void Setup(const GetDumpProvidersFunction&,
              const scoped_refptr<SequencedTaskRunner>&,
              const OnPeakDetectedCallback&);
 
   // Releases the |task_runner_| and the bound callbacks.
   void TearDown();
 
   // This posts a task onto the passed task runner which refreshes the list of
   // dump providers via the GetDumpProvidersFunction. If at least one dump
   // provider is available, this starts immediately polling on the task runner.
   // If not, the detector remains in the ENABLED state and will start polling
   // automatically (i.e. without requiring another call to Start()) on the
   // next call to NotifyMemoryDumpProvidersChanged().
-  void Start();
+  void Start(Config);
 
   // Stops the polling on the task runner (if was active at all). This doesn't
   // wait for the task runner to drain pending tasks, so it is possible that
   // a polling will happen concurrently (or in the immediate future) with the
   // Stop() call. It is responsibility of the caller to drain or synchronize
   // with the task runner.
   void Stop();
 
+  // Clears the history of the peak detector, both for the sliding window and
+  // static threshold logic, if Start()-ed, otherwise it's a no-op. The intended
+  // use case is to prevent that a memory dump is triggered due to peak
+  // detection soon after another dump has been triggered for other reasons.
+  void Clear();

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

MemoryPeakDetector::Clear() sounds like this would clear the detector after
stopping. If this is the only use case why not NotifyDumpTriggered?
Also it is useful to set the last total at the notify instead of just resetting
results.
Also, you'd not have to write such elaborate comment about it if it's just
notify they are impl details.

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

But the comment says "If start()-ed, otherwise it's a noop". Isn't that clear
enough?
Hm not sure I get this part. Can you expand? I guess I am missing some part of
the logic?
NotifyDumpTriggered is kind of weird to see here. First of all this class
doesn't know anything anymore about dumps. This just gives a callback when a
peak is detected. If I say "hey peak detector, I notify you I just created a
memory dump" my reaction is "so what?".

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

I tried to explain this in the comment below.
Okay in that case the main use case of the Clear() is to reset the skip_polls_.
That is the state that prevents dumps from being created immediately. Clearing
of the sliding window is required because we skip the polls and we will have an
inconsistent window. Resetting the static threshold is required because a dump
was triggered at the current total. Maybe this function should be named
NotifyPeakDetected()? But if we did that then in future if we wanted to delay
the dump triggering because of a periodic dump then it'd not make sense. So,
maybe ResetTimer ? I can't think of a better name.

In any case looking at the comment, my question is why should peak detector have
an api to clear the sliding window(implementation detail). If this function is
to stop dump from being created then that should only be in the comment.

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

See my other comment. That suggests to me that we should keep accumulating the
window in the Poll function regardless of the |skip|, and then just not do
dumps. Let's discuss this in a future cl.
Okay good point. Reanemed to Throttle() and made the comment less
implementation-dependent.

+
   // Used by MemoryDumpManager to notify that the list of polling-capable dump
   // providers has changed. The peak detector will reload the list on the next
   // polling task. This function can be called before Setup(), in which
   // case will be just a no-op.
   void NotifyMemoryDumpProvidersChanged();
 
+  void SetStaticThresholdForTesting(uint64_t static_threshold_bytes);
+
  private:
   friend class MemoryPeakDetectorTest;
 
+  static constexpr uint32_t kSlidingWindowNumSamples = 50;
+
   MemoryPeakDetector();
   ~MemoryPeakDetector();
 
   // All these methods are always called on the |task_runner_|.
-  void StartInternal();
+  void StartInternal(Config);
   void StopInternal();
   void TearDownInternal();
   void ReloadDumpProvidersAndStartPollingIfNeeded();
   void PollMemoryAndDetectPeak(uint32_t expected_generation);
+  bool DetectPeakUsingSlidingWindowStddev(uint64_t last_sample_bytes);
+  void ResetPollHistory();
 
   // It is safe to call these testing methods only on the |task_runner_|.
   State state_for_testing() const { return state_; }
   uint32_t poll_tasks_count_for_testing() const {
     return poll_tasks_count_for_testing_;
   }
 
   // The task runner where all the internal calls are posted onto. This field
   // must be NOT be accessed by the tasks posted on the |task_runner_| because
   // there might still be outstanding tasks on the |task_runner_| while this
@@ skipping 14 matching lines @@
   DumpProvidersList dump_providers_;
 
   // The generation is incremented every time the |state_| is changed and causes
   // PollMemoryAndDetectPeak() to early out if the posted task doesn't match the
   // most recent |generation_|. This allows to drop on the floor outstanding
   // PostDelayedTask that refer to an old sequence that was later Stop()-ed or
   // disabled because of NotifyMemoryDumpProvidersChanged().
   uint32_t generation_;
 
   State state_;
-  uint32_t polling_interval_ms_;
+
+  // Config passed to Start(), only valid when |state_| = {ENABLED, RUNNING}.
+  Config config_;
+
+  int64_t static_threshold_bytes_;
+  uint32_t skip_polls_;
+  uint64_t last_dump_memory_total_;
+  uint64_t samples_bytes_[kSlidingWindowNumSamples];

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

Samples were stored in kb because of the overflows in stddev calculation.

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

I moved the stddev to floats, should be fine now.

+  uint32_t samples_index_;
   uint32_t poll_tasks_count_for_testing_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryPeakDetector);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_PEAK_DETECTOR_H_