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 0266e488f83e0217f31d50990bec68312256ab5e..1becaa3c5fc7cff142e6aeaa099d3a2714d1c807 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/ptr_util.h"
 #include "base/rand_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -247,7 +248,9 @@ ResourceReporter::ResourceReporter()
                           task_manager::REFRESH_TYPE_CPU |
                               task_manager::REFRESH_TYPE_MEMORY |
                               task_manager::REFRESH_TYPE_PRIORITY),
-      system_cpu_cores_range_(GetCurrentSystemCpuCoresRange()) {}
+      system_cpu_cores_range_(GetCurrentSystemCpuCoresRange()) {
+  base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this);
+}
 
 // static
 std::unique_ptr<rappor::Sample> ResourceReporter::CreateRapporSample(
@@ -356,73 +359,98 @@ ResourceReporter::SampleTaskByMemory() const {
 
 void ResourceReporter::OnMemoryPressure(
     MemoryPressureLevel memory_pressure_level) {
-  if (have_seen_first_task_manager_refresh_ &&
-      memory_pressure_level ==
+  if (memory_pressure_level ==
       MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_CRITICAL) {
-    // Report browser and GPU processes usage using UMA histograms.
-    UMA_HISTOGRAM_ENUMERATION(
-        "ResourceReporter.BrowserProcess.CpuUsage",
-        GET_ENUM_VAL(GetCpuUsageRange(last_browser_process_cpu_)),
-        GET_ENUM_VAL(CpuUsageRange::NUM_RANGES));
-    UMA_HISTOGRAM_ENUMERATION(
-        "ResourceReporter.BrowserProcess.MemoryUsage",
-        GET_ENUM_VAL(GetMemoryUsageRange(last_browser_process_memory_)),
-        GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
-    UMA_HISTOGRAM_ENUMERATION(
-        "ResourceReporter.GpuProcess.CpuUsage",
-        GET_ENUM_VAL(GetCpuUsageRange(last_gpu_process_cpu_)),
-        GET_ENUM_VAL(CpuUsageRange::NUM_RANGES));
-    UMA_HISTOGRAM_ENUMERATION(
-        "ResourceReporter.GpuProcess.MemoryUsage",
-        GET_ENUM_VAL(GetMemoryUsageRange(last_gpu_process_memory_)),
-        GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
+    RecordCurrentState();
+  }
+}
 
-    // For the rest of tasks, report them using Rappor.
-    auto* rappor_service = g_browser_process->rappor_service();
-    if (!rappor_service)
-      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 (!have_seen_first_memory_pressure_event_) {
-      have_seen_first_memory_pressure_event_ = true;
-    } else if ((base::TimeTicks::Now() - last_memory_pressure_event_time_) <
-        base::TimeDelta::FromMilliseconds(kMinimumTimeBetweenReportsInMs)) {
-      return;
-    }
+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
+  // present.
+  switch (state) {
+    case base::MemoryState::NORMAL:
+      break;
+    case base::MemoryState::THROTTLED:
+      RecordCurrentState();

[bashi, 2016/10/04 10:12:20]

If I remember the thread we've discussed this migration correctly, these
UMAs/rappor aren't very useful. In memory coordinator world, we might want to
remove RecordCurrentState(). So my suggestion is that not making
ResourceReporter be a client of memory coordinator. When MemoryPressureListener
is replaced with memory coordinator, we can remove OnTrimMemory().

afakhry@: What do you think?

[hajimehoshi, 2016/10/18 10:13:45]

The situation has been changed by https://codereview.chromium.org/2363223002 and
now UMAs are much more meaningful?

+      break;
+    case base::MemoryState::SUSPENDED:
+    // Note: Not supported at present. Fall through.
+    case base::MemoryState::UNKNOWN:
+      NOTREACHED();
+      break;
+  }
+}
 
-    last_memory_pressure_event_time_ = base::TimeTicks::Now();
-
-    // Use weighted random sampling to select a task to report in the CPU
-    // metric.
-    const TaskRecord* sampled_cpu_task = SampleTaskByCpu();
-    if (sampled_cpu_task) {
-      std::unique_ptr<rappor::Sample> cpu_sample(
-          CreateRapporSample(rappor_service, *sampled_cpu_task));
-      cpu_sample->SetFlagsField(kRapporNumCoresRangeFlagsField,
-                                GET_ENUM_VAL(system_cpu_cores_range_),
-                                GET_ENUM_VAL(CpuCoresNumberRange::NUM_RANGES));
-      cpu_sample->SetFlagsField(
-          kRapporUsageRangeFlagsField,
-          GET_ENUM_VAL(GetCpuUsageRange(sampled_cpu_task->cpu_percent)),
-          GET_ENUM_VAL(CpuUsageRange::NUM_RANGES));
-      rappor_service->RecordSampleObj(kCpuRapporMetric, std::move(cpu_sample));
-    }
+void ResourceReporter::RecordCurrentState() {
+  if (!have_seen_first_task_manager_refresh_)
+    return;
 
-    // Use weighted random sampling to select a task to report in the memory
-    // metric.
-    const TaskRecord* sampled_memory_task = SampleTaskByMemory();
-    if (sampled_memory_task) {
-      std::unique_ptr<rappor::Sample> memory_sample(
-          CreateRapporSample(rappor_service, *sampled_memory_task));
-      memory_sample->SetFlagsField(
-          kRapporUsageRangeFlagsField,
-          GET_ENUM_VAL(GetMemoryUsageRange(sampled_memory_task->memory_bytes)),
-          GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
-      rappor_service->RecordSampleObj(kMemoryRapporMetric,
-                                      std::move(memory_sample));
-    }
+  // Report browser and GPU processes usage using UMA histograms.
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourceReporter.BrowserProcess.CpuUsage",
+      GET_ENUM_VAL(GetCpuUsageRange(last_browser_process_cpu_)),
+      GET_ENUM_VAL(CpuUsageRange::NUM_RANGES));
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourceReporter.BrowserProcess.MemoryUsage",
+      GET_ENUM_VAL(GetMemoryUsageRange(last_browser_process_memory_)),
+      GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourceReporter.GpuProcess.CpuUsage",
+      GET_ENUM_VAL(GetCpuUsageRange(last_gpu_process_cpu_)),
+      GET_ENUM_VAL(CpuUsageRange::NUM_RANGES));
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourceReporter.GpuProcess.MemoryUsage",
+      GET_ENUM_VAL(GetMemoryUsageRange(last_gpu_process_memory_)),
+      GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
+
+  // For the rest of tasks, report them using Rappor.
+  auto* rappor_service = g_browser_process->rappor_service();
+  if (!rappor_service)
+    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 (!have_seen_first_memory_pressure_event_) {
+    have_seen_first_memory_pressure_event_ = true;
+  } else if ((base::TimeTicks::Now() - last_memory_pressure_event_time_) <
+             base::TimeDelta::FromMilliseconds(
+                 kMinimumTimeBetweenReportsInMs)) {
+    return;
+  }
+
+  last_memory_pressure_event_time_ = base::TimeTicks::Now();
+
+  // Use weighted random sampling to select a task to report in the CPU
+  // metric.
+  const TaskRecord* sampled_cpu_task = SampleTaskByCpu();
+  if (sampled_cpu_task) {
+    std::unique_ptr<rappor::Sample> cpu_sample(
+        CreateRapporSample(rappor_service, *sampled_cpu_task));
+    cpu_sample->SetFlagsField(kRapporNumCoresRangeFlagsField,
+                              GET_ENUM_VAL(system_cpu_cores_range_),
+                              GET_ENUM_VAL(CpuCoresNumberRange::NUM_RANGES));
+    cpu_sample->SetFlagsField(
+        kRapporUsageRangeFlagsField,
+        GET_ENUM_VAL(GetCpuUsageRange(sampled_cpu_task->cpu_percent)),
+        GET_ENUM_VAL(CpuUsageRange::NUM_RANGES));
+    rappor_service->RecordSampleObj(kCpuRapporMetric, std::move(cpu_sample));
+  }
+
+  // Use weighted random sampling to select a task to report in the memory
+  // metric.
+  const TaskRecord* sampled_memory_task = SampleTaskByMemory();
+  if (sampled_memory_task) {
+    std::unique_ptr<rappor::Sample> memory_sample(
+        CreateRapporSample(rappor_service, *sampled_memory_task));
+    memory_sample->SetFlagsField(
+        kRapporUsageRangeFlagsField,
+        GET_ENUM_VAL(GetMemoryUsageRange(sampled_memory_task->memory_bytes)),
+        GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
+    rappor_service->RecordSampleObj(kMemoryRapporMetric,
+                                    std::move(memory_sample));
   }
 }