Proposal: Use /usr/bin/top in about:memory...

chrome/browser/process_info_snapshot_mac.cc

Index: chrome/browser/process_info_snapshot_mac.cc
===================================================================
--- chrome/browser/process_info_snapshot_mac.cc	(revision 69842)
+++ chrome/browser/process_info_snapshot_mac.cc	(working copy)
@@ -5,16 +5,15 @@
 #include "chrome/browser/process_info_snapshot.h"
 
 #include <sstream>
+#include <sys/sysctl.h>

[Mark Mentovai, 2011/01/04 20:09:31]

C system headers should precede C++ system headers and should be in a separate
section.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

[sail, 2011/01/06 00:22:59]

Done.

 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
+#include "base/sys_info.h"
 #include "base/thread.h"
 
-// Implementation for the Mac; calls '/bin/ps' for information when
-// |Sample()| is called.
-
 // Default constructor.
 ProcessInfoSnapshot::ProcessInfoSnapshot() { }
 
@@ -25,30 +24,117 @@
 
 const size_t ProcessInfoSnapshot::kMaxPidListSize = 1000;
 
-// Capture the information by calling '/bin/ps'.
-// Note: we ignore the "tsiz" (text size) display option of ps because it's
-// always zero (tested on 10.5 and 10.6).
-bool ProcessInfoSnapshot::Sample(std::vector<base::ProcessId> pid_list) {
-  const char* kPsPathName = "/bin/ps";
-  Reset();
+static bool GetKInfoForProcessID(kinfo_proc* kinfo, pid_t pid) {

[Mark Mentovai, 2011/01/04 20:09:31]

I suggest putting “in” parameters before “out” ones. Same on line 43.

[sail, 2011/01/06 00:22:59]

Done.

+  int mib[4];

[Mark Mentovai, 2011/01/04 20:09:31]

Why not just use int mib[] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, pid};?

[sail, 2011/01/06 00:22:59]

Done.

+  mib[0] = CTL_KERN;
+  mib[1] = KERN_PROC;
+  mib[2] = KERN_PROC_PID;
+  mib[3] = pid;
 
-  // Nothing to do if no PIDs given.
-  if (pid_list.size() == 0)
-    return true;
-  if (pid_list.size() > kMaxPidListSize) {
-    // The spec says |pid_list| *must* not have more than this many entries.
+  size_t miblen = 4;

[Mark Mentovai, 2011/01/04 20:09:31]

Why not just use arraysize(mib)? (You don’t even need a separate variable for
it.)

[sail, 2011/01/06 00:22:59]

Done.

+  size_t len = sizeof(*kinfo);
+  if (sysctl(mib, miblen, kinfo, &len, NULL, 0) != 0) {
+    PLOG(ERROR) << "sysctl() for KERN_PROC";
+    return false;
+  }
+  return true;
+}
+
+static bool GetExecutableNameForProcessID(char** executable_name, pid_t pid) {

[Mark Mentovai, 2011/01/04 20:09:31]

The interface would probably be better if the out parameter were an
std::string*. That would make ownership much clearer.

[sail, 2011/01/06 00:22:59]

Done.

+  static bool s_should_init_arg_max = true;

[Mark Mentovai, 2011/01/04 20:09:31]

These usually work a little better in terms of code size, data size, or both
when the initial state is 0 (false). Use the power of zero to your advantage.

[sail, 2011/01/06 00:22:59]

Done.

+  static int s_arg_max = 0;

[Mark Mentovai, 2011/01/04 20:09:31]

You may not need two separate variables here, either. s_arg_max == 0 can just be
the signal that initialization never happened. This will mean that you’ll keep
retrying the sysctl each time through the function if it keeps failing for some
reason, but that should be harmless. Less code, less data.

[sail, 2011/01/06 00:22:59]

Done.

+
+  if (!executable_name) {
     NOTREACHED();
     return false;
   }
 
+  if (s_should_init_arg_max) {

[Mark Mentovai, 2011/01/04 20:09:31]

The declarations should be closer to the point of first use. This is the point
of first use.

[sail, 2011/01/06 00:22:59]

Done.

+    s_should_init_arg_max = false;
+    int mib[2];

[Mark Mentovai, 2011/01/04 20:09:31]

Simplify mib and size as discussed above. Same on 67-76.

[sail, 2011/01/06 00:22:59]

Done.

+    mib[0] = CTL_KERN;
+    mib[1] = KERN_ARGMAX;
+    size_t size = sizeof(s_arg_max);
+    if (sysctl(mib, 2, &s_arg_max, &size, NULL, 0) != 0) {
+      PLOG(ERROR) << "sysctl() for KERN_ARGMAX";
+    }
+  }
+
+  if (s_arg_max == 0) {
+    return false;
+  }
+
+  int mib[3];
+  mib[0] = CTL_KERN;
+  mib[1] = KERN_PROCARGS;
+  mib[2] = pid;
+  char *arg = new char[s_arg_max];
+  size_t size = s_arg_max;
+  if (sysctl(mib, 3, arg, &size, NULL, 0) != 0) {
+    // Don't log the error since it's normal for this to fail.
+    return false;

[Mark Mentovai, 2011/01/04 20:09:31]

Leaks arg? See, the ownership model is spaghetti here.

This is something else that would be cleaner with std::string *executable_name.
Instead of doing |new char|, you would just do
|executable_name->resize(s_arg_max + 1);| and then pass |&(*executable_name)[0]|
 to sysctl.

Of course, you’d probably also want to find the first NUL character before
returning, and resizing the string so that it ONLY refers to the executable name
and not the whole argv, envp, apple, etc.

[sail, 2011/01/06 00:22:59]

Done.

+  }
+
+  *executable_name = arg;
+  return true;
+}
+
+// The units are based on humanize_number(). See:
+// http://www.opensource.apple.com/source/libutil/libutil-21/humanize_number.c

[Mark Mentovai, 2011/01/04 20:09:31]

The function name doesn’t really tell me what to pass for the arguments or what
to expect the function to do, and neither does this comment.

[sail, 2011/01/06 00:22:59]

Done.

+static bool ConvertUnitToScale(std::string unit, uint64_t* scale) {

[Mark Mentovai, 2011/01/04 20:09:31]

unit can be a const std::string&.

unit can also just be a char. That might be even better.

[sail, 2011/01/06 00:22:59]

Done.

+  if (unit.size() == 0) {
+    return false;
+  }
+
+  uint64_t shift = 0;

[Mark Mentovai, 2011/01/04 20:09:31]

This is fine as just an int.

[sail, 2011/01/06 00:23:00]

Done.

+  switch (unit[0]) {
+    case 'B':
+      shift = 0;
+      break;
+    case 'K':

[Mark Mentovai, 2011/01/04 20:09:31]

Care about lowercase 'k'?

[sail, 2011/01/06 00:23:00]

Done. Libtop doesn't use lowercase 'k' but it doesn't hurt to be able to parse
it.

+      shift = 1;
+      break;
+    case 'M':
+      shift = 2;
+      break;
+    case 'G':
+      shift = 3;
+      break;
+    case 'T':
+      shift = 4;
+      break;
+    case 'P':
+      shift = 5;
+      break;
+    case 'E':
+      shift = 6;
+      break;
+    default:
+      return false;
+  }
+
+  *scale = 1;
+  for (size_t i = 0; i < shift; i++) {

[Mark Mentovai, 2011/01/04 20:09:31]

This is fine as just an int too.

[sail, 2011/01/06 00:23:00]

Done.

+    *scale *= 1024;

[Mark Mentovai, 2011/01/04 20:09:31]

Rather than dereferencing scale each time through the loop, why not do the work
on a local temporary and then stash it in scale after the loop?

As an alternative, you can have this function return scale as a uint64_t instead
of a bool. You can use 0 as a special “I failed” return value.

[sail, 2011/01/06 00:23:00]

Done.

+  }
+  return true;
+}
+
+// Capture the information by calling '/bin/ps'.
+// Note: we ignore the "tsiz" (text size) display option of ps because it's
+// always zero (tested on 10.5 and 10.6).
+static bool GetProcessMemoryInfoUsingPS(
+    const std::vector<base::ProcessId>& pid_list,
+    std::map<int,ProcessInfoSnapshot::ProcInfoEntry>& proc_info_entries) {
+  const char* kPsPathName = "/bin/ps";

[Mark Mentovai, 2011/01/04 20:09:31]

Some of the comments below, like using a const char[], might be relevant in this
function too.

[sail, 2011/01/06 00:23:00]

Done.

   std::vector<std::string> argv;
   argv.push_back(kPsPathName);
-  // Get PID, PPID, (real) UID, effective UID, resident set size, virtual memory
-  // size, and command.
+
+  // Get resident set size, virtual memory size.
   argv.push_back("-o");
-  argv.push_back("pid=,ppid=,ruid=,uid=,rss=,vsz=,comm=");
+  argv.push_back("pid=,rss=,vsz=");
   // Only display the specified PIDs.
-  for (std::vector<base::ProcessId>::iterator it = pid_list.begin();
+  for (std::vector<base::ProcessId>::const_iterator it = pid_list.begin();
       it != pid_list.end(); ++it) {
     argv.push_back("-p");
     argv.push_back(base::Int64ToString(static_cast<int64>(*it)));
@@ -67,20 +153,21 @@
 
   // Process lines until done.
   while (true) {
-    ProcInfoEntry proc_info;
-
     // The format is as specified above to ps (see ps(1)):
-    //   "-o pid=,ppid=,ruid=,uid=,rss=,vsz=,comm=".
+    //   "-o pid=,rss=,vsz=".
     // Try to read the PID; if we get it, we should be able to get the rest of
     // the line.
-    in >> proc_info.pid;
+    pid_t pid;
+    in >> pid;
     if (in.eof())
       break;
-    in >> proc_info.ppid;
-    in >> proc_info.uid;
-    in >> proc_info.euid;
+
+    ProcessInfoSnapshot::ProcInfoEntry proc_info = proc_info_entries[pid];
+    proc_info.pid = pid;
     in >> proc_info.rss;
     in >> proc_info.vsize;
+    proc_info.rss *= 1024;                // Convert from kilobytes to bytes.
+    proc_info.vsize *= 1024;
     in.ignore(1, ' ');                    // Eat the space.
     std::getline(in, proc_info.command);  // Get the rest of the line.
     if (!in.good()) {
@@ -88,24 +175,218 @@
       return false;
     }
 
-    // Make sure the new PID isn't already in our list.
-    if (proc_info_entries_.find(proc_info.pid) != proc_info_entries_.end()) {
-      LOG(ERROR) << "Duplicate PID in output from " << kPsPathName << ".";
-      return false;
-    }
-
     if (!proc_info.pid || ! proc_info.vsize) {
       LOG(WARNING) << "Invalid data from " << kPsPathName << ".";
       return false;
     }
 
     // Record the process information.
-    proc_info_entries_[proc_info.pid] = proc_info;
+    proc_info_entries[proc_info.pid] = proc_info;
   }
 
   return true;
 }
 
+static bool GetProcessMemoryInfoUsingTop(
+    std::map<int,ProcessInfoSnapshot::ProcInfoEntry>& proc_info_entries) {
+  const char* kPsPathName = "/usr/bin/top";

[Mark Mentovai, 2011/01/04 20:09:31]

Isn’t this more like kTopPathName?

And wouldn’t you prefer a const char[]?

[sail, 2011/01/06 00:23:00]

Done.

+  std::vector<std::string> argv;
+  argv.push_back(kPsPathName);
+
+  // -stats tells top to print just the given fields as ordered.
+  argv.push_back("-stats");
+  argv.push_back("pid,"    // Process ID
+                 "rsize,"  // Resident memory
+                 "rshrd,"  // Resident shared memory
+                 "rprvt,"  // Resident private memory
+                 "vsize"); // Total virtual memory
+  // Run top in logging (non-interactive) mode.
+  argv.push_back("-l");
+  argv.push_back("1");
+
+  std::string output;
+  CommandLine command_line(argv);
+  // Limit output read to a megabyte for safety.
+  if (!base::GetAppOutputRestricted(command_line, &output, 1024 * 1024)) {
+    LOG(ERROR) << "Failure running " << kPsPathName << " to acquire data.";
+    return false;
+  }
+
+  // Process lines until done.
+  std::istringstream topIn(output, std::istringstream::in);

[Mark Mentovai, 2011/01/04 20:09:31]

top_in, not topIn.

[sail, 2011/01/06 00:23:00]

Done.

+  std::string line;
+  while (std::getline(topIn, line)) {

[Mark Mentovai, 2011/01/04 20:09:31]

Show an example line or two, so that the reader has some sense of the expected
data without having to cobble together a top invocation manually to figure it
out.

[sail, 2011/01/06 00:23:00]

Done.

+    std::istringstream in(line, std::istringstream::in);
+
+    // Try to read the PID.
+    pid_t pid;
+    in >> pid;
+    if (in.fail())
+      continue;
+
+    // Make sure that caller is interested in this process.
+    if (proc_info_entries.find(pid) == proc_info_entries.end())
+      continue;
+
+    // Skip the - or + sign that top puts after the pid.
+    in.get();
+
+    uint64_t values[4];
+    size_t i;
+    for (i = 0; i < arraysize(values); i++) {
+      in >> values[i];
+      if (in.fail())
+        break;
+      std::string unit;
+      in >> unit;
+      if (in.fail())
+        break;
+
+      uint64_t scale;
+      if (!ConvertUnitToScale(unit, &scale)) {
+        break;
+      }
+      values[i] *= scale;
+    }
+    if (i != arraysize(values))
+      continue;
+
+    ProcessInfoSnapshot::ProcInfoEntry proc_info = proc_info_entries[pid];
+    proc_info.rss = values[0];
+    proc_info.rshrd = values[1];
+    proc_info.rprvt = values[2];
+    proc_info.vsize = values[3];
+    // Record the process information.
+    proc_info_entries[proc_info.pid] = proc_info;
+  }
+
+  return true;
+}
+
+static bool GetProcessMemoryInfoUsingTop_10_5(
+    std::map<int,ProcessInfoSnapshot::ProcInfoEntry>& proc_info_entries) {
+  const char* kPsPathName = "/usr/bin/top";

[Mark Mentovai, 2011/01/04 20:09:31]

Refer to similar comments above.

[sail, 2011/01/06 00:23:00]

Done.

+  std::vector<std::string> argv;
+  argv.push_back(kPsPathName);
+
+  // -p tells top to print just the given fields as ordered.
+  argv.push_back("-p");
+  argv.push_back("^aaaaaaaaaaaaaaaaaaaa "  // Process ID (PID)
+                 "^jjjjjjjjjjjjjjjjjjjj "  // Resident memory (RSIZE)
+                 "^iiiiiiiiiiiiiiiiiiii "  // Resident shared memory (RSHRD)
+                 "^hhhhhhhhhhhhhhhhhhhh "  // Resident private memory (RPRVT)
+                 "^llllllllllllllllllll"); // Total virtual memory (VSIZE)
+  // Run top in logging (non-interactive) mode.
+  argv.push_back("-l");
+  argv.push_back("1");
+
+  std::string output;
+  CommandLine command_line(argv);
+  // Limit output read to a megabyte for safety.
+  if (!base::GetAppOutputRestricted(command_line, &output, 1024 * 1024)) {
+    LOG(ERROR) << "Failure running " << kPsPathName << " to acquire data.";
+    return false;
+  }
+
+  // Process lines until done.
+  std::istringstream topIn(output, std::istringstream::in);
+  std::string line;
+  while (std::getline(topIn, line)) {
+    std::istringstream in(line, std::istringstream::in);
+
+    // Try to read the PID.
+    pid_t pid;
+    in >> pid;
+    if (in.fail())
+      continue;
+
+    // Make sure that caller is interested in this process.
+    if (proc_info_entries.find(pid) == proc_info_entries.end())
+      continue;
+
+    uint64_t values[4];
+    size_t i;
+    for (i = 0; i < arraysize(values); i++) {
+      in >> values[i];
+      if (in.fail())
+        break;
+    }
+    if (i != arraysize(values))
+      continue;
+
+    ProcessInfoSnapshot::ProcInfoEntry proc_info = proc_info_entries[pid];
+    proc_info.rss = values[0];
+    proc_info.rshrd = values[1];
+    proc_info.rprvt = values[2];
+    proc_info.vsize = values[3];
+    // Record the process information.
+    proc_info_entries[proc_info.pid] = proc_info;
+  }
+
+  return true;
+}
+
+
+bool ProcessInfoSnapshot::Sample(std::vector<base::ProcessId> pid_list) {
+  Reset();
+
+  // Nothing to do if no PIDs given.
+  if (pid_list.size() == 0)
+    return true;
+  if (pid_list.size() > kMaxPidListSize) {
+    // The spec says |pid_list| *must* not have more than this many entries.
+    NOTREACHED();
+    return false;
+  }
+
+  // Get basic process info from KERN_PROC.
+  for (std::vector<base::ProcessId>::iterator it = pid_list.begin();
+       it != pid_list.end(); ++it) {
+    ProcInfoEntry proc_info;
+    proc_info.pid = *it;
+
+    kinfo_proc kinfo;
+    if (!GetKInfoForProcessID(&kinfo, *it)) {
+      return false;
+    }
+
+    proc_info.ppid = kinfo.kp_eproc.e_ppid;
+    proc_info.uid = kinfo.kp_eproc.e_pcred.p_ruid;
+    proc_info.euid = kinfo.kp_eproc.e_ucred.cr_uid;
+    // Note, p_comm is truncated to 15 characters.

[Mark Mentovai, 2011/01/04 20:09:31]

Really? 15 or 16? p_comm is a char[MAXCOMLEN + 1], and MAXCOMLEN is 16.

[sail, 2011/01/06 00:23:00]

Ahh, you're right. Fixed.

+    proc_info.command = kinfo.kp_proc.p_comm;
+    proc_info_entries_[*it] = proc_info;
+  }
+
+  // Use KERN_PROCARGS to get the full executable name. This may fail if we

[Mark Mentovai, 2011/01/04 20:09:31]

we?

[sail, 2011/01/06 00:23:00]

Done.

+  // don't have privileges to inspect that process.
+  for (std::vector<base::ProcessId>::iterator it = pid_list.begin();
+       it != pid_list.end(); ++it) {
+    char* exectuable_name = NULL;
+    if (GetExecutableNameForProcessID(&exectuable_name, *it)) {
+      ProcInfoEntry proc_info = proc_info_entries_[*it];
+      proc_info.command = exectuable_name;
+      delete exectuable_name;

[Mark Mentovai, 2011/01/04 20:09:31]

See? Doesn’t this suck?

[sail, 2011/01/06 00:23:00]

Done.

+    }
+  }
+
+  // Get memory information using top.
+  bool memoryInfoSuccess = false;

[Mark Mentovai, 2011/01/04 20:09:31]

memory_info_success, because of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

[sail, 2011/01/06 00:23:00]

Done.

+  int32 major, minor, bugfix;
+  base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &bugfix);
+  if (major == 10 && minor == 5)
+    memoryInfoSuccess = GetProcessMemoryInfoUsingTop_10_5(proc_info_entries_);
+  else
+    memoryInfoSuccess = GetProcessMemoryInfoUsingTop(proc_info_entries_);

[Mark Mentovai, 2011/01/04 20:09:31]

I wouldn’t even try this unless ((major == 10 && minor >= 6) || major > 10).

[sail, 2011/01/06 00:23:00]

Done.

+
+  // If top didn't work then fallback to ps.

[Mark Mentovai, 2011/01/04 20:09:31]

“Fall back” as a verb is two words.

[sail, 2011/01/06 00:23:00]

Done.

+  if (!memoryInfoSuccess)
+    memoryInfoSuccess = GetProcessMemoryInfoUsingPS(pid_list,

[Mark Mentovai, 2011/01/04 20:09:31]

This needs {braces} because it’s a multi-line.

[sail, 2011/01/06 00:23:00]

Done.

+                                                    proc_info_entries_);
+
+  return memoryInfoSuccess;
+}
+
 // Clear all the stored information.
 void ProcessInfoSnapshot::Reset() {
   proc_info_entries_.clear();
@@ -139,7 +420,7 @@
     return false;
   }
 
-  usage->priv = proc_info.vsize;
+  usage->priv = proc_info.vsize / 1024;
   usage->mapped = 0;
   usage->image = 0;
   return true;
@@ -163,8 +444,8 @@
     return false;
   }
 
-  ws_usage->priv = 0;
-  ws_usage->shareable = proc_info.rss;
-  ws_usage->shared = 0;
+  ws_usage->priv = proc_info.rprvt / 1024;
+  ws_usage->shareable = proc_info.rss / 1024;
+  ws_usage->shared = proc_info.rshrd / 1024;
   return true;
 }