memory-infra: Add peak-detector skeleton.

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"
+#include "base/sequence_checker.h"
+
+namespace base {
+
+class SequencedTaskRunner;
+
+namespace trace_event {
+
+struct MemoryDumpProviderInfo;
+
+// This class is NOT thread-safe, the public methods must be called on the same
+// thread, which does not have to be |task_runner| on which the polling tasks

[ssid, 2017/03/31 18:03:35]

How are you planning to ensure this?
Which thread is MDM going to use when starting polling? MDM needs to store an
extra task runner for this? or it is going to postTask all methods of this class
to dump thread?
I think the issue here is that dump providers can be registered on any thread
and calling NotifyChanged() should be posted everytime.
What will we do for the case where dump thread is not yet created?
Maybe you are planning to store the thread that created MDM just for posting
tasks to this class?!

[Primiano Tucci (use gerrit), 2017/03/31 20:01:54]

Oh you are right. Nah there is no need to post, because all these public methods
here just post under the hoods. I just removed the thread checker and specified
that the caller (MDM) will have to take care of linearization.
Essentially means just calling these from a lock.
I though to this this morning when I designed all this, but then I must have
forgot at some point later when I added the seqchecker . too much used to be
paranoid I guess :)

+// run (see Start()).

[ssid, 2017/03/31 18:03:35]

Start() does not have any documentation.

[Primiano Tucci (use gerrit), 2017/03/31 20:01:55]

now it does :)

+class BASE_EXPORT MemoryPeakDetector {
+ public:
+  using OnPeakDetectedCallback = RepeatingClosure;
+  using DumpProvidersList = std::vector<scoped_refptr<MemoryDumpProviderInfo>>;
+  using GetDumpProvidersFunction = RepeatingCallback<void(DumpProvidersList*)>;
+
+  enum State {

[ssid, 2017/03/31 18:03:35]

I think "State" is a confusing name here. Especially because of the existing
structs in scheduler names PollingState. But, i can't think of better name. I
guess it's fine.

[Primiano Tucci (use gerrit), 2017/03/31 20:01:54]

It's going to be better once the polling part of the scheduler is no more and
we'll have only MemoryPeakDetector and PeriodicMemorySampler (or something like
that)

+    NOT_INITIALIZED = 0,  // Before Initialize()
+    DISABLED,             // Before Start() or after Stop().
+    ENABLED,              // After Start() but no dump_providers_ are available.
+    RUNNING  // After Start(). The PollMemoryAndDetectPeak() task is scheduled.
+  };
+
+  static MemoryPeakDetector* GetInstance();
+
+  // 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 same
+  //   task runner) when a memory peak is detected.
+  void Initialize(const GetDumpProvidersFunction&,
+                  const scoped_refptr<SequencedTaskRunner>&,

[ssid, 2017/03/31 18:03:35]

Is there any advantage of using sequenced task runner here?

[Primiano Tucci (use gerrit), 2017/03/31 20:01:54]

nah just being future proof in case, in future, we want to move to a blocking
pool (which is sequenced but not singlethread IIRC)

+                  const OnPeakDetectedCallback&);

[ssid, 2017/03/31 18:03:35]

Maybe this callback shouldn't be part of Initialize().
We would anyway need another function to set up peak detection parameters like
time interval and modes. So, this can be paramter to Start() along with the
others that you'll add in future.
Essentially we need the flexibility of changing the callback every time we start
polling.

[Primiano Tucci (use gerrit), 2017/03/31 20:01:55]

Isn't the callback to be always the same though (either short-circuited to
ReqGlobalDump in the near term or sent to the service)?.
Let's see how this evolved in future

[ssid, 2017/04/03 17:55:27]

hm callback can be different. for instance for slow reports we will setup with
BACKGROUND mode or DETAILED mode in normal case.
Also supporting multiple Initialize() solves this issue. But, just not happy
with calling initialize() twice on a singleton. Maybe this can be names Setup()?

[Primiano Tucci (use gerrit), 2017/04/03 20:28:15]

I think we should deal with that within the callback.
Yeah but it's overkill really. That complexity can be handled in the callback
with one state variable.
Done

+  void TearDownForTesting();
+
+  void Start();
+  void Stop();
+
+  // 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.
+  void NotifyMemoryDumpProvidersChanged();
+
+ private:
+  friend class MemoryPeakDetectorTest;
+
+  MemoryPeakDetector();
+  ~MemoryPeakDetector();
+
+  // All these methods are always called on the |task_runner_|.
+  void StartInternal();
+  void StopInternal();
+  void ReloadDumpProvidersAndStartPollingIfNeeded();
+  void PollMemoryAndDetectPeak();
+
+  // It is safe to call these testing method only on the |task_runner_|.
+  State state_for_testing() const { return state_; }
+  uint32_t poll_tasks_count_for_testing() const { return poll_tasks_count_; }
+
+  // The task runner where all the internal calls are posted onto. This field
+  // must be accessed only by the thread that owns the class instance and NOT by
+  // by the posted tasks that run on the |task_runner_| itself.
+  scoped_refptr<SequencedTaskRunner> task_runner_;
+  SequenceChecker sequence_checker_;
+
+  // After the Initialize() call, the fields below, must be accessed only from
+  // the |task_runner_|.
+
+  // Bound function to get an updated list of polling-capable dump providers.
+  GetDumpProvidersFunction get_dump_providers_function_;
+
+  // The callback to invoke when peaks are detected.
+  OnPeakDetectedCallback on_peak_detected_callback_;
+
+  // List of polling-aware dump providers to invoke upon each poll.
+  DumpProvidersList dump_providers_;
+
+  State state_;
+
+  uint32_t polling_interval_ms_;
+
+  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_