Add parser for new cpu freq file

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
index dc4c0cc84dd810fe9cb1eedb3ef957f975563837..c603f06a9c26a49e4f2086220444119dfe2aa887 100644
--- a/chrome/browser/chromeos/power/cpu_data_collector.cc
+++ b/chrome/browser/chromeos/power/cpu_data_collector.cc
@@ -48,12 +48,12 @@ const char kCpuOnlinePathSuffixFormat[] = "/cpu%d/online";
 
 // Format of the suffix of the path to the file which contains freq state
 // information of a CPU.
-const char kCpuFreqTimeInStatePathSuffixOldFormat[] =
+const char kCpuFreqTimeInStatePathSuffixFormat[] =
     "/cpu%d/cpufreq/stats/time_in_state";
 
-// The path to the file which contains cpu freq state informatino of a CPU
-// in newer kernel.
-const char kCpuFreqTimeInStateNewPath[] =
+// The path to the file which contains cpu freq state information of a CPU
+// in some kernels such as 3.14.0.
+const char kCpuFreqAllTimeInStatePath[] =
     "/sys/devices/system/cpu/cpufreq/all_time_in_state";
 
 // Format of the suffix of the path to the directory which contains information
@@ -70,7 +70,8 @@ const char kCpuIdleStateTimePathSuffixFormat[] = "/cpu%d/cpuidle/state%d/time";
 
 // Returns the index at which |str| is in |vector|. If |str| is not present in
 // |vector|, then it is added to it before its index is returned.
-size_t IndexInVector(const std::string& str, std::vector<std::string>* vector) {
+size_t EnsureInVector(const std::string& str,
+                      std::vector<std::string>* vector) {
   for (size_t i = 0; i < vector->size(); ++i) {
     if (str == (*vector)[i])
       return i;
@@ -108,10 +109,9 @@ bool CpuIsOnline(const int i) {
   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(
+}  // namespace
+
+void CpuDataCollector::SampleCpuIdleData(
     int cpu_count,
     std::vector<std::string>* cpu_idle_state_names,
     std::vector<CpuDataCollector::StateOccupancySample>* idle_samples) {
@@ -169,7 +169,7 @@ void SampleCpuIdleData(
         if (base::StringToInt64(occupancy_time_string, &occupancy_time_usec)) {
           // idle state occupancy time in sysfs is recorded in microseconds.
           int64_t time_in_state_ms = occupancy_time_usec / 1000;
-          size_t index = IndexInVector(state_name, cpu_idle_state_names);
+          size_t index = EnsureInVector(state_name, cpu_idle_state_names);
           if (index >= idle_sample.time_in_state.size())
             idle_sample.time_in_state.resize(index + 1);
           idle_sample.time_in_state[index] = time_in_state_ms;
@@ -196,7 +196,7 @@ void SampleCpuIdleData(
   }
 }
 
-bool ReadCpuFreqFromOldFile(
+bool CpuDataCollector::ReadCpuFreqTimeInState(
     const std::string& path,
     std::vector<std::string>* cpu_freq_state_names,
     CpuDataCollector::StateOccupancySample* freq_sample) {
@@ -209,14 +209,15 @@ bool ReadCpuFreqFromOldFile(
                << "Dropping sample.";
     return false;
   }
+  // The last line could end with '\n'. Ignore the last empty string in

[Daniel Erat, 2017/05/02 23:41:10]

shorter: "// Remove trailing newlines."

[weidongg, 2017/05/03 21:32:31]

Done.

+  // such cases.
+  base::TrimWhitespaceASCII(time_in_state_string,
+                            base::TrimPositions::TRIM_TRAILING,
+                            &time_in_state_string);
 
   std::vector<base::StringPiece> lines = base::SplitStringPiece(
       time_in_state_string, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-  // The last line could end with '\n'. Ignore the last empty string in
-  // such cases.
   size_t state_count = lines.size();
-  if (state_count > 0 && lines.back().empty())
-    state_count -= 1;
   for (size_t state = 0; state < state_count; ++state) {
     int freq_in_khz;
     int64_t occupancy_time_centisecond;
@@ -224,27 +225,91 @@ bool ReadCpuFreqFromOldFile(
     // Occupancy of each state is in the format "<state> <time>"
     std::vector<base::StringPiece> pair = base::SplitStringPiece(
         lines[state], " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-    if (pair.size() == 2 && base::StringToInt(pair[0], &freq_in_khz) &&
-        base::StringToInt64(pair[1], &occupancy_time_centisecond)) {
-      const std::string state_name = base::IntToString(freq_in_khz / 1000);
-      size_t index = IndexInVector(state_name, cpu_freq_state_names);
-      if (index >= freq_sample->time_in_state.size())
-        freq_sample->time_in_state.resize(index + 1);
-      // The occupancy time is in units of centiseconds.
-      freq_sample->time_in_state[index] = occupancy_time_centisecond * 10;
-    } else {
+    if (pair.size() != 2 || !base::StringToInt(pair[0], &freq_in_khz) ||
+        !base::StringToInt64(pair[1], &occupancy_time_centisecond)) {
       LOG(ERROR) << "Bad format in " << path << ". "
                  << "Dropping sample.";

[Daniel Erat, 2017/05/02 23:41:09]

include the bad line in the error message so this is easier to debug

[weidongg, 2017/05/03 21:32:31]

Done.

       return false;
     }
+
+    const std::string state_name = base::IntToString(freq_in_khz / 1000);
+    size_t index = EnsureInVector(state_name, cpu_freq_state_names);
+    if (index >= freq_sample->time_in_state.size())
+      freq_sample->time_in_state.resize(index + 1);
+    // The occupancy time is in units of centiseconds.

[Daniel Erat, 2017/05/02 23:41:10]

nit: remove this comment; the variable name occupancy_time_centisecond already
makes the units obvious

[weidongg, 2017/05/03 21:32:31]

Done.

+    freq_sample->time_in_state[index] = occupancy_time_centisecond * 10;
   }
   return true;
 }
 
-// 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(
+bool CpuDataCollector::ReadCpuFreqAllTimeInState(
+    int cpu_count,
+    const std::string& path,

[Daniel Erat, 2017/05/02 23:41:09]

pass paths as const base::FilePath&, not std::string

[weidongg, 2017/05/03 21:32:31]

Done.

+    std::vector<std::string>* cpu_freq_state_names,
+    std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
+  std::string all_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.
+  if (!base::ReadFileToString(base::FilePath(path),
+                              &all_time_in_state_string)) {
+    LOG(ERROR) << "Error reading " << path << ". "

[Daniel Erat, 2017/05/02 23:41:10]

LOG(ERROR) << "Error reading " << path << "; dropping sample.";

[weidongg, 2017/05/03 21:32:31]

Done.

+               << "Dropping sample.";
+    return false;
+  }
+  // The last line could end with '\n'. Ignore the last empty string in
+  // such cases.

[Daniel Erat, 2017/05/02 23:41:10]

nit: shorten this comment too

[weidongg, 2017/05/03 21:32:31]

Done.

+  base::TrimWhitespaceASCII(all_time_in_state_string,
+                            base::TrimPositions::TRIM_TRAILING,
+                            &all_time_in_state_string);
+
+  std::vector<base::StringPiece> lines =
+      base::SplitStringPiece(all_time_in_state_string, "\n",
+                             base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  size_t state_count = lines.size();

[Daniel Erat, 2017/05/02 23:41:10]

why not just do this instead of resizing, maintaining a separate count, etc.?

  // Skip the first line, which contains column names.
  for (size_t i = 1; i < lines.size(); ++i) {
    ...

[weidongg, 2017/05/03 21:32:31]

Yes, this is better.

+  // The first line is descriptions in the format "freq\t\tcpu0\t\tcpu1...".
+  if (state_count > 0) {
+    state_count -= 1;
+    lines.erase(lines.begin());
+  } else {
+    // No content exists in the file.
+    return false;
+  }
+  for (size_t state = 0; state < state_count; ++state) {
+    int freq_in_khz;

[Daniel Erat, 2017/05/02 23:41:10]

declare this just before it's used instead of up here:
https://google.github.io/styleguide/cppguide.html#Local_Variables

[weidongg, 2017/05/03 21:32:31]

Done.

+    // Occupancy of each state is in the format "<state>\t\t<time>\t\t<time>
+    // ..."
+    std::vector<base::StringPiece> array = base::SplitStringPiece(
+        lines[state], "\t", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+    if (array.size() != size_t(cpu_count) + 1 ||

[Daniel Erat, 2017/05/02 23:41:10]

use c++ casts, not c casts:
https://google.github.io/styleguide/cppguide.html#Casting

[weidongg, 2017/05/03 21:32:31]

Done.

+        !base::StringToInt(array[0], &freq_in_khz)) {
+      LOG(ERROR) << "Bad format in " << path << ". "
+                 << "Dropping sample.";

[Daniel Erat, 2017/05/02 23:41:10]

include the bad data in the error message

[weidongg, 2017/05/03 21:32:31]

Done.

+      return false;
+    }
+
+    const std::string state_name = base::IntToString(freq_in_khz / 1000);
+    size_t index = EnsureInVector(state_name, cpu_freq_state_names);
+    for (int cpu = 0; cpu < cpu_count; ++cpu) {
+      if (!(*freq_samples)[cpu].cpu_online) {
+        continue;
+      }
+      if (index >= (*freq_samples)[cpu].time_in_state.size())
+        (*freq_samples)[cpu].time_in_state.resize(index + 1);
+      int64_t occupancy_time_centisecond;
+      if (!base::StringToInt64(array[cpu + 1], &occupancy_time_centisecond)) {
+        LOG(ERROR) << "Bad format in " << path << ". "
+                   << "Dropping sample.";

[Daniel Erat, 2017/05/02 23:41:10]

include bad data

[weidongg, 2017/05/03 21:32:31]

Done.

+        return false;
+      }
+      (*freq_samples)[cpu].time_in_state[index] =
+          occupancy_time_centisecond * 10;
+    }
+  }
+  return true;
+}
+
+void CpuDataCollector::SampleCpuFreqData(
     int cpu_count,
     std::vector<std::string>* cpu_freq_state_names,
     std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
@@ -252,41 +317,42 @@ void SampleCpuFreqData(
   for (int cpu = 0; cpu < cpu_count; ++cpu) {
     CpuDataCollector::StateOccupancySample freq_sample;
     freq_sample.time_in_state.reserve(cpu_freq_state_names->size());
-
     freq_sample.time = base::Time::Now();
-    if (!CpuIsOnline(cpu)) {
-      freq_sample.cpu_online = false;
-    } else {
-      freq_sample.cpu_online = true;
-
-      const std::string time_in_state_path_old_format = base::StringPrintf(
-          "%s%s", kCpuDataPathBase, kCpuFreqTimeInStatePathSuffixOldFormat);
-      const std::string time_in_state_path =
-          base::StringPrintf(time_in_state_path_old_format.c_str(), cpu);
-      if (base::PathExists(base::FilePath(time_in_state_path))) {
-        if (!ReadCpuFreqFromOldFile(time_in_state_path, cpu_freq_state_names,
-                                    &freq_sample)) {
+    freq_sample.cpu_online = CpuIsOnline(cpu);
+    freq_samples->push_back(freq_sample);
+  }
+
+  if (base::PathExists(base::FilePath(kCpuFreqAllTimeInStatePath))) {
+    if (!ReadCpuFreqAllTimeInState(cpu_count, kCpuFreqAllTimeInStatePath,
+                                   cpu_freq_state_names, freq_samples)) {
+      freq_samples->clear();
+      return;
+    }
+  } else {
+    for (int cpu = 0; cpu < cpu_count; ++cpu) {
+      if ((*freq_samples)[cpu].cpu_online) {
+        const std::string time_in_state_path_old_format = base::StringPrintf(
+            "%s%s", kCpuDataPathBase, kCpuFreqTimeInStatePathSuffixFormat);
+        const std::string time_in_state_path =
+            base::StringPrintf(time_in_state_path_old_format.c_str(), cpu);
+        if (base::PathExists(base::FilePath(time_in_state_path))) {
+          if (!ReadCpuFreqTimeInState(time_in_state_path, cpu_freq_state_names,
+                                      &(*freq_samples)[cpu])) {
+            freq_samples->clear();
+            return;
+          }
+        } else {
+          // If the path to the 'time_in_state' for a single CPU is missing,
+          // then 'time_in_state' for all CPUs is missing. This could happen
+          // on a VM where the 'cpufreq_stats' kernel module is not loaded.
+          LOG_IF(ERROR, base::SysInfo::IsRunningOnChromeOS())
+              << "CPU freq stats not available in sysfs.";
           freq_samples->clear();
           return;
         }
-      } else if (base::PathExists(base::FilePath(kCpuFreqTimeInStateNewPath))) {
-        // TODO(oshima): Parse the new file. crbug.com/548510.
-        freq_samples->clear();
-        return;
-      } else {
-        // If the path to the 'time_in_state' for a single CPU is missing,
-        // then 'time_in_state' for all CPUs is missing. This could happen
-        // on a VM where the 'cpufreq_stats' kernel module is not loaded.
-        LOG_IF(ERROR, base::SysInfo::IsRunningOnChromeOS())
-            << "CPU freq stats not available in sysfs.";
-        freq_samples->clear();
-        return;
       }
     }
-
-    freq_samples->push_back(freq_sample);
   }
-
   // If there was an interruption in sampling (like system suspended),
   // discard the samples!
   int64_t delay =
@@ -298,12 +364,7 @@ void SampleCpuFreqData(
   }
 }
 
-// Samples CPU idle and CPU freq data from sysfs. This function should run on
-// the blocking pool as reading from sysfs is a blocking task. Elements at
-// index i in |idle_samples| and |freq_samples| correspond to the idle and
-// freq samples of CPU i. This also function reads the number of CPUs from
-// sysfs if *|cpu_count| < 0.
-void SampleCpuStateAsync(
+void CpuDataCollector::SampleCpuStateAsync(
     int* cpu_count,
     std::vector<std::string>* cpu_idle_state_names,
     std::vector<CpuDataCollector::StateOccupancySample>* idle_samples,
@@ -350,8 +411,6 @@ void SampleCpuStateAsync(
   SampleCpuFreqData(*cpu_count, cpu_freq_state_names, freq_samples);
 }
 
-}  // namespace
-
 // Set |cpu_count_| to -1 and let SampleCpuStateAsync discover the
 // correct number of CPUs.
 CpuDataCollector::CpuDataCollector() : cpu_count_(-1), weak_ptr_factory_(this) {