Reporting top cpu and memory consumers via rappor on chromeos

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 <map>
+#include <string>
+#include <vector>
+
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "base/memory/memory_pressure_listener.h"
+#include "base/time/time.h"
+#include "chrome/browser/task_management/task_manager_observer.h"
+#include "components/metrics/metrics_service.h"
+#include "components/rappor/sample.h"
+
+namespace base {
+template <typename T>
+struct DefaultSingletonTraits;
+}

[Daniel Erat, 2015/11/23 23:57:20]

i think it's generally considered fine (or even preferable) to include base
headers inside of header files instead of forward-declaring their objects. i'd
recommend just adding a #include <base/memory/singleton.h> to this file.

[afakhry, 2015/11/24 01:52:33]

Done.

+
+using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;

[Daniel Erat, 2015/11/23 23:57:20]

please don't pull things out into the top-level namespace in header files.
MemoryPressureLevel only appears in a single place in this file, so i think it'd
be better to just fully-qualify it there.

[afakhry, 2015/11/24 01:52:33]

Done.

+
+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 moderate or higher. The reporting is limited to once per
+// |kMinimumTimeBetweenReportsInMS|.
+class ResourceReporter : public task_management::TaskManagerObserver {
+ public:
+  // A collection of the data of a task manager's task that the ResourceReporter
+  // is interested in.
+  struct TaskRecord {
+    explicit TaskRecord(task_management::TaskId the_id);

[Daniel Erat, 2015/11/23 23:57:20]

s/the_id/task_id/ ?

[afakhry, 2015/11/24 01:52:34]

Done.

+
+    TaskRecord(task_management::TaskId the_id,

[Daniel Erat, 2015/11/23 23:57:20]

same here

[afakhry, 2015/11/24 01:52:33]

Done.

+               const std::string& task_name,
+               double the_cpu,

[Daniel Erat, 2015/11/23 23:57:20]

why "the_" on all of these? something like "cpu_percent" would be clearer.
please also add a comment documenting whether this is [0.0, 1.0] or [0.0,
100.0].

note also that it's safe to use same name for a member and c'tor parameter and
then do something like:

    cpu(cpu)

in the initialization list.

[afakhry, 2015/11/24 01:52:33]

Done.

+               int64_t the_memory,

[Daniel Erat, 2015/11/23 23:57:20]

include the units here. "memory_bytes"? "memory_kb"?

[afakhry, 2015/11/24 01:52:34]

Done.

+               bool background);
+
+    // The ID of the task.
+    task_management::TaskId id;
+
+    // The canonicalized task name to be used to represent the task in a Rappor
+    // sample.
+    std::string task_name_for_rappor;
+
+    // The CPU usage of the task from the most recent task manager refresh in
+    // percentage.
+    double cpu;
+
+    // The physical memory usage of the task from the most recent task manager
+    // refresh in bytes. It doesn't include shared memory. A value of -1 is
+    // invalid. See TaskManagerInterface::GetPhysicalMemoryUsage().

[Daniel Erat, 2015/11/23 23:57:20]

does this mean "-1 is used to represent missing values", or something else?

[afakhry, 2015/11/24 01:52:33]

It means that the memory usage calculation for this task is not ready yet. This
happens at the value before the very first calculation is complete and the
result is received on the UI thread.

I clarified the comment a bit.

+    int64_t memory;
+
+    // True if the task is running on a process at background priority.
+    bool is_background;
+  };
+
+  ~ResourceReporter() override;
+
+  // The singleton instance.
+  static ResourceReporter* GetInstance();
+
+  // Start / stop observing the task manager and the memory pressure events.
+  void StartMonitoring();
+  void StopMonitoring();
+
+  // task_management::TaskManagerObserver:
+  void OnTaskAdded(task_management::TaskId id) override;
+  void OnTaskToBeRemoved(task_management::TaskId id) override;
+  void OnTasksRefreshed(const task_management::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
+  // before the last.
+  enum TaskProcessPriority {

[Daniel Erat, 2015/11/23 23:57:20]

please use enum classes for all of these to make it harder to mess up the
typing. see "Enumerated Type Classes and Enum Bases" at
https://chromium-cpp.appspot.com/.

[afakhry, 2015/11/24 01:52:33]

I think it would be better not to use enum classes here. We need to convert
these enum values to their integral values when creating a Rappor sample and an
UMA histogram stat. Is it worth to clutter the code with static_cast<>'s here
and there?

+    FOREGROUND = 0,
+    BACKGROUND,

[Daniel Erat, 2015/11/23 23:57:20]

i'd recommend explicitly assigning e.g. BACKGROUND = 1 here to make it clearer
that these are fixed values, and also doing the same to the other enums.

[afakhry, 2015/11/24 01:52:33]

Done.

+    PRIORITIES_NUM,
+  };
+
+  // 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
+  // before the last.
+  enum CpuUsageRange {
+    RANGE_0_TO_10_PERCENT = 0,
+    RANGE_10_TO_30_PERCENT,
+    RANGE_30_TO_60_PERCENT,
+    RANGE_ABOVE_60_PERCENT,
+    CPU_RANGES_NUM,
+  };
+
+  // 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
+  // before the last.
+  enum MemoryUsageRange {
+    RANGE_0_TO_200_MB = 0,
+    RANGE_200_TO_400_MB,
+    RANGE_400_TO_600_MB,
+    RANGE_600_TO_800_MB,
+    RANGE_800_TO_1_GB,
+    RANGE_ABOVE_1_GB,
+    MEMORY_RANGES_NUM,
+  };
+
+  // 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
+  // before the last.
+  enum CpuCoresNumberRange {
+    RANGE_CORES_NA = 0,
+    RANGE_CORES_1_CORE,
+    RANGE_CORES_2_CORES,
+    RANGE_CORES_3_TO_4_CORES,
+    RANGE_CORES_5_TO_8_CORES,
+    RANGE_CORES_9_TO_16_CORES,
+    RANGE_CORES_ABOVE_16_CORES,
+    CORES_RANGES_NUM,
+  };
+
+  // The maximum number of top consumer tasks of each resource that we're
+  // interested in reporting.
+  static const size_t kTopConsumersCount;
+
+  ResourceReporter();
+
+  // Creates a Rappor sample for the given |task_record|.
+  static scoped_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);

[Daniel Erat, 2015/11/23 23:57:20]

any reason not to just put these into the anonymous namespace in the .cc file?

[afakhry, 2015/11/24 01:52:33]

These are exposed for testing.

+  static MemoryUsageRange GetMemoryUsageRange(int64_t memory_in_bytes);
+
+  // Gets the bucket in which the current system's number of CPU cores fall
+  // into.

[Daniel Erat, 2015/11/23 23:57:20]

grammar nit: remove "into" since this comment already includes "in which"
earlier

[afakhry, 2015/11/24 01:52:33]

Done.

+  static CpuCoresNumberRange GetCurrentSystemCpuCoresRange();
+
+  // Determines if we're allowed to record Rappor samples when a memory pressure
+  // event greater than or equal to a moderate pressure is received.
+  bool ShouldRecordSamples();
+
+  // 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;
+
+  // The callback function that will be invoked on memory pressure events.
+  void OnMemoryPressure(MemoryPressureLevel memory_pressure_level);
+
+  // We'll use this to watch for memory pressure events so that we can trigger
+  // Rappor sampling at moderate memory pressure or higher.
+  scoped_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_management::TaskId, scoped_ptr<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 memory pressure event (moderate or higher)
+  // 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_;
+
+  const CpuCoresNumberRange system_cpu_cores_range_;

[Daniel Erat, 2015/11/23 23:57:20]

what does this represent? the range that includes the number of cores present on
the system?

[afakhry, 2015/11/24 01:52:33]

Yes. I added a comment.

+
+  // the most recent reading for the browser and GPU processes to be reported as

[Daniel Erat, 2015/11/23 23:57:20]

nit: s/the/The/

[afakhry, 2015/11/24 01:52:33]

Done.

+  // UMA histograms when the system is under moderate memory pressure or higher.
+  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;
+
+  // Tracks whether monitoring started or not.
+  bool is_monitoring_ = false;
+
+  // Whether we ever received a memory pressure event for the first time that is
+  // greater than or equal to a moderate pressure.
+  bool is_first_memory_pressure_event_ = true;
+
+  DISALLOW_COPY_AND_ASSIGN(ResourceReporter);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_RESOURCE_REPORTER_RESOURCE_REPORTER_H_