Fix the Chrome OS ResourceReporter

chrome/browser/chromeos/resource_reporter/resource_reporter.h

 // 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_RESOURCE_REPORTER_RESOURCE_REPORTER_H_
 #define CHROME_BROWSER_CHROMEOS_RESOURCE_REPORTER_RESOURCE_REPORTER_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
-#include <map>
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/memory_pressure_listener.h"
 #include "base/memory/singleton.h"
-#include "base/time/time.h"
 #include "chrome/browser/task_manager/task_manager_observer.h"
 #include "components/metrics/metrics_service.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "components/rappor/sample.h"
 
 namespace chromeos {
 
 // A system that tracks the top |kTopConsumersCount| CPU and memory consumer
 // Chrome tasks and reports a weighted random sample of them via Rappor whenever
 // memory pressure is critical. The reporting is limited to once per
 // |kMinimumTimeBetweenReportsInMS|.
 class ResourceReporter : public task_manager::TaskManagerObserver {
  public:
@@ skipping 27 matching lines @@
 
     // True if the task is running on a process at background priority.
     bool is_background;
   };
 
   ~ResourceReporter() override;
 
   // The singleton instance.
   static ResourceReporter* GetInstance();
 
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
   // Start / stop observing the task manager and the memory pressure events.
   void StartMonitoring();
   void StopMonitoring();
 
   // task_manager::TaskManagerObserver:
-  void OnTaskAdded(task_manager::TaskId id) override;
-  void OnTaskToBeRemoved(task_manager::TaskId id) override;
-  void OnTasksRefreshed(const task_manager::TaskIdList& task_ids) override;
+  void OnTasksRefreshedWithBackgroundCalculations(
+      const task_manager::TaskIdList& task_ids) override;
 
  private:
   friend struct base::DefaultSingletonTraits<ResourceReporter>;
   friend class ResourceReporterTest;
   FRIEND_TEST_ALL_PREFIXES(ResourceReporterTest, TestGetCpuRapporMetricName);
   FRIEND_TEST_ALL_PREFIXES(ResourceReporterTest, TestGetMemoryRapporMetricName);
   FRIEND_TEST_ALL_PREFIXES(ResourceReporterTest, TestAll);
 
   // WARNING: The below enum MUST never be renamed, modified or reordered, as
   // they're written to logs. You can only insert a new element immediately
@@ skipping 35 matching lines @@
     RANGE_NA              = 0,
     RANGE_1_CORE          = 1 << 0,
     RANGE_2_CORES         = 1 << 1,
     RANGE_3_TO_4_CORES    = 1 << 2,
     RANGE_5_TO_8_CORES    = 1 << 3,
     RANGE_9_TO_16_CORES   = 1 << 4,
     RANGE_ABOVE_16_CORES  = 1 << 5,
     NUM_RANGES            = 7,
   };
 
-  // The maximum number of top consumer tasks of each resource that we're
-  // interested in reporting.
-  static const size_t kTopConsumersCount;
+  // The CPU and memory thresholds beyond which the tasks will be reported.
+  static const double kTaskCpuThresholdForReporting;
+  static const int64_t kTaskMemoryThresholdForReporting;
 
   ResourceReporter();
 
   // Creates a Rappor sample for the given |task_record|.
   static std::unique_ptr<rappor::Sample> CreateRapporSample(
       rappor::RapporService* rappor_service,
       const TaskRecord& task_record);
 
   // Gets the CPU/memory usage ranges given the |cpu| / |memory_in_bytes|
   // values.
   static CpuUsageRange GetCpuUsageRange(double cpu);
   static MemoryUsageRange GetMemoryUsageRange(int64_t memory_in_bytes);
 
   // Gets the bucket in which the current system's number of CPU cores fall.
   static CpuCoresNumberRange GetCurrentSystemCpuCoresRange();
 
   // Perform a weighted random sampling to select a task by its CPU or memory
   // usage weights so that we can record samples for them via Rappor.
   // They return nullptr if no TaskRecord has been selected.
   const TaskRecord* SampleTaskByCpu() const;
   const TaskRecord* SampleTaskByMemory() const;
 
+  // Does the actual recording of Rappor and UMA samples.
+  void ReportSamples();
+
   // The callback function that will be invoked on memory pressure events.
   using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
   void OnMemoryPressure(MemoryPressureLevel memory_pressure_level);
 
-  // We'll use this to watch for memory pressure events so that we can trigger
-  // Rappor sampling at at the critical memory pressure level.
+  // Monitor memory pressure events.
   std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
 
-  // Contains the collected data about the currently running tasks from the most
-  // recent task manager refresh.
-  std::map<task_manager::TaskId, std::unique_ptr<TaskRecord>> task_records_;
+  // Contains the data about the most CPU and memory intensive tasks collected
+  // at a critical memory pressure event.
+  std::vector<TaskRecord> task_records_;
 
-  // Contains the top |kTopConsumerCount| CPU consumer tasks sorted in a
-  // descending order by their CPU usage.
-  std::vector<TaskRecord*> task_records_by_cpu_;
-
-  // Contains the top |kTopConsumerCount| memory consumer tasks sorted in a
-  // descending order by their memory usage.
-  std::vector<TaskRecord*> task_records_by_memory_;
-
-  // The time at which the previous critical memory pressure event was received
-  // at which we recorded Rappor samples. This is used to limit generating a
-  // Rappor report to once per |kMinimumTimeBetweenReportsInMS|.
-  // This is needed to avoid generating a lot of samples that can lower the
-  // Rappor privacy guarantees.
-  base::TimeTicks last_memory_pressure_event_time_;
+  // The task manager used for unit tests.
+  task_manager::TaskManagerInterface* task_manager_for_testing_;

[ncarter (slow), 2016/10/07 19:04:16]

A for_testing_ data member should be a last resort, since it'll result in
codeblocks in the .cc file that are unreachable in unittests. Two ways I see to
switch this to dependency injection:

(1) is to pass the TaskManagerInterface in the ctor. That's awkward to
impossible with a singleton, so I'd suggest instead:

(2) Pass a TaskManagerInterface* to StartMonitoring.

Then this member can be, like, task_manager_to_observe_, with a comment. Then
ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() just gets the real TM
singleton instance, and passes it on.

[afakhry, 2016/10/12 16:37:36]

That's a great suggestion actually! Done.
Thanks!

 
   // The range that includes the number of CPU cores in the current system.
   const CpuCoresNumberRange system_cpu_cores_range_;
 
   // The most recent reading for the browser and GPU processes to be reported as
   // UMA histograms when the system is under critical memory pressure.
-  double last_browser_process_cpu_ = 0.0;
-  double last_gpu_process_cpu_ = 0.0;
-  int64_t last_browser_process_memory_ = 0;
-  int64_t last_gpu_process_memory_ = 0;
+  double last_browser_process_cpu_;
+  double last_gpu_process_cpu_;
+  int64_t last_browser_process_memory_;
+  int64_t last_gpu_process_memory_;
 
   // Tracks whether monitoring started or not.
-  bool is_monitoring_ = false;
-
-  // True after we've seen a critical memory pressure event.
-  bool have_seen_first_memory_pressure_event_ = false;
-
-  // True after the first task manager OnTasksRefreshed() event is received.
-  bool have_seen_first_task_manager_refresh_ = false;
+  bool is_monitoring_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceReporter);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_RESOURCE_REPORTER_RESOURCE_REPORTER_H_