Make ResourceReporter a client of memory coordinator

chrome/browser/chromeos/resource_reporter/resource_reporter.cc

 // Copyright 2015 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 "chrome/browser/chromeos/resource_reporter/resource_reporter.h"
 
 #include <cstdint>
 #include <queue>
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/task_manager/task_manager_interface.h"
 #include "components/rappor/rappor_service.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 89 matching lines @@
     task_manager::TaskManagerInterface* task_manager_to_observe) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (is_monitoring_)
     return;
 
   task_manager_to_observe_ = task_manager_to_observe;
   DCHECK(task_manager_to_observe_);
   is_monitoring_ = true;
   memory_pressure_listener_.reset(new base::MemoryPressureListener(
       base::Bind(&ResourceReporter::OnMemoryPressure, base::Unretained(this))));

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

Now, I'm a bit confused! I thought we're moving to a MemoryCoordinatorClient as
a replacement for being a MemoryPressureListener. What am I missing?

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

Temporarily MemoryCoordinatorClient and MemoryPressureListener are present at
the same time during migration. MemoryCoordinator and MemoryPressure are
exclusive and an experimental flag --memory-coordinator enables
MemoryCoordinator and disables MemoryPressure.

 }
 
 void ResourceReporter::StopMonitoring() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (!is_monitoring_)
     return;
 
   // We might be shutting down right after a critical memory pressure event, and
   // before we get an update from the task manager with all background
@@ skipping 79 matching lines @@
     : TaskManagerObserver(base::TimeDelta::FromSeconds(kRefreshIntervalSeconds),
                           task_manager::REFRESH_TYPE_CPU |
                               task_manager::REFRESH_TYPE_MEMORY |
                               task_manager::REFRESH_TYPE_PRIORITY),
       task_manager_to_observe_(nullptr),
       system_cpu_cores_range_(GetCurrentSystemCpuCoresRange()),
       last_browser_process_cpu_(0.0),
       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(
     rappor::RapporService* rappor_service,
     const ResourceReporter::TaskRecord& task_record) {
   std::unique_ptr<rappor::Sample> sample(
       rappor_service->CreateSample(rappor::UMA_RAPPOR_TYPE));
   sample->SetStringField(kRapporTaskStringField,
                          task_record.task_name_for_rappor);
   sample->SetFlagsField(kRapporPriorityFlagsField,
@@ skipping 152 matching lines @@
         GET_ENUM_VAL(MemoryUsageRange::NUM_RANGES));
     rappor_service->RecordSampleObj(kMemoryRapporMetric,
                                     std::move(memory_sample));
   }
 }
 
 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())
-      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);
+    StartRecordingCurrentState();
   } 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);
+    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;
+  }
+
+  // Start listening to the task manager and wait for the first refresh event
+  // with background calculations completion.
+  task_manager_to_observe_->AddObserver(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;
+  }
+}
+
 }  // namespace chromeos