Add parser for new cpu freq file

chrome/browser/chromeos/power/cpu_data_collector.cc

 // 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 "chrome/browser/chromeos/power/cpu_data_collector.h"
 
 #include <stddef.h>
 
 #include <vector>
 
@@ skipping 30 matching lines @@
 // Suffix of the path to the file listing the range of possible CPUs on the
 // system.
 const char kPossibleCpuPathSuffix[] = "/possible";
 
 // Format of the suffix of the path to the file which contains information
 // about a particular CPU being online or offline.
 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
 // about an idle state of a CPU on the system.
 const char kCpuIdleStateDirPathSuffixFormat[] = "/cpu%d/cpuidle/state%d";
 
 // Format of the suffix of the path to the file which contains the name of an
 // idle state of a CPU.
 const char kCpuIdleStateNamePathSuffixFormat[] = "/cpu%d/cpuidle/state%d/name";
 
 // Format of the suffix of the path which contains information about time spent
 // in an idle state on a CPU.
 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;
   }
 
   // If this is reached, then it means |str| is not present in vector.  Add it.
   vector->push_back(str);
   return vector->size() - 1;
 }
 
@@ skipping 17 matching lines @@
                               &cpu_online_string);
     if (base::StringToInt(cpu_online_string, &online))
       return online == kCpuOnlineStatus;
   }
 
   LOG(ERROR) << "Bad format or error reading " << cpu_online_file << ". "
              << "Assuming offline.";
   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) {
   base::Time start_time = base::Time::Now();
   for (int cpu = 0; cpu < cpu_count; ++cpu) {
     CpuDataCollector::StateOccupancySample idle_sample;
     idle_sample.time = base::Time::Now();
     idle_sample.time_in_state.reserve(cpu_idle_state_names->size());
 
     if (!CpuIsOnline(cpu)) {
@@ skipping 37 matching lines @@
           idle_samples->clear();
           return;
         }
 
         base::TrimWhitespaceASCII(state_name, base::TRIM_ALL, &state_name);
         base::TrimWhitespaceASCII(occupancy_time_string, base::TRIM_ALL,
                                   &occupancy_time_string);
         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;
         } else {
           LOG(ERROR) << "Bad format in " << time_file_path << ". "
                      << "Dropping sample.";
           idle_samples->clear();
           return;
         }
       }
     }
 
     idle_samples->push_back(idle_sample);
   }
 
   // If there was an interruption in sampling (like system suspended),
   // discard the samples!
   int64_t delay =
       base::TimeDelta(base::Time::Now() - start_time).InMilliseconds();
   if (delay > kSamplingDurationLimitMs) {
     idle_samples->clear();
     LOG(WARNING) << "Dropped an idle state sample due to excessive time delay: "
                  << delay << "milliseconds.";
   }
 }
 
-bool ReadCpuFreqFromOldFile(
+bool CpuDataCollector::ReadCpuFreqTimeInState(
     const std::string& path,
     std::vector<std::string>* cpu_freq_state_names,
     CpuDataCollector::StateOccupancySample* freq_sample) {
   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.
   if (!base::ReadFileToString(base::FilePath(path), &time_in_state_string)) {
     LOG(ERROR) << "Error reading " << path << ". "
                << "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;
 
     // 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;
+}
+
+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;
 }
 
-// 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(
+void CpuDataCollector::SampleCpuFreqData(
     int cpu_count,
     std::vector<std::string>* cpu_freq_state_names,
     std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
   base::Time start_time = base::Time::Now();
   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();
+    freq_sample.cpu_online = CpuIsOnline(cpu);
+    freq_samples->push_back(freq_sample);
+  }
 
-    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)) {
+  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 =
       base::TimeDelta(base::Time::Now() - start_time).InMilliseconds();
   if (delay > kSamplingDurationLimitMs) {
     freq_samples->clear();
     LOG(WARNING) << "Dropped a freq state sample due to excessive time delay: "
                  << delay << "milliseconds.";
   }
 }
 
-// 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,
     std::vector<std::string>* cpu_freq_state_names,
     std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {
   DCHECK(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   if (*cpu_count < 0) {
     // Set |cpu_count_| to 1. If it is something else, it will get corrected
     // later. A system will at least have one CPU. Hence, a value of 1 here
@@ skipping 26 matching lines @@
                    << "Defaulting CPU count to 1.";
       }
     }
   }
 
   // Initialize the deques in the data vectors.
   SampleCpuIdleData(*cpu_count, cpu_idle_state_names, idle_samples);
   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) {
 }
 
 CpuDataCollector::~CpuDataCollector() {
 }
 
 void CpuDataCollector::Start() {
   timer_.Start(FROM_HERE,
@@ skipping 75 matching lines @@
     : cpu_online(false) {
 }
 
 CpuDataCollector::StateOccupancySample::StateOccupancySample(
     const StateOccupancySample& other) = default;
 
 CpuDataCollector::StateOccupancySample::~StateOccupancySample() {
 }
 
 }  // namespace chromeos