Adding extended report to memdump with resident pages bit reporting.

tools/android/memdump/memdump.cc

Index: tools/android/memdump/memdump.cc
diff --git a/tools/android/memdump/memdump.cc b/tools/android/memdump/memdump.cc
index 21ef291f3d76d8f3b47814ade2e758f2725a3e95..8d1cdadde3e21e7370d45c94a08ebe6c4d3a7dbf 100644
--- a/tools/android/memdump/memdump.cc
+++ b/tools/android/memdump/memdump.cc
@@ -16,6 +16,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/base64.h"
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -30,6 +31,29 @@
 
 namespace {
 
+class BitSet {

[bulach, 2013/07/22 12:59:39]

I suppose the ndk has #include <bitset> ?

[digit1, 2013/07/22 21:03:14]

That's true, but note that std::bitset<> is not a dynamically-sized container,
which BitSet here seems to be.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Yeah, unforunately STL's bitset require the size of the bitmap to be known at
compile time (i.e. length it's an argument of the template)

[bulach, 2013/07/23 08:10:57]

ops, forgot that tiny detail :) thanks for the refresher!

+public:

[Philippe, 2013/07/22 09:20:55]

Nit: there should be a leading space before 'public'.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Done.

+  BitSet() : nbits_(0), data_(0) { }

[Philippe, 2013/07/22 09:20:55]

Nit: no space between the braces.

[Philippe, 2013/07/22 09:20:55]

Nit: I believe the |data_| explicit initialization should not be needed. See my
comment line 39 but we might be able to completely remove the constructor if you
get rid of |nbits|.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Done.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Hmm, agree for nbits (see below), but _data was just for paranoy. I'm not sure
that calling the default vector ctor won't pre-reserve an "optimistic" (impl.
dependent) chunk of memory. Most of the mappings have very few pages.

+
+  void setSize(int nbits) {

[Philippe, 2013/07/22 09:20:55]

Nit: maybe call this 'resize()' and also s/getData/data, s/getLength/size to
make this interface closer to std::vector.

[digit1, 2013/07/22 21:03:14]

Also, you should use the Chromium style guide, which is that method names are
CamelCased, except for simple accessors like member() and set_member().

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Right, makes sense!

+    nbits_ = nbits;

[Philippe, 2013/07/22 09:20:55]

Nit: do we need to store |nbits|? Can we entirely rely on std::vector?

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Yeah, that was just me being uber-paranoid. The at() accessor should be enough.
Ah, I just realized that the entire sense of nbits_ was a CHECK on set(..),
which I later removed. So that does not make any sense at all!

+    data_.resize((nbits_+7)/8);

[Philippe, 2013/07/22 09:20:55]

Nit: spaces around binary operators (here and on a few other lines below).

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Done.

+    memset(&data_[0], 0, getLength());

[Philippe, 2013/07/22 09:20:55]

Nit: I believe you can delete this line and use the resize(size_type,
value_type) overload of std::vector::resize() unless I'm missing something :)

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Oh, apparently I am the one missing the autofill on resize() :)

+  }
+
+  void set(int bit) {
+    data_.at(bit/8) |= (1 << (bit % 8));

[digit1, 2013/07/22 21:03:14]

nit: since 'bit' is signed, using 'bit & 7' is more efficient than 'bit % 8'.
The latter will generate a little more instructions to deal with negative 'bit'
values, which isn't needed here.

Also, this code is built without exceptions, so it's probably better to use []
and an explicit bound check rather than relying on at() for this.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Great catch! Right.
For the records:

  return argc % 8;
   0x00008428 <+0>:     ldr     r3, [pc, #16]   ; (0x843c <main(int,
char**)+20>)
   0x0000842a <+2>:     ands    r3, r0
   0x0000842c <+4>:     cmp     r3, #0
   0x0000842e <+6>:     bge.n   0x8438 <main(int, char**)+16>
   0x00008430 <+8>:     subs    r3, #1
   0x00008432 <+10>:    orn     r3, r3, #7
   0x00008436 <+14>:    adds    r3, #1
   0x00008438 <+16>:    mov     r0, r3
   0x0000843a <+18>:    bx      lr
   0x0000843c <+20>:    andhi   r0, r0, r7
   
  return argc & 7;
   0x00008428 <+0>:     and.w   r0, r0, #7
   0x0000842c <+4>:     bx      lr
Ah right I forgot that exception are not enabled! Good point, I'll revert to the
original CHECK.

+  }
+
+  const char* getData() const { return &data_[0]; }

[Philippe, 2013/07/22 09:20:55]

Nit: data_.data()?

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

I'm afraid that is only possible in C++11.

+
+  int getLength() const { return data_.size(); }

[digit1, 2013/07/22 21:03:14]

nit: This is very misleading because this doesn't return the length in bits, but
the length of bytes needed for the std::string representation. I'd suggest
removing getData() / getLength() completely and instead provide a single
function ToString() or AsString() that returns a std::string directly.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

At this point, let's do directly  AsB64String().

+
+private:
+  int nbits_;
+  std::vector<char> data_;
+};
+
 // An entry in /proc/<pid>/pagemap.
 struct PageMapEntry {
   uint64 page_frame_number : 55;
@@ -49,6 +73,7 @@ struct MemoryMap {
   std::string flags;
   uint start_address;
   uint end_address;
+  uint offset;
   int private_count;
   int unevictable_private_count;
   int other_shared_count;
@@ -57,6 +82,9 @@ struct MemoryMap {
   // (only among the processes that are being analyzed).
   std::vector<int> app_shared_counts;
   std::vector<PageInfo> committed_pages;
+  // committed_pages_bits is a bitmap reflecting the present bit for all the

[Philippe, 2013/07/22 09:20:55]

Nit: s/bitmap/bitset maybe.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Done.

+  // virtual pages of the mapping.
+  BitSet committed_pages_bits;
 };
 
 struct ProcessMemory {
@@ -114,6 +142,11 @@ bool ParseMemoryMapLine(const std::string& line,
   if (tokens->at(1).size() != strlen("rwxp"))
     return false;
   memory_map->flags.swap(tokens->at(1));
+  if (!base::HexStringToUInt64(tokens->at(2), &tmp))
+    return false;
+  memory_map->offset = static_cast<uint>(tmp);
+  memory_map->committed_pages_bits.setSize(
+      (memory_map->end_address - memory_map->start_address) / PAGE_SIZE);
   const int map_name_index = 5;
   if (tokens->size() >= map_name_index + 1) {
     for (std::vector<std::string>::const_iterator it =
@@ -167,9 +200,11 @@ bool GetProcessMaps(pid_t pid, std::vector<MemoryMap>* process_maps) {
 // provided memory map.
 bool GetPagesForMemoryMap(int pagemap_fd,
                           const MemoryMap& memory_map,
-                          std::vector<PageInfo>* committed_pages) {
-  for (uint addr = memory_map.start_address; addr < memory_map.end_address;
-       addr += PAGE_SIZE) {
+                          std::vector<PageInfo>* committed_pages,
+                          BitSet* committed_pages_bits) {
+  for (uint addr = memory_map.start_address, page_index = 0;
+       addr < memory_map.end_address;
+       addr += PAGE_SIZE, ++page_index) {
     DCHECK_EQ(0, addr % PAGE_SIZE);
     PageMapEntry page_map_entry = {};
     COMPILE_ASSERT(sizeof(PageMapEntry) == sizeof(uint64), unexpected_size);
@@ -182,6 +217,7 @@ bool GetPagesForMemoryMap(int pagemap_fd,
       PageInfo page_info = {};
       page_info.page_frame_number = page_map_entry.page_frame_number;
       committed_pages->push_back(page_info);
+      committed_pages_bits->set(page_index);
     }
   }
   return true;
@@ -387,6 +423,44 @@ void DumpProcessesMemoryMapsInShortFormat(
   }
 }
 
+void DumpProcessesMemoryMapsInExtendedFormat(
+    const std::vector<ProcessMemory>& processes_memory) {
+  std::string buf;
+  std::string app_shared_buf;
+  for (std::vector<ProcessMemory>::const_iterator it = processes_memory.begin();
+       it != processes_memory.end(); ++it) {
+    const ProcessMemory& process_memory = *it;
+    std::cout << "[ PID=" << process_memory.pid << "]" << '\n';
+    const std::vector<MemoryMap>& memory_maps = process_memory.memory_maps;
+    for (std::vector<MemoryMap>::const_iterator it = memory_maps.begin();
+         it != memory_maps.end(); ++it) {
+      const MemoryMap& memory_map = *it;
+      app_shared_buf.clear();
+      AppendAppSharedField(memory_map.app_shared_counts, &app_shared_buf);
+      std::string pages_bits(memory_map.committed_pages_bits.getData(),
+                             memory_map.committed_pages_bits.getLength());
+      std::string encoded_page_bits;
+      base::Base64Encode(pages_bits, &encoded_page_bits);
+      base::SStringPrintf(
+          &buf,
+          "%x-%x %s %x private_unevictable=%d private=%d shared_app=%s "
+          "shared_other_unevictable=%d shared_other=%d \"%s\" [%s]\n",
+          memory_map.start_address,
+          memory_map.end_address,
+          memory_map.flags.c_str(),
+          memory_map.offset,
+          memory_map.unevictable_private_count * PAGE_SIZE,
+          memory_map.private_count * PAGE_SIZE,
+          app_shared_buf.c_str(),
+          memory_map.unevictable_other_shared_count * PAGE_SIZE,
+          memory_map.other_shared_count * PAGE_SIZE,
+          memory_map.name.c_str(),
+          encoded_page_bits.c_str());
+      std::cout << buf;
+    }
+  }
+}
+
 bool CollectProcessMemoryInformation(int page_count_fd,
                                      int page_flags_fd,
                                      ProcessMemory* process_memory) {
@@ -404,7 +478,8 @@ bool CollectProcessMemoryInformation(int page_count_fd,
   for (std::vector<MemoryMap>::iterator it = process_maps->begin();
        it != process_maps->end(); ++it) {
     std::vector<PageInfo>* const committed_pages = &it->committed_pages;
-    GetPagesForMemoryMap(pagemap_fd, *it, committed_pages);
+    BitSet* pages_bits = &it->committed_pages_bits;

[Philippe, 2013/07/22 09:20:55]

Nit: 'BitSet* const pages_bits' maybe for consistency with the line above.

[Primiano Tucci (use gerrit), 2013/07/22 23:22:35]

Done.

+    GetPagesForMemoryMap(pagemap_fd, *it, committed_pages, pages_bits);
     SetPagesInfo(page_count_fd, page_flags_fd, committed_pages);
   }
   return true;
@@ -420,17 +495,17 @@ void KillAll(const std::vector<pid_t>& pids, int signal_number) {
 }  // namespace
 
 int main(int argc, char** argv) {
-  bool short_output = false;
   if (argc == 1) {
-    LOG(ERROR) << "Usage: " << argv[0] << " [-a] <PID1>... <PIDN>";
+    LOG(ERROR) << "Usage: " << argv[0] << " [-a|-x] <PID1>... <PIDN>";
     return EXIT_FAILURE;
   }
-  if (!strncmp(argv[1], "-a", 2)) {
+  const bool short_output = !strncmp(argv[1], "-a", 2);
+  const bool extended_output = !strncmp(argv[1], "-x", 2);
+  if (short_output || extended_output) {
     if (argc == 2) {
-      LOG(ERROR) << "Usage: " << argv[0] << " [-a] <PID1>... <PIDN>";
+      LOG(ERROR) << "Usage: " << argv[0] << " [-a|-x] <PID1>... <PIDN>";
       return EXIT_FAILURE;
     }
-    short_output = true;
     ++argv;
   }
   std::vector<pid_t> pids;
@@ -476,6 +551,8 @@ int main(int argc, char** argv) {
   ClassifyPages(&processes_memory);
   if (short_output)
     DumpProcessesMemoryMapsInShortFormat(processes_memory);
+  else if (extended_output)
+    DumpProcessesMemoryMapsInExtendedFormat(processes_memory);
   else
     DumpProcessesMemoryMaps(processes_memory);
   return EXIT_SUCCESS;