ProcessMetrics::GetCPUUsage() is Slow

base/process/process_metrics_linux.cc

 // Copyright (c) 2013 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 "base/process/process_metrics.h"
 
 #include <dirent.h>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <sys/time.h>
@@ skipping 381 matching lines @@
   return ret;
 }
 
 size_t GetSystemCommitCharge() {
   SystemMemoryInfoKB meminfo;
   if (!GetSystemMemoryInfo(&meminfo))
     return 0;
   return meminfo.total - meminfo.free - meminfo.buffers - meminfo.cached;
 }
 
-// Exposed for testing.
 int ParseProcStatCPU(const std::string& input) {
-  std::vector<std::string> proc_stats;
-  if (!internal::ParseProcStats(input, &proc_stats))
+  if (input.empty())

[danakj, 2015/02/04 01:29:18]

Can you leave a comment like ParseProcStats?

// |input| may be empty if the process disappeared somehow.
// e.g. http://crbug.com/145811

[afakhry, 2015/02/04 02:31:22]

Done.

     return -1;
 
-  if (proc_stats.size() <= internal::VM_STIME)
+  // The index of the last space before 'utime', taking the space after the last
+  // ')' as the beginning.
+  int last_space_index = internal::VM_UTIME - 1;

[danakj, 2015/02/04 01:29:18]

why do you call this index? they seem like counts, would num_spaces_remaining be
accurate for this variable?

[afakhry, 2015/02/04 02:31:22]

Renamed.

+  int utime_start_index = 0;

[oshima, 2015/02/04 01:49:46]

move these variable after next if block

[afakhry, 2015/02/04 02:31:22]

Done.

+  auto start = input.find_last_of(')');

[oshima, 2015/02/04 01:49:46]

just use size_t. it isn't worth it imo.

[afakhry, 2015/02/04 02:31:22]

Done.

+  if (start == input.npos || start == 0)

[danakj, 2015/02/04 01:29:18]

why do you have to check for 0 here?

     return -1;
-  int utime = GetProcStatsFieldAsInt64(proc_stats, internal::VM_UTIME);
-  int stime = GetProcStatsFieldAsInt64(proc_stats, internal::VM_STIME);
+
+  auto end = input.size() - 1;

[danakj, 2015/02/04 01:29:18]

can you call this last instead of end? end is usually past the end, ie size(),
or end().

[afakhry, 2015/02/04 02:31:22]

Done.

+  for (auto j = start + 1; j < end; ++j) {

[danakj, 2015/02/04 01:29:18]

a nit, but why j instead of i? usually we'd use i for an outermost loop.

can you leave a comment mentioning why it skips the last char?

+    if (input[j] != ' ' && input[j - 1] == ' ') {

[danakj, 2015/02/04 01:29:18]

could you just string::find(" ") from the current position for the next space N
times, then read the numbers there?

[oshima, 2015/02/04 01:49:46]

I think he wanted to deal with two space case. I'm fine to use find
if you think it's not important.

It's easier to understand if you flip j and j-1 if we want to keep this way.

[danakj, 2015/02/04 01:52:54]

Can just skip past contiguous spaces if that can happen in the input. Something
like this, modulo off-by-ones

while (...) {
  pos = std::find(" ", pos);
  if (pos == npos) break;
  while (*pos == ' ') ++pos;
  --num_spaces_remaining;

[oshima, 2015/02/04 02:28:37]

That's fine too. I don't have much preference.

[afakhry, 2015/02/04 02:31:22]

Done.

+      if (--last_space_index == 0) {
+        utime_start_index = j;
+        break;
+      }

[oshima, 2015/02/04 01:49:46]

optional: you can actually increase j here because you know [j] isn't ' '.

+    }
+  }
+
+  if (last_space_index != 0)
+    return -1;
+
+  int utime = 0;
+  int stime = 0;
+  if (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != 2)

[danakj, 2015/02/04 01:29:18]

Why not StringToInt instead of sscanf?

[oshima, 2015/02/04 02:28:37]

Is there any issue with sscanf?

[afakhry, 2015/02/04 02:31:22]

Because it's much simpler. No additional parsing is needed once you find the
index of utime. Used to extract the two values we're interested in from a
formatted input.

+    return -1;
+
   return utime + stime;
 }
 
 const char kProcSelfExe[] = "/proc/self/exe";
 
 int GetNumberOfThreads(ProcessHandle process) {
   return internal::ReadProcStatsAndGetFieldAsInt64(process,
                                                    internal::VM_NUMTHREADS);
 }
 
@@ skipping 466 matching lines @@
 #if defined(OS_LINUX)
 int ProcessMetrics::GetIdleWakeupsPerSecond() {
   uint64 wake_ups;
   const char kWakeupStat[] = "se.statistics.nr_wakeups";
   return ReadProcSchedAndGetFieldAsUint64(process_, kWakeupStat, &wake_ups) ?
       CalculateIdleWakeupsPerSecond(wake_ups) : 0;
 }
 #endif  // defined(OS_LINUX)
 
 }  // namespace base