Mac: Shared and Private memory for Task Manager...

base/process_util_mac.mm

Index: base/process_util_mac.mm
===================================================================
--- base/process_util_mac.mm	(revision 68959)
+++ base/process_util_mac.mm	(working copy)
@@ -11,6 +11,8 @@
 #include <mach/mach.h>
 #include <mach/mach_init.h>
 #include <mach/task.h>
+#include <mach/mach_vm.h>

[viettrungluu, 2010/12/21 04:21:06]

Nit: alphabetical order (see style guide).

[sail, 2010/12/21 18:15:33]

Done.

+#include <mach/shared_region.h>
 #include <malloc/malloc.h>
 #import <objc/runtime.h>
 #include <spawn.h>
@@ -22,6 +24,7 @@
 
 #include <new>
 #include <string>
+#include <map>

[viettrungluu, 2010/12/21 04:21:06]

Ditto.

[sail, 2010/12/21 18:15:33]

Done.

 
 #include "base/debug/debugger.h"
 #include "base/eintr_wrapper.h"
@@ -235,13 +238,144 @@
   return 0;
 }
 
-// OSX appears to use a different system to get its memory.
+static bool GetCPUTypeForProcess(pid_t pid, cpu_type_t* cpuType) {

[viettrungluu, 2010/12/21 04:21:06]

Nit: This is a C++-ish function, so the parameter should be called |cpu_type|.

[sail, 2010/12/21 18:15:33]

Done.

+  size_t len = sizeof(*cpuType);
+  int result = sysctlbyname("sysctl.proc_cputype",
+                            cpuType,
+                            &len,
+                            NULL,
+                            0);
+  if (result != 0) {
+    LOG(ERROR) << "Call to sysctlbyname failed with error "
+               << strerror(errno);
+    return false;
+  }
+
+  return true;
+}
+
+// From libtop in_shared_region().

[viettrungluu, 2010/12/21 04:21:06]

If this is basically copied, then we need to do something about it
copyright-wise. (At the very least, we'd need to indicate the original
copyright. Possibly it should go in third_party/apple or something like that.)

It may be sufficiently simple (and there's lots which you could trim out, since
we don't support PPC or ARM) that perhaps you could make a sufficiently
non-derivative version....

[sail, 2010/12/21 18:15:33]

Implemented a very simple version that just handles the CPU types that we
support.

+static bool IsAddressInSharedRegion(mach_vm_address_t addr, cpu_type_t type) {
+  mach_vm_address_t base = 0, size = 0;

[viettrungluu, 2010/12/21 04:21:06]

Nit: We don't declare multiple variables in the same statement.

+
+  switch(type) {
+    case CPU_TYPE_ARM:
+    base = SHARED_REGION_BASE_ARM;

[viettrungluu, 2010/12/21 04:21:06]

Nit: Indentation.

[sail, 2010/12/21 18:15:33]

Done.

[shaneelaticejohnson, 2012/05/26 05:06:43]

Done.

+    size = SHARED_REGION_SIZE_ARM;
+    break;
+
+    case CPU_TYPE_X86_64:
+      base = SHARED_REGION_BASE_X86_64;
+      size = SHARED_REGION_SIZE_X86_64;
+      break;
+
+    case CPU_TYPE_I386:
+      base = SHARED_REGION_BASE_I386;
+      size = SHARED_REGION_SIZE_I386;
+      break;
+
+    case CPU_TYPE_POWERPC:
+      base = SHARED_REGION_BASE_PPC;
+      size = SHARED_REGION_SIZE_PPC;
+      break;
+
+    case CPU_TYPE_POWERPC64:
+      base = SHARED_REGION_BASE_PPC64;
+      size = SHARED_REGION_SIZE_PPC64;
+      break;
+
+    default:
+      // Unknown CPU type.
+      break;
+  }
+
+  return(addr >= base && addr < (base + size));

[viettrungluu, 2010/12/21 04:21:06]

Nit: No parentheses (and even if you felt they were needed for some reason,
there should be a space after "return").

[sail, 2010/12/21 18:15:33]

Done.

+}
+
+// This is a rough approximation of the algorithm that libtop uses.
+// private_bytes is the size of private resident memory.
+// shared_bytes is the size of shared resident memory.
+// Note, libtop doesn't include shared memory that is referenced by other
+// process but we do since we only look at a single process at a time.
 bool ProcessMetrics::GetMemoryBytes(size_t* private_bytes,
                                     size_t* shared_bytes) {
-  if (private_bytes)
-    *private_bytes = 0;
-  if (shared_bytes)
-    *shared_bytes = 0;
+  kern_return_t kr;
+
+  mach_port_t task = TaskForPid(process_);
+  if (task == MACH_PORT_NULL) {
+    LOG(ERROR) << "Invalid process";
+    return false;
+  }
+
+  vm_size_t pageSize;

[viettrungluu, 2010/12/21 04:21:06]

Nit: |page_size|.

[sail, 2010/12/21 18:15:33]

Done.

+  kr = host_page_size(task, &pageSize);
+  if (kr != KERN_SUCCESS) {
+    LOG(ERROR) << "Failed to fetch host page size, error: "
+               << mach_error_string(kr);
+    return false;
+  }
+
+  cpu_type_t cpuType;

[viettrungluu, 2010/12/21 04:21:06]

Ditto, etc. for other variables.

[sail, 2010/12/21 18:15:33]

Done.

+  if (!GetCPUTypeForProcess(process_, &cpuType))
+    return false;
+
+  // The same region can be reference multiple times. To avoid double counting

[viettrungluu, 2010/12/21 04:21:06]

reference*d*

[sail, 2010/12/21 18:15:33]

Done.

+  // we need to keep track of which regions we've already counted.
+  std::map<int, bool> objectMap;

[viettrungluu, 2010/12/21 04:21:06]

I wonder if you shouldn't be using base::hash_map instead. Or maybe
base::hash_set. (Or you could just be using std::set, I guess, though that's not
much lighter weight than std::map.)

Also "object map" isn't the most descriptive name. I would use some sort of set,
and name it "seen_objects" or something like that.

[sail, 2010/12/21 18:15:33]

Fixed, used base::hash_set.

+
+  mach_vm_size_t size = 0;
+  for (mach_vm_address_t address = MACH_VM_MIN_ADDRESS; ; address += size) {

[viettrungluu, 2010/12/21 04:21:06]

Nit: No space after first ';', I think.

[sail, 2010/12/21 18:15:33]

Done.

+    vm_region_top_info_data_t info;
+    mach_msg_type_number_t infoCount = VM_REGION_TOP_INFO_COUNT;
+    mach_port_t object_name;
+    kr = mach_vm_region(
+        task,

[viettrungluu, 2010/12/21 04:21:06]

I believe you can align these with the '(' on the previous line (and put |task,|
on the previous line).

[sail, 2010/12/21 18:15:33]

Done.

+        &address,
+        &size,
+        VM_REGION_TOP_INFO,
+        (vm_region_info_t)&info,
+        &infoCount,
+        &object_name);
+    if (kr == KERN_INVALID_ADDRESS) {
+      // We're at the end of the address space.
+      break;
+    } else if (kr != KERN_SUCCESS) {
+      LOG(ERROR) << "Calling mach_vm_region failed with error: "
+                 << mach_error_string(kr);
+      return false;
+    }
+
+    if (IsAddressInSharedRegion(address, cpuType) &&
+        info.share_mode != SM_PRIVATE)
+      continue;
+
+    if (info.share_mode == SM_COW && info.ref_count == 1)
+      info.share_mode = SM_PRIVATE;
+
+    switch (info.share_mode) {
+      case SM_PRIVATE:
+        if (private_bytes) {

[viettrungluu, 2010/12/21 04:21:06]

I think you may as well just calculate these (always) using a local variable,
and do the conditional assignments at the end (just to make this a bit easier to
read).

[sail, 2010/12/21 18:15:33]

Done.

+          *private_bytes += info.private_pages_resident * pageSize;
+          *private_bytes += info.shared_pages_resident * pageSize;
+        }
+        break;
+      case SM_COW:
+      case SM_SHARED:
+        if (objectMap.find(info.obj_id) == objectMap.end()) {
+          // Only count the first reference to this region.
+          objectMap[info.obj_id] = true;
+          if (shared_bytes)

[viettrungluu, 2010/12/21 04:21:06]

Ditto.

[sail, 2010/12/21 18:15:33]

Done.

+            *shared_bytes += info.shared_pages_resident * pageSize;
+        }
+        if (info.share_mode == SM_COW) {

[viettrungluu, 2010/12/21 04:21:06]

This might be nicer using a fall-through (explicitly labelling the
fall-through); I think the style guide would be okay with that, though you
should double-check.

[sail, 2010/12/21 18:15:33]

Fixed. The C++ style guide doesn't mention fall throughs so I guess it's ok.

+          if (private_bytes)
+            *private_bytes += info.private_pages_resident * pageSize;
+        }
+        break;
+      default:
+        break;
+    }
+  }
   return true;
 }