memory-infra: Add peak-detector skeleton.

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/threading/sequenced_task_runner_handle.h"
+#include "base/time/time.h"
+#include "base/trace_event/memory_dump_provider_info.h"
+
+namespace {
+const uint32_t kInitialPollingIntervalMs = 1;

[Primiano Tucci (use gerrit), 2017/03/31 16:51:07]

will sort this out in the next CL when I move the stuff from the MDScheduler

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

Acknowledged.

+}  // namespace
+
+namespace base {
+namespace trace_event {
+
+// static
+MemoryPeakDetector* MemoryPeakDetector::GetInstance() {
+  static MemoryPeakDetector* instance = new MemoryPeakDetector();
+  return instance;
+}
+
+MemoryPeakDetector::MemoryPeakDetector()
+    : state_(NOT_INITIALIZED), polling_interval_ms_(0), poll_tasks_count_(0) {
+  // Bind the checker to the Initialize() call. Where this get constructed is
+  // irrelevant.
+  sequence_checker_.DetachFromSequence();
+}
+
+MemoryPeakDetector::~MemoryPeakDetector() {
+  // This should be hit only in tests. In that case the test should call
+  // TearDownForTesting() first.
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK_EQ(NOT_INITIALIZED, state_);
+}
+
+void MemoryPeakDetector::Initialize(
+    const GetDumpProvidersFunction& get_dump_providers_function,
+    const scoped_refptr<SequencedTaskRunner>& task_runner,
+    const OnPeakDetectedCallback& on_peak_detected_callback) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(!get_dump_providers_function.is_null());
+  DCHECK(!on_peak_detected_callback.is_null());
+  DCHECK(state_ == NOT_INITIALIZED || state_ == DISABLED);

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

Why initialize multiple times? shouldn't this be state_ == DISABLED?
TearDown sets to NOT_INITIALIZED anyway.

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

TearDown is only for tests.
initialize multiple times is for near-term compatibility, where every time we do
OnTraceLogEnabled we'll get a new thread, hence will have to re-initialize the
task-runner

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

If we call it multiple times, maybe Setup()? also setup without tear down seems
error prone in future

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

+1, done
Alright, the only argument is really avoiding keeping the task runner
refcounted, in the case we switch to another task runner model where the
task_runner is not just a proxy.
Done

+  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;
+}
+
+void MemoryPeakDetector::TearDownForTesting() {
+  get_dump_providers_function_.Reset();
+  task_runner_ = nullptr;
+  on_peak_detected_callback_.Reset();
+  dump_providers_.clear();
+  state_ = NOT_INITIALIZED;
+  poll_tasks_count_ = 0;
+}
+
+void MemoryPeakDetector::Start() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  task_runner_->PostTask(
+      FROM_HERE, Bind(&MemoryPeakDetector::StartInternal, Unretained(this)));
+}
+
+void MemoryPeakDetector::StartInternal() {
+  DCHECK_EQ(DISABLED, state_);
+  state_ = ENABLED;
+  polling_interval_ms_ = kInitialPollingIntervalMs;
+

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

You'd also need to set poll_count = 0;

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

That is for testing only and I only tear it down in the TearDownForTesting

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

Does it mean this function will not support multiple polling sessions in single
browsing session?
1. start polling()
2. stop polling()
3. start polling() -> now we start with poll_count = X.
I don't think there is a need to keep the state and we might run into some bugs.

In case of multiple tracing sessions with different dump threads, tear down will
be needed because we will then be keeping a reference to a task runner of the
dead thread.

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

I see where the confusion comes from. poll_count_ is really
poll_count_for_testing_, i.e. it is not used for production decisions. Renamed
var accordingly.

The use case you mentioned is supported, just I need the absolute number of poll
counts for the test, to keep track of spurious poll tasks between stop/start.
Is purely for tests.
Is not a problem with the current Thread, as the task runner is just a
MessageLoop proxy and will just no-op PostTask, but agree might become a problem
in future.

[ssid, 2017/04/04 05:08:35]

Acknowledged.

+  // 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. Deferring the polling, hence, this is not
+  // just a minor optimization.

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

Do we need a comment that says "this is not just a minor optimization"?

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

That was to prevent a future myself from removing it. although, thinking twice,
we have enough tests to make sure that that won't happen so you are right this
comment is superfluos

+  ReloadDumpProvidersAndStartPollingIfNeeded();
+}
+
+void MemoryPeakDetector::Stop() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  task_runner_->PostTask(
+      FROM_HERE, Bind(&MemoryPeakDetector::StopInternal, Unretained(this)));
+}
+
+void MemoryPeakDetector::StopInternal() {
+  DCHECK_NE(NOT_INITIALIZED, state_);
+  state_ = DISABLED;
+  dump_providers_.clear();
+}
+
+void MemoryPeakDetector::NotifyMemoryDumpProvidersChanged() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  task_runner_->PostTask(

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

What if we registered the first dump provider before creating the dump thread.
Or are we going to use some existing worker pool here?
Where are you planning to do Initialize()? at MDM constructor? Why not do it in
this CL?

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

Ah you are right, I have to make this a no-op (early out) if task_runner_ is
nullptr.
The code should still work, because what will happen there is:
 - MDP gets registered before Initialize(). This call is a noop
 - Then this get Initiazed() and Start().
 - Start will call ReloadDumpProvidersAndStartPollingIfNeeded,, so we'll fetch
the MDP from the list.

added a test to cover this.

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

Acknowledged.

+      FROM_HERE,
+      Bind(&MemoryPeakDetector::ReloadDumpProvidersAndStartPollingIfNeeded,
+           Unretained(this)));
+}
+
+void MemoryPeakDetector::ReloadDumpProvidersAndStartPollingIfNeeded() {
+  DCHECK_NE(NOT_INITIALIZED, state_);
+  if (state_ == DISABLED)
+    return;  // Start will deal with this later.

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

nit: Start()

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

Done.

+
+  DCHECK((state_ == RUNNING && !dump_providers_.empty()) ||
+         (state_ == ENABLED && dump_providers_.empty()));
+
+  // This is really MemoryDumpManager::GetDumpProvidersForPolling, % testing.
+  dump_providers_.clear();
+  get_dump_providers_function_.Run(&dump_providers_);
+
+  if (state_ == ENABLED && !dump_providers_.empty()) {
+    // It's now time to start polling for realz.
+    state_ = RUNNING;
+    task_runner_->PostTask(
+        FROM_HERE,
+        Bind(&MemoryPeakDetector::PollMemoryAndDetectPeak, Unretained(this)));
+  } else if (state_ == RUNNING && dump_providers_.empty()) {
+    // Will cause th next PollMemoryAndDetectPeak() task to early return.

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

the

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

Done.

+    state_ = ENABLED;
+  }
+}
+
+void MemoryPeakDetector::PollMemoryAndDetectPeak() {
+  if (state_ != RUNNING)
+    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_++;
+  uint64_t memory_total = 0;
+  for (const scoped_refptr<MemoryDumpProviderInfo>& mdp_info :
+       dump_providers_) {
+    DCHECK(mdp_info->options.is_fast_polling_supported);
+    mdp_info->dump_provider->PollFastMemoryTotal(&memory_total);

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

    uint64_t value = 0;
    mdpinfo->dump_provider->PollFastMemoryTotal(&value);
    *memory_total += value;

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

Yup, I would have replaced allt this part next, but put this here to avoid to do
something silly next .)

+  }
+
+  // TODO(primiano): Move actual peak detection logic from the
+  // MemoryDumpScheduler in next CLs.
+
+  SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      Bind(&MemoryPeakDetector::PollMemoryAndDetectPeak, Unretained(this)),
+      TimeDelta::FromMilliseconds(polling_interval_ms_));
+}
+
+}  // namespace trace_event
+}  // namespace base