Make ResourceReporter a client of memory coordinator

chrome/browser/chromeos/resource_reporter/resource_reporter.cc

Index: chrome/browser/chromeos/resource_reporter/resource_reporter.cc
diff --git a/chrome/browser/chromeos/resource_reporter/resource_reporter.cc b/chrome/browser/chromeos/resource_reporter/resource_reporter.cc
index 30d3041fa8945618ea9cbd2d5fca6a65acc9c8ea..ee7ea67d13c8a7ef9f28bee056ba4e713f0ed186 100644
--- a/chrome/browser/chromeos/resource_reporter/resource_reporter.cc
+++ b/chrome/browser/chromeos/resource_reporter/resource_reporter.cc
@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/memory/memory_coordinator_client_registry.h"
 #include "base/memory/memory_pressure_monitor.h"
 #include "base/memory/ptr_util.h"
 #include "base/rand_util.h"
@@ -218,7 +219,9 @@ ResourceReporter::ResourceReporter()
       last_gpu_process_cpu_(0.0),
       last_browser_process_memory_(0),
       last_gpu_process_memory_(0),
-      is_monitoring_(false) {}
+      is_monitoring_(false) {
+  base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this);

[afakhry, 2016/10/18 16:20:20]

I believe this line belongs to StartMonitoring().

Also shouldn't there be a corresponding:
base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this);
in StopMonitoring()?

[hajimehoshi, 2016/10/25 07:39:42]

Done.

+}
 
 // static
 std::unique_ptr<rappor::Sample> ResourceReporter::CreateRapporSample(
@@ -391,35 +394,61 @@ void ResourceReporter::OnMemoryPressure(
     MemoryPressureLevel memory_pressure_level) {
   if (memory_pressure_level ==
       MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_CRITICAL) {
-    // If we are already listening to the task manager, then we're waiting for
-    // a refresh event.
-    if (observed_task_manager())
+    StartRecordingCurrentState();
+  } else {
+    StopRecordingCurrentState();
+  }
+}
+
+void ResourceReporter::StartRecordingCurrentState() {
+  // If we are already listening to the task manager, then we're waiting for
+  // a refresh event.
+  if (observed_task_manager())
+    return;
+
+  // We only record Rappor samples only if it's the first ever critical memory
+  // pressure event we receive, or it has been more than
+  // |kMinimumTimeBetweenReportsInMs| since the last time we recorded samples.
+  if (g_browser_process->local_state()) {
+    const base::Time now = base::Time::NowFromSystemTime();
+    const base::Time last_rappor_report_time = base::Time::FromDoubleT(
+        g_browser_process->local_state()->GetDouble(kLastRapporReportTimeKey));
+    const base::TimeDelta delta_since_last_report =
+        now >= last_rappor_report_time ? now - last_rappor_report_time
+                                       : base::TimeDelta::Max();
+
+    if (delta_since_last_report < kMinimumTimeBetweenReports)
       return;
+  }
 
-    // We only record Rappor samples only if it's the first ever critical memory
-    // pressure event we receive, or it has been more than
-    // |kMinimumTimeBetweenReportsInMs| since the last time we recorded samples.
-    if (g_browser_process->local_state()) {
-      const base::Time now = base::Time::NowFromSystemTime();
-      const base::Time last_rappor_report_time =
-          base::Time::FromDoubleT(g_browser_process->local_state()->GetDouble(
-              kLastRapporReportTimeKey));
-      const base::TimeDelta delta_since_last_report =
-          now >= last_rappor_report_time ? now - last_rappor_report_time
-                                         : base::TimeDelta::Max();
-
-      if (delta_since_last_report < kMinimumTimeBetweenReports)
-        return;
-    }
+  // Start listening to the task manager and wait for the first refresh event
+  // with background calculations completion.
+  task_manager_to_observe_->AddObserver(this);
+}
 
-    // Start listening to the task manager and wait for the first refresh event
-    // with background calculations completion.
-    task_manager_to_observe_->AddObserver(this);
-  } else {
-    // If we are still listening to the task manager from an earlier critical
-    // memory pressure level, we need to stop listening to it.
-    if (observed_task_manager())
-      observed_task_manager()->RemoveObserver(this);
+void ResourceReporter::StopRecordingCurrentState() {
+  // If we are still listening to the task manager from an earlier critical
+  // memory pressure level, we need to stop listening to it.
+  if (observed_task_manager())
+    observed_task_manager()->RemoveObserver(this);
+}
+
+void ResourceReporter::OnMemoryStateChange(base::MemoryState state) {
+  // TODO(hajimehoshi): Adjust the size of this memory usage according to
+  // |state|. ResourceReporter doesn't have a feature to limit memory usage at

[afakhry, 2016/10/18 16:20:20]

I don't understand this comment. The ResourceReporter was never meant to be used
to limit memory usage. I'm not aware of any design docs suggesting to extend its
behavior. Please clarify.

[haraken, 2016/10/18 18:52:20]

You're correct. ResourceReporter is used only for UMA reporting. Let's just
remove the comment.

[hajimehoshi, 2016/10/25 07:39:42]

Hmm, so this doesn't match with MemoryCoordinator's 'stateful' concept. I
removed this comment, but actually should ResourceReporter be
MemoryCoordinatorClient? +bashi, +chrisha

[hajimehoshi, 2016/10/26 07:42:36]

On second thought, this starting/stopping recording UMA seems stateful and this
matches MemoryCoordinator's concept. Sorry for confusion.

PTAL

[chrisha, 2016/10/26 21:32:43]

Yeah, this seems like a reasonable use to me, to grab a snapshot of the resource
situation when under pressure.

+  // present.
+  switch (state) {
+    case base::MemoryState::NORMAL:
+      StopRecordingCurrentState();
+      break;
+    case base::MemoryState::THROTTLED:
+      StartRecordingCurrentState();
+      break;
+    case base::MemoryState::SUSPENDED:
+    // Note: Not supported at present. Fall through.
+    case base::MemoryState::UNKNOWN:
+      NOTREACHED();

[afakhry, 2016/10/18 16:20:20]

What effect does this NOTREACHED() have on Chrome OS debug builds? Do we ever
hit SUSPENDED or UNKNOWN? I just want to make sure that we don't cause
developers a lot of pain crashing their runs because of this NOTREACHED().

[hajimehoshi, 2016/10/25 07:39:42]

Right, SUSPENDED and UNKNOWN never come to this component.

+      break;
   }
 }