Fix MMAP_LIST in dumps by deep-heap-profile again.

All bucket_ids in MMAP_LIST were wrongly 0.
This change fixes the issue by changing the scope and the oparation order.

It also adds a description for SnapshotMaps().

BUG=181517

Review URL: https://chromiumcodereview.appspot.com/13299002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@191619 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 85 insertions, 43 deletions

diff --git a/third_party/tcmalloc/chromium/src/deep-heap-profile.cc b/third_party/tcmalloc/chromium/src/deep-heap-profile.cc
index 9037d3c..f700c6b 100644
--- a/third_party/tcmalloc/chromium/src/deep-heap-profile.cc
+++ b/third_party/tcmalloc/chromium/src/deep-heap-profile.cc
@@ -21,7 +21,6 @@
 #include "base/cycleclock.h"
 #include "base/sysinfo.h"
 #include "internal_logging.h"  // for ASSERT, etc
-#include "memory_region_map.h"
 
 static const int kProfilerBufferSize = 1 << 20;
 static const int kHashTableSize = 179999;  // Same as heap-profile-table.cc.
@@ -602,6 +601,30 @@ void DeepHeapProfile::RegionStats::Unparse(const char* name,
   buffer->AppendString("\n", 0);
 }
 
+// Snapshots all virtual memory mappging stats by merging mmap(2) records from
+// MemoryRegionMap and /proc/maps, the OS-level memory mapping information.
+// Memory regions described in /proc/maps, but which are not created by mmap,
+// are accounted as "unhooked" memory regions.
+//
+// This function assumes that every memory region created by mmap is covered
+// by VMA(s) described in /proc/maps except for http://crbug.com/189114.
+// Note that memory regions created with mmap don't align with borders of VMAs
+// in /proc/maps.  In other words, a memory region by mmap can cut across many
+// VMAs.  Also, of course a VMA can include many memory regions by mmap.
+// It means that the following situation happens:
+//
+// => Virtual address
+// <----- VMA #1 -----><----- VMA #2 ----->...<----- VMA #3 -----><- VMA #4 ->
+// ..< mmap #1 >.<- mmap #2 -><- mmap #3 ->...<- mmap #4 ->..<-- mmap #5 -->..
+//
+// It can happen easily as permission can be changed by mprotect(2) for a part
+// of a memory region.  A change in permission splits VMA(s).
+//
+// To deal with the situation, this function iterates over MemoryRegionMap and
+// /proc/maps independently.  The iterator for MemoryRegionMap is initialized
+// at the top outside the loop for /proc/maps, and it goes forward inside the
+// loop while comparing their addresses.
+//
 // TODO(dmikurube): Eliminate dynamic memory allocation caused by snprintf.
 void DeepHeapProfile::GlobalStats::SnapshotMaps(
     const MemoryResidenceInfoGetterInterface* memory_residence_info_getter,
@@ -610,7 +633,7 @@ void DeepHeapProfile::GlobalStats::SnapshotMaps(
   MemoryRegionMap::LockHolder lock_holder;
   ProcMapsIterator::Buffer procmaps_iter_buffer;
   ProcMapsIterator procmaps_iter(0, &procmaps_iter_buffer);
-  uint64 first_address, last_address, offset;
+  uint64 vma_start_addr, vma_last_addr, offset;
   int64 inode;
   char* flags;
   char* filename;
@@ -623,19 +646,21 @@ void DeepHeapProfile::GlobalStats::SnapshotMaps(
 
   MemoryRegionMap::RegionIterator mmap_iter =
       MemoryRegionMap::BeginRegionLocked();
+  DeepBucket* deep_bucket = GetInformationOfMemoryRegion(
+      mmap_iter, memory_residence_info_getter, deep_profile);
 
-  while (procmaps_iter.Next(&first_address, &last_address,
+  while (procmaps_iter.Next(&vma_start_addr, &vma_last_addr,
                             &flags, &offset, &inode, &filename)) {
     if (mmap_dump_buffer) {
       char buffer[1024];
       int written = procmaps_iter.FormatLine(buffer, sizeof(buffer),
-                                             first_address, last_address, flags,
-                                             offset, inode, filename, 0);
+                                             vma_start_addr, vma_last_addr,
+                                             flags, offset, inode, filename, 0);
       mmap_dump_buffer->AppendString(buffer, 0);
     }
 
-    // 'last_address' should be the last inclusive address of the region.
-    last_address -= 1;
+    // 'vma_last_addr' should be the last inclusive address of the region.
+    vma_last_addr -= 1;
     if (strcmp("[vsyscall]", filename) == 0) {
       continue;  // Reading pagemap will fail in [vsyscall].
     }
@@ -654,59 +679,40 @@ void DeepHeapProfile::GlobalStats::SnapshotMaps(
       type = OTHER;
     }
     all_[type].Record(
-        memory_residence_info_getter, first_address, last_address);
+        memory_residence_info_getter, vma_start_addr, vma_last_addr);
 
     // TODO(dmikurube): Stop double-counting pagemap.
-    // Counts unhooked memory regions in /proc/<pid>/maps.
     if (MemoryRegionMap::IsRecordingLocked()) {
-      // It assumes that every mmap'ed region is included in one maps line.
-      uint64 cursor = first_address;
+      uint64 cursor = vma_start_addr;
       bool first = true;
 
+      // Iterates over MemoryRegionMap until the iterator moves out of the VMA.
       do {
-        Bucket* bucket = NULL;
-        DeepBucket* deep_bucket = NULL;
         if (!first) {
-          size_t committed = deep_profile->memory_residence_info_getter_->
-              CommittedSize(mmap_iter->start_addr, mmap_iter->end_addr - 1);
-          // TODO(dmikurube): Store a reference to the bucket in region.
-          Bucket* bucket = MemoryRegionMap::GetBucket(
-              mmap_iter->call_stack_depth, mmap_iter->call_stack);
-          DeepBucket* deep_bucket = NULL;
-          if (bucket != NULL) {
-            deep_bucket = deep_profile->deep_table_.Lookup(
-                bucket,
-#if defined(TYPE_PROFILING)
-                NULL,  // No type information for mmap'ed memory regions.
-#endif
-                /* is_mmap */ true);
-          }
-
-          if (deep_bucket != NULL)
-            deep_bucket->committed_size += committed;
-          profiled_mmap_.AddToVirtualBytes(
-              mmap_iter->end_addr - mmap_iter->start_addr);
-          profiled_mmap_.AddToCommittedBytes(committed);
-
           cursor = mmap_iter->end_addr;
           ++mmap_iter;
           // Don't break here even if mmap_iter == EndRegionLocked().
+
+          if (mmap_iter != MemoryRegionMap::EndRegionLocked()) {
+            deep_bucket = GetInformationOfMemoryRegion(
+                mmap_iter, memory_residence_info_getter, deep_profile);
+          }
         }
         first = false;
 
         uint64 last_address_of_unhooked;
-        // If the next mmap entry is away from the current maps line.
+        // If the next mmap entry is away from the current VMA.
         if (mmap_iter == MemoryRegionMap::EndRegionLocked() ||
-            mmap_iter->start_addr > last_address) {
-          last_address_of_unhooked = last_address;
+            mmap_iter->start_addr > vma_last_addr) {
+          last_address_of_unhooked = vma_last_addr;
         } else {
           last_address_of_unhooked = mmap_iter->start_addr - 1;
         }
 
         if (last_address_of_unhooked + 1 > cursor) {
-          RAW_CHECK(cursor >= first_address,
+          RAW_CHECK(cursor >= vma_start_addr,
                     "Wrong calculation for unhooked");
-          RAW_CHECK(last_address_of_unhooked <= last_address,
+          RAW_CHECK(last_address_of_unhooked <= vma_last_addr,
                     "Wrong calculation for unhooked");
           uint64 committed_size = unhooked_[type].Record(
               memory_residence_info_getter,
@@ -727,10 +733,10 @@ void DeepHeapProfile::GlobalStats::SnapshotMaps(
         }
 
         if (mmap_iter != MemoryRegionMap::EndRegionLocked() &&
-            mmap_iter->start_addr <= last_address &&
+            mmap_iter->start_addr <= vma_last_addr &&
             mmap_dump_buffer) {
-          bool trailing = mmap_iter->start_addr < first_address;
-          bool continued = mmap_iter->end_addr - 1 > last_address;
+          bool trailing = mmap_iter->start_addr < vma_start_addr;
+          bool continued = mmap_iter->end_addr - 1 > vma_last_addr;
           mmap_dump_buffer->AppendString(trailing ? " (" : "  ", 0);
           mmap_dump_buffer->AppendPtr(mmap_iter->start_addr, 0);
           mmap_dump_buffer->AppendString(trailing ? ")" : " ", 0);
@@ -749,7 +755,7 @@ void DeepHeapProfile::GlobalStats::SnapshotMaps(
           mmap_dump_buffer->AppendString("\n", 0);
         }
       } while (mmap_iter != MemoryRegionMap::EndRegionLocked() &&
-               mmap_iter->end_addr - 1 <= last_address);
+               mmap_iter->end_addr - 1 <= vma_last_addr);
     }
   }
 
@@ -857,6 +863,36 @@ void DeepHeapProfile::GlobalStats::RecordAlloc(const void* pointer,
   deep_profile->stats_.profiled_malloc_.AddToCommittedBytes(committed);
 }
 
+DeepHeapProfile::DeepBucket*
+    DeepHeapProfile::GlobalStats::GetInformationOfMemoryRegion(
+        const MemoryRegionMap::RegionIterator& mmap_iter,
+        const MemoryResidenceInfoGetterInterface* memory_residence_info_getter,
+        DeepHeapProfile* deep_profile) {
+  size_t committed = deep_profile->memory_residence_info_getter_->
+      CommittedSize(mmap_iter->start_addr, mmap_iter->end_addr - 1);
+
+  // TODO(dmikurube): Store a reference to the bucket in region.
+  Bucket* bucket = MemoryRegionMap::GetBucket(
+      mmap_iter->call_stack_depth, mmap_iter->call_stack);
+  DeepBucket* deep_bucket = NULL;
+  if (bucket != NULL) {
+    deep_bucket = deep_profile->deep_table_.Lookup(
+        bucket,
+#if defined(TYPE_PROFILING)
+        NULL,  // No type information for memory regions by mmap.
+#endif
+        /* is_mmap */ true);
+    if (deep_bucket != NULL)
+      deep_bucket->committed_size += committed;
+  }
+
+  profiled_mmap_.AddToVirtualBytes(
+      mmap_iter->end_addr - mmap_iter->start_addr);
+  profiled_mmap_.AddToCommittedBytes(committed);
+
+  return deep_bucket;
+}
+
 // static
 void DeepHeapProfile::WriteProcMaps(const char* prefix,
                                     int buffer_size,
diff --git a/third_party/tcmalloc/chromium/src/deep-heap-profile.h b/third_party/tcmalloc/chromium/src/deep-heap-profile.h
index 4294184..c2f65ea 100644
--- a/third_party/tcmalloc/chromium/src/deep-heap-profile.h
+++ b/third_party/tcmalloc/chromium/src/deep-heap-profile.h
@@ -42,6 +42,7 @@
 
 #include "addressmap-inl.h"
 #include "heap-profile-table.h"
+#include "memory_region_map.h"
 
 class DeepHeapProfile {
  public:
@@ -270,6 +271,11 @@ class DeepHeapProfile {
                             AllocValue* alloc_value,
                             DeepHeapProfile* deep_profile);
 
+    DeepBucket* GetInformationOfMemoryRegion(
+        const MemoryRegionMap::RegionIterator& mmap_iter,
+        const MemoryResidenceInfoGetterInterface* memory_residence_info_getter,
+        DeepHeapProfile* deep_profile);
+
     // All RegionStats members in this class contain the bytes of virtual
     // memory and committed memory.
     // TODO(dmikurube): These regions should be classified more precisely later