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 223 matching lines @@
       freq_sample->time_in_state[index] = occupancy_time_centisecond * 10;
     } else {
       LOG(ERROR) << "Bad format in " << path << ". "
                  << "Dropping sample.";
       return false;
     }
   }
   return true;
 }
 
+bool ReadCpuFreqFromNewFile(

[Daniel Erat, 2017/05/01 19:46:32]

"old file" and "new file" aren't very descriptive. :-) please add comments to
the tops of both functions describing the differences between the two (i.e. the
standard locations of the files and an example of the content of each).

[weidongg, 2017/05/02 22:16:16]

Done.

+    int cpu_count,
+    const std::string& path,
+    std::vector<std::string>* cpu_freq_state_names,

[Daniel Erat, 2017/05/01 22:28:38]

high-level question about this code: is there any reason that we don't instead
pass a std::map<std::string, CpuDataCollector::StateOccupancySample>*? it seems
strange to have a separate vector that's just used to map names to indices.

[weidongg, 2017/05/02 22:16:15]

It seems that the author intentionally separated the indice with values. Putting
CpuDataCollector::StateOccupancySample in vector, it is easier to pop out them
from the vector and push them into the sample queues [1].

[1]
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/power/cpu_data_c...

+    std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) {

[Daniel Erat, 2017/05/01 19:46:32]

please document all of these parameters. freq_samples looks like it's supposed
to contain some existing information, for example.

[weidongg, 2017/05/02 22:16:16]

Done.

+  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;
+  }
+
+  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;

[Daniel Erat, 2017/05/01 19:46:32]

consider just using base::TrimWhitespaceASCII on the input string to drop
trailing whitespace without needing to walk the count back like this

[weidongg, 2017/05/02 22:16:16]

Done.

+  // The first line is descriptions in the format "freq\t\tcpu0\t\tcpu1...".
+  if (state_count > 0) {

[Daniel Erat, 2017/05/01 19:46:32]

if lines is empty, shouldn't you just return false?

[weidongg, 2017/05/02 22:16:15]

Yes, it is supposed to return false.

+    state_count -= 1;
+    lines.erase(lines.begin());
+  }
+  for (size_t state = 0; state < state_count; ++state) {
+    int freq_in_khz;
+
+    // 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 &&
+        base::StringToInt(array[0], &freq_in_khz)) {
+      const std::string state_name = base::IntToString(freq_in_khz / 1000);
+      size_t index = IndexInVector(state_name, cpu_freq_state_names);

[Daniel Erat, 2017/05/01 19:46:32]

presumably you need to check the index here.

[weidongg, 2017/05/01 21:50:10]

the index returned is always valid, as the state_name is added to the
cpu_freq_state_names if it does not exist.

[Daniel Erat, 2017/05/01 22:28:38]

sorry, i was confused by the name IndexInVector. would you mind renaming it to
something like EnsureInVector to make it more obvious that it'll insert the
element if it isn't already there?

[weidongg, 2017/05/02 22:16:16]

Done.

+      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;
+        base::StringToInt64(array[cpu + 1], &occupancy_time_centisecond);

[Daniel Erat, 2017/05/01 19:46:32]

you need to check the return value of this conversion

[weidongg, 2017/05/02 22:16:15]

Done.

+        (*freq_samples)[cpu].time_in_state[index] =
+            occupancy_time_centisecond * 10;
+      }
+    } else {

[Daniel Erat, 2017/05/01 19:46:32]

test for bad input and return false immediately; then you don't need to indent
the non-broken case

[weidongg, 2017/05/01 21:50:10]

Sorry, I don't understand, could you be more specific? Do you mean I need to
check format at the beginning and then return false immediately if it's bad
format?

[Daniel Erat, 2017/05/01 22:28:38]

i mean this code would be simpler if you did:

  if (array.size() != size_t(cpu_count) + 1 ||
      base::StringToInt(...)) {
    LOG(ERROR) << ...;
    return false;
  }

  const std::string state_name = ...;

[weidongg, 2017/05/02 22:16:15]

Ok, I see. Yes, that's better.

+      LOG(ERROR) << "Bad format in " << path << ". "
+                 << "Dropping sample.";
+      return false;
+    }
+  }
+  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(
     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(kCpuFreqTimeInStateNewPath))) {
+    if (!ReadCpuFreqFromNewFile(cpu_count, kCpuFreqTimeInStateNewPath,
+                                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, 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_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.";
   }
 }
@@ skipping 140 matching lines @@
     : cpu_online(false) {
 }
 
 CpuDataCollector::StateOccupancySample::StateOccupancySample(
     const StateOccupancySample& other) = default;
 
 CpuDataCollector::StateOccupancySample::~StateOccupancySample() {
 }
 
 }  // namespace chromeos