Reporting top cpu and memory consumers via rappor on chromeos

chrome/browser/chromeos/resource_reporter/resource_reporter.h

Index: chrome/browser/chromeos/resource_reporter/resource_reporter.h
diff --git a/chrome/browser/chromeos/resource_reporter/resource_reporter.h b/chrome/browser/chromeos/resource_reporter/resource_reporter.h
new file mode 100644
index 0000000000000000000000000000000000000000..0bac8360d20e5bfdf3526e8835027da716bc4efe
--- /dev/null
+++ b/chrome/browser/chromeos/resource_reporter/resource_reporter.h
@@ -0,0 +1,214 @@
+// 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 <set>
+
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "base/memory/memory_pressure_listener.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;
+}
+
+using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
+
+namespace chromeos {
+
+// A system that tracks the top |kTopConsumersCount| CPU and memory consumer
+// Chrome tasks and reports them via Rappor whenever memory pressure is moderate
+// or higher.
+class ResourceReporter
+    : public task_management::TaskManagerObserver,
+      public metrics::MetricsServiceObserver {
+ public:
+  // A collection of the data of a task manager's task that the ResourceReporter
+  // is interested in.
+  struct TaskRecord {
+    // The ID of the task.
+    task_management::TaskId id;
+
+    // The name of the Rappor sample to be used for recording a sample
+    // representing this task.
+    std::string rappor_sample;
+
+    // The CPU usage of the task from most recent task manager refresh.

[ncarter (slow), 2015/11/10 23:29:56]

Is this a percentage? Clarify the comment.

[afakhry, 2015/11/12 00:21:28]

Done.

+    double cpu;
+
+    // The physical memory usage of the task from the most recent task manager
+    // refresh.

[ncarter (slow), 2015/11/10 23:29:56]

In bytes? Does this include shared memory?

[afakhry, 2015/11/12 00:21:28]

Done.

+    int64_t memory;
+
+    // True if the task is running on backgrounded process.

[ncarter (slow), 2015/11/10 23:29:56]

"running at background priority" might be a better way to phrase this.

[afakhry, 2015/11/12 00:21:28]

Done.

+    bool is_background;
+
+    explicit TaskRecord(task_management::TaskId the_id)
+        : id(the_id), cpu(0.0), memory(0), is_background(false) {}
+
+    TaskRecord(task_management::TaskId the_id,
+               const std::string& the_sample,
+               double the_cpu,
+               int64_t the_memory,
+               bool background)

[ncarter (slow), 2015/11/10 23:29:56]

Should be in the .cc file.

[afakhry, 2015/11/12 00:21:28]

Done.

+        : id(the_id),
+          rappor_sample(the_sample),
+          cpu(the_cpu),
+          memory(the_memory),
+          is_background(background) {}
+  };
+
+  ~ResourceReporter() override;
+
+  // The system runs only if the user has opted-in to send anonymous usage stats
+  // to Google. We observe state changes in the MetricsService which will

[ncarter (slow), 2015/11/10 23:29:56]

Is there a way to word this without saying "Google" ?

[afakhry, 2015/11/12 00:21:28]

Done.

+  // determine if the ResourceReporter needs to observe the task manager and
+  // collect resource usage stats.
+  static void StartObservingMetricsService();
+  static void StopObservingMetricsService();
+
+  // The singleton instance.
+  static ResourceReporter* GetInstance();
+
+  // 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;
+
+  // metrics::MetricsServiceObserver:
+  void OnMetricsServiceStart() override;
+  void OnMetricsServiceStop() 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);
+
+  // A functor to sort the TaskRecords by their |cpu| in a descending order.
+  struct TaskRecordByCpuSorter {
+    bool operator()(TaskRecord* const& lhs, TaskRecord* const& rhs) const {
+      if (lhs->cpu == rhs->cpu)

[ncarter (slow), 2015/11/10 23:29:56]

This is a nontrivial function; shouldn't be in a header file.

[afakhry, 2015/11/12 00:21:28]

Done and modified for both sorters to be a Less then comparison of task records
by cpu and memory respectively. So that we can use in the priority queue.

+        return lhs->id > rhs->id;
+      return lhs->cpu > rhs->cpu;
+    }
+  };
+
+  // A functor to sort the TaskRecords by their |memory| in a descending order.
+  struct TaskRecordByMemorySorter {
+    bool operator()(TaskRecord* const& lhs, TaskRecord* const& rhs) const {
+      if (lhs->memory == rhs->memory)
+        return lhs->id > rhs->id;
+      return lhs->memory > rhs->memory;
+    }
+  };
+
+  enum TaskProcessPriority {
+    FOREGROUND = 0,
+    BACKGROUND,
+    PRIORITIES_NUM,
+  };
+
+  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,
+  };
+
+  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,
+  };
+
+  enum CpuCoresNumberRange {
+    RANGE_CORES_0_CORES = 0,

[ncarter (slow), 2015/11/10 23:29:56]

I've never seen a machine with 0 cores. How would that work? If we need this
bucket, should it be a _NA?

[afakhry, 2015/11/12 00:21:28]

Done.

+    RANGE_CORES_1_TO_2_CORES,   // [1, 2]

[ncarter (slow), 2015/11/10 23:29:56]

Should 1 and 2 cores be separate buckets?

[afakhry, 2015/11/12 00:21:28]

Done.

+    RANGE_CORES_2_TO_4_CORES,   // ]2, 4]

[ncarter (slow), 2015/11/10 23:29:56]

This "]2" notation (which I assume means an open interval) is unfamiliar to me.
The mathematical notation I'm familiar with is "(2, 4]", which would mean an
interval that excludes 2 and includes 4.

For the cores, I'd just express them as closed intervals: RANGE_CORES_5_TO_8,
3_TO_4, etc, which I think is unambiguous.

[afakhry, 2015/11/12 00:21:28]

Haha! Yes it means an open interval. We used to write it this way in math
classes back in Egypt! But I like your suggestion to make them unambiguous!
Done!

+    RANGE_CORES_4_TO_8_CORES,   // ]4, 8]
+    RANGE_CORES_8_TO_16_CORES,  // ]8, 16]
+    RANGE_CORES_ABOVE_16_CORES, // > 16
+    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);
+  static MemoryUsageRange GetMemoryUsageRange(int64_t memory_in_bytes);
+
+  // Gets the bucket in which the current system's number of CPU cores fall
+  // into.
+  static CpuCoresNumberRange GetCurrentSystemCpuCoresRange();
+
+  // Start / stop observing the task manager and the memory pressure events.
+  void StartMonitoring();
+  void StopMonitoring();
+
+  // 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::set<TaskRecord*, TaskRecordByCpuSorter> task_records_by_cpu_;
+
+  // Contains the top |kTopConsumerCount| memory consumer tasks sorted in a
+  // descending order by their memory usage.
+  std::set<TaskRecord*, TaskRecordByMemorySorter> task_records_by_memory_;
+
+  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 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;
+
+  MemoryPressureLevel previous_memory_pressure_level_ =
+      MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_NONE;
+
+  // MetricsService::Start()/Stop() can be called multiple times which can cause
+  // all sort of trouble. We need to track whether we have already started
+  // monitoring or not using the below flag.
+  bool is_monitoring_;

[ncarter (slow), 2015/11/10 23:29:56]

This is kind of weird. Why are there redundant Start calls? Do they not balance
with the Stop calls? Should we stop monitoring on the first Stop() or the last
Stop()?

[afakhry, 2015/11/12 00:21:28]

The problem happens when Start() is called several times, you end up adding the
ResourceReporter as an observer of the task manager multiple times which will
cause it to crash of course.

See here, there is no code to handle multiple calls.

https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

[ncarter (slow), 2015/11/13 22:44:32]

Are we just coding around a bug in the MetricsService? Should this bool be added
to the MetricsService? Seems reasonable for an observer to expect balanced
Start/Stop calls.

+
+  DISALLOW_COPY_AND_ASSIGN(ResourceReporter);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_RESOURCE_REPORTER_RESOURCE_REPORTER_H_