[chromeos/about:power] Collect cpuidle and cpufreq stats

chrome/browser/chromeos/power/cpu_data_collector.cc

Index: chrome/browser/chromeos/power/cpu_data_collector.cc
diff --git a/chrome/browser/chromeos/power/cpu_data_collector.cc b/chrome/browser/chromeos/power/cpu_data_collector.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a1aad26fe35aa1a7a949b24c47f88dcff5bdd35a
--- /dev/null
+++ b/chrome/browser/chromeos/power/cpu_data_collector.cc
@@ -0,0 +1,272 @@
+// Copyright 2014 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 <vector>
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/chromeos/power/cpu_data_collector.h"
+#include "chrome/browser/chromeos/power/power_data_collector.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace chromeos {
+
+namespace {
+// The CPU data is sampled every |kCpuDataSamplePeriodSec| seconds.
+const int kCpuDataSamplePeriodSec = 30;
+
+const int kCpuOnlineStatus = 1;

[Daniel Erat, 2014/02/15 15:31:25]

add a comment describing what this corresponds to (is it a value of a particular
file in sysfs?)

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+
+#define CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE "/sys/devices/system/cpu"

[Daniel Erat, 2014/02/15 15:31:25]

why is this a macro instead of a const char[]?

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+
+// Path to file listing the range of possible CPUs on the system.
+const char kPossibleCpuPath[] = CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE

[Daniel Erat, 2014/02/15 15:31:25]

if the answer to the previous question is "so it can be concatenated with
literals", you should just make these be suffixes instead, e.g.

  const char kPossibleCpuPathSuffix[] = "/possible";

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+                                "/possible";
+
+// Format of the path to the file which contains information about a particular
+// CPU being online or offline.
+const char kCpuOnlinePathFormat[] =
+    CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE

[Daniel Erat, 2014/02/15 15:31:25]

same here

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    "/cpu%d/online";
+
+// Format of the path to the file which contains freq state information of a
+// CPU.
+const char kCpuFreqTimeInStatePathFormat[] =
+    CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE
+    "/cpu%d/cpufreq/stats/time_in_state";
+
+// Format of the path to the directory which contains information about an
+// idle state of a CPU on the system.
+const char kCpuIdleStateDirPathFormat[] =
+    CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE
+    "/cpu%d/cpuidle/state%d";
+
+// Format of the path to the file which contains the name of an idle state
+// of a CPU.
+const char kCpuIdleStateNamePathFormat[] =
+    CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE
+    "/cpu%d/cpuidle/state%d/name";
+
+// Format of the path which contains information about time spent in an idle
+// state on a CPU.
+const char kCpuIdleStateTimePathFormat[] =
+    CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE
+    "/cpu%d/cpuidle/state%d/time";
+
+#undef CHROMEOS_CPU_DATA_COLLECTOR_CPU_DATA_PATH_BASE
+
+// Returns true if the |i|-th CPU is online; false otherwise.
+bool CpuIsOnline(const int i) {
+  std::string cpu_online_file = base::StringPrintf(kCpuOnlinePathFormat, i);
+  if (!base::PathExists(base::FilePath(cpu_online_file))) {
+    // If the 'online' status file is missing, then it means that the CPU is
+    // not hot-pluggable and hence is always online.
+    return true;
+  }
+
+  int online;
+  std::string cpu_online_string;
+  base::ReadFileToString(base::FilePath(cpu_online_file), &cpu_online_string);

[Daniel Erat, 2014/02/15 15:31:25]

check the return value of reads

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  base::StringToInt(cpu_online_string, &online);
+  if (online == kCpuOnlineStatus) {

[Daniel Erat, 2014/02/15 15:31:25]

just do "return online == kCpuOnlineStatus"

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    return true;
+  } else {
+    return false;
+  }
+}
+
+// Samples the CPU idle state information from sysfs. |cpu_count| is the number
+// of possible CPUs on the system. Sample at index i in |idle_samples|
+// corresponds to the idle state information of the i-th CPU.
+void SampleCpuIdleData(
+    const int cpu_count,

[Daniel Erat, 2014/02/15 15:31:25]

don't need const for pass-by-value

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    std::vector<CpuDataCollector::StateOccupancySample>* idle_samples) {
+  base::Time startTime = base::Time::Now();

[Daniel Erat, 2014/02/15 15:31:25]

start_time

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  for (int i = 0; i < cpu_count; ++i) {
+    CpuDataCollector::StateOccupancySample idle_sample;
+    idle_sample.time = base::Time::Now();
+
+    if (!CpuIsOnline(i)) {
+      idle_sample.cpu_online = false;
+    } else {
+      idle_sample.cpu_online = true;
+      int k = 0;

[Daniel Erat, 2014/02/15 15:31:25]

give this variable a meaningful name

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+      std::string idle_state_dir = base::StringPrintf(
+          kCpuIdleStateDirPathFormat, i, k);
+      while (base::DirectoryExists(base::FilePath(idle_state_dir))) {
+        std::string name_file_path = base::StringPrintf(
+            kCpuIdleStateNamePathFormat, i, k);
+        DCHECK(base::PathExists(base::FilePath(name_file_path)));
+
+        std::string time_file_path = base::StringPrintf(
+            kCpuIdleStateTimePathFormat, i, k);
+        DCHECK(base::PathExists(base::FilePath(time_file_path)));
+
+        std::string state_name, occupancy_time_string;
+        base::ReadFileToString(base::FilePath(name_file_path), &state_name);

[Daniel Erat, 2014/02/15 15:31:25]

check return values

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+        base::ReadFileToString(base::FilePath(time_file_path),
+                               &occupancy_time_string);
+        int64 occupancy_time;
+        base::StringToInt64(occupancy_time_string, &occupancy_time);

[Daniel Erat, 2014/02/15 15:31:25]

check return value

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+        // idle state occupany time in sysfs is recorded in microseconds.
+        idle_sample.state_occupancy[state_name] = occupancy_time/1000;

[Daniel Erat, 2014/02/15 15:31:25]

it'd probably be better to keep a separate map from from int id to state name,
and key this by id. otherwise, a bunch of duplicate strings are getting stored
here.

[Siva Chandra, 2014/02/19 20:02:30]

Which duplicate strings are you referring to? Also, I am not sure if all CPUs
will have exactly similar states on different platforms.

[Daniel Erat, 2014/02/21 02:29:37]

i meant that each state name will be duplicated in each sample right now. you
can avoid this if you do something like this instead (untested, and assuming
that there are few enough states and frequencies that scanning through a vector
is faster than using std::map):

  std::vector<std::string> state_names_;

  ...
  std::string state_name = ...;
  int state_index = -1;
  for (size_t i = 0; i < state_names_.size(); ++i) {
    if (state_names_[i] == state_name) {
      state_index = static_cast<int>(i);
      break;
    }
  }
  if (state_index < 0) {
    state_index = static_cast<int>(state_names_.size());
    state_names_.push_back(state_name);
  }

then make state_occupancy be keyed by int instead of string and use state_index
instead of state_name when saving data to it. you can provide an accessor for
state_names_ so you can pass it to the JS code.

this is a bit more code than just using string keys like what you have here, but
i think it's worth it to avoid duplicating these strings across many different
maps.

[Siva Chandra, 2014/03/05 21:20:26]

Done.

+
+        ++k;
+        idle_state_dir = base::StringPrintf(kCpuIdleStateDirPathFormat, i, k);
+      }
+    }
+
+    idle_samples->push_back(idle_sample);
+  }
+
+  // If there was an interruption in sampling (like system suspended),
+  // re-sample!
+  if (base::TimeDelta(base::Time::Now() - startTime).InMilliseconds() > 500) {
+    idle_samples->clear();
+    SampleCpuIdleData(cpu_count, idle_samples);

[Daniel Erat, 2014/02/15 15:31:25]

i'd strongly prefer you didn't use recursion here -- if something consistently
goes wrong, you could loop forever. what's wrong with just skipping the sample
if too much time passed?

[Siva Chandra, 2014/02/19 20:02:30]

Removed recursion now. Just discarding the sample.

+  }
+}
+
+// Samples the CPU freq state information from sysfs. |cpu_count| is the number
+// of possible CPUs on the system. Sample at index i in |freq_samples|
+// corresponds to the freq state information of the i-th CPU.
+void SampleCpuFreqData(
+    const int cpu_count,
+    std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
+  base::Time startTime = base::Time::Now();

[Daniel Erat, 2014/02/15 15:31:25]

start_time

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  for (int i = 0; i < cpu_count; ++i) {
+    CpuDataCollector::StateOccupancySample freq_sample;
+
+    if (!CpuIsOnline(i)) {
+      freq_sample.time = base::Time::Now();
+      freq_sample.cpu_online = false;
+    } else {
+      freq_sample.cpu_online = true;
+
+      std::string time_in_state_path = base::StringPrintf(
+          kCpuFreqTimeInStatePathFormat, i);
+      DCHECK(!base::PathExists(base::FilePath(time_in_state_path)));

[Daniel Erat, 2014/02/15 15:31:25]

i don't understand. why are you asserting that a file doesn't exist just before
you try to read it?

[Siva Chandra, 2014/02/19 20:02:30]

I am lost too. Having '!' here is an error, but I don't understand why this has
been working at all!

+
+      std::string time_in_state_string;
+      // Note time as close to reading the file as possible. This is not
+      // possible for idle state samples as the information for each state there
+      // is recorded in different files.
+      base::Time now = base::Time::Now();
+      base::ReadFileToString(base::FilePath(time_in_state_path),

[Daniel Erat, 2014/02/15 15:31:25]

check return value

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+                             &time_in_state_string);
+      freq_sample.time = now;
+
+      std::vector<std::string> lines;
+      base::SplitString(time_in_state_string, '\n', &lines);
+      // The last line could end with '\n'. Ignore the last empty string in
+      // such cases.
+      unsigned int state_count = lines.size();

[Daniel Erat, 2014/02/15 15:31:25]

s/unsigned int/size_t/

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+      if (state_count > 0 && lines.back().empty()) {

[Daniel Erat, 2014/02/15 15:31:25]

omit curly brackets here

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+        state_count -= 1;
+      }
+      for (unsigned int k = 0; k < state_count; ++k) {
+        std::vector<std::string> pair;
+        // Occupancy of each state is in the format "<state> <time>"
+        base::SplitString(lines[k], ' ', &pair);

[Daniel Erat, 2014/02/15 15:31:25]

check that you got two parts

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+
+        int freq_in_khz;
+        int64 occupancy_time;
+        base::StringToInt(pair[0], &freq_in_khz);
+        base::StringToInt64(pair[1], &occupancy_time);

[Daniel Erat, 2014/02/15 15:31:25]

check return values

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+
+        // Freq state occupancy time is recorded in tens of milliseconds.
+        freq_sample.state_occupancy[base::IntToString(freq_in_khz / 1000)] =

[Daniel Erat, 2014/02/15 15:31:25]

why do you need to convert this to a string? keeping it as an int would be more
efficient in terms of storage space.

[Siva Chandra, 2014/02/19 20:02:30]

I did have it as an int initially. I changed it to being a string as idle states
are strings anyway. If we made these also strings, then we have only one sample
type and the rest of the code (as in power_ui.cc) is simpler/smaller as
consequence.

+            occupancy_time * 10;
+      }
+    }
+
+    freq_samples->push_back(freq_sample);
+  }
+
+  // If there was an interruption in sampling (like system suspended),
+  // re-sample!
+  if (base::TimeDelta(base::Time::Now() - startTime).InMilliseconds() > 500) {
+    freq_samples->clear();
+    SampleCpuFreqData(cpu_count, freq_samples);

[Daniel Erat, 2014/02/15 15:31:25]

same comment about recursion here

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  }
+}
+
+}  // namespace
+
+CpuDataCollector::CpuDataCollector() : cpu_count_(1), weak_ptr_factory_(this) {
+  DCHECK(base::PathExists(base::FilePath(kPossibleCpuPath)));

[Daniel Erat, 2014/02/15 15:31:25]

it'd be better to just log an error here and set cpu_count_ to 0 instead of
crashing

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  std::string possible_string;
+  base::ReadFileToString(base::FilePath(kPossibleCpuPath), &possible_string);
+  if (possible_string.find("-") != std::string::npos) {

[Daniel Erat, 2014/02/15 15:31:25]

also check that possible_string is long enough so you don't crash later if you
index beyond the end

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    int max_cpu;
+    // The possible CPUs are listed in the format "0-N". Hence, N is present in
+    // the substring starting at offset 2.
+    base::StringToInt(possible_string.substr(2), &max_cpu);

[Daniel Erat, 2014/02/15 15:31:25]

check return value

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    cpu_count_ = max_cpu + 1;
+  }
+
+  // Initialize the deques in the data vectors.
+  for (int i = 0; i < cpu_count_; ++i) {

[Daniel Erat, 2014/02/15 15:31:25]

just do:

  cpu_idle_state_data_.resize(cpu_count_);
  etc.

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    cpu_idle_state_data_.push_back(StateOccupancySampleDeque());
+    cpu_freq_state_data_.push_back(StateOccupancySampleDeque());
+  }
+}
+
+void CpuDataCollector::Start() {
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSeconds(kCpuDataSamplePeriodSec),
+               this,
+               &CpuDataCollector::PostSampleCpuState);
+}
+
+void CpuDataCollector::PostSampleCpuState() {
+  std::vector<StateOccupancySample>* idle_samples =
+      new std::vector<StateOccupancySample>;
+  std::vector<StateOccupancySample>* freq_samples =
+      new std::vector<StateOccupancySample>;
+  content::BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE,
+      base::Bind(&CpuDataCollector::SampleCpuStateOnBlockingPool,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Unretained(idle_samples),
+                 base::Unretained(freq_samples)),
+      base::Bind(&CpuDataCollector::SaveCpuStateSamplesOnUIThread,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(idle_samples),
+                 base::Owned(freq_samples)));
+}
+
+void CpuDataCollector::SampleCpuStateOnBlockingPool(
+    std::vector<CpuDataCollector::StateOccupancySample>* idle_samples,
+    std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
+  DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  SampleCpuIdleData(cpu_count_, idle_samples);
+  SampleCpuFreqData(cpu_count_, freq_samples);
+}
+
+void CpuDataCollector::SaveCpuStateSamplesOnUIThread(
+    const std::vector<CpuDataCollector::StateOccupancySample>* idle_samples,
+    const std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DCHECK(idle_samples->size() == cpu_idle_state_data_.size());

[Daniel Erat, 2014/02/15 15:31:25]

nit: DCHECK_EQ

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  for (unsigned int i = 0; i < cpu_idle_state_data_.size(); ++i) {

[Daniel Erat, 2014/02/15 15:31:25]

nit: omit curly brackets

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    AddSample(&cpu_idle_state_data_[i], (*idle_samples)[i]);

[Daniel Erat, 2014/02/15 15:31:25]

does this compile? i don't see where AddSample is declared or defined

[Siva Chandra, 2014/02/19 20:02:30]

AddSample is a template function defined in power_data_collector.h.

+  }
+
+  DCHECK(freq_samples->size() == cpu_freq_state_data_.size());

[Daniel Erat, 2014/02/15 15:31:25]

nit: DCHECK_EQ

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+  for (unsigned int i = 0; i < cpu_freq_state_data_.size(); ++i) {

[Daniel Erat, 2014/02/15 15:31:25]

nit: omit curly brackets

[Siva Chandra, 2014/02/19 20:02:30]

Done.

+    AddSample(&cpu_freq_state_data_[i], (*freq_samples)[i]);
+  }
+}
+
+CpuDataCollector::StateOccupancySample::StateOccupancySample()
+    : cpu_online(false) {
+}
+
+}  // namespace chromeos