Count m(un)map for each stacktrace in MemoryRegionMap instead of HeapProfileTable.

third_party/tcmalloc/chromium/src/memory_region_map.h

Index: third_party/tcmalloc/chromium/src/memory_region_map.h
diff --git a/third_party/tcmalloc/chromium/src/memory_region_map.h b/third_party/tcmalloc/chromium/src/memory_region_map.h
index 988ea707375aed124136fe0bb7db105dbb2952a7..b26071b9103ae76d96f58ed716b7a6e08fa8fc87 100644
--- a/third_party/tcmalloc/chromium/src/memory_region_map.h
+++ b/third_party/tcmalloc/chromium/src/memory_region_map.h
@@ -45,6 +45,7 @@
 #include "base/spinlock.h"
 #include "base/thread_annotations.h"
 #include "base/low_level_alloc.h"
+#include "heap-profile-stats.h"
 
 // TODO(maxim): add a unittest:
 //  execute a bunch of mmaps and compare memory map what strace logs
@@ -72,6 +73,8 @@ class MemoryRegionMap {
   // don't take the address of it!
   static const int kMaxStackDepth = 32;
 
+  static const int kHashTableSize = 179999;

[jar (doing other things), 2013/03/08 04:43:31]

nit: please justify this magic number in a comment.

Assuming the template does not need to be in this file, this constant shouldn't
be here either.

I don't believe static member initialization in a class definition is (was?)
legal (and accepted by all our compilers).

Do all our current compilers accept this?

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Added a comment on this constant, and a detailed description in
heap-profile-stats.h.

The template should be in this header file like addressmap-inl.h does.  If it's
defined in memory_region_map.cc, a linker error happens since the template is
not instantiated.

For static const member initialization, see my reply at
https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro....

+
  public:
   // interface ================================================================
 
@@ -87,11 +90,12 @@ class MemoryRegionMap {
   // are automatically shrunk to "max_stack_depth" when they are recorded.
   // Init() can be called more than once w/o harm, largest max_stack_depth
   // will be the effective one.
+  // It counts mmap and munmap sizes per stack trace if "use_buckets".

[jar (doing other things), 2013/03/08 04:43:31]

net: Contrary to the comment, I doubt that "counting" is done during this call,
but rather we possibly enable the counting to be done.  In addition, it doesn't
discuss the multiple call case, which was described (for the other arguments)
just above this new comment.  A plausible relpacement comment would be:


When "use_buckets" is true, then counts of mmap and munmap sizes will be
recorded with each stack trace.  If Init() is called more than once, then
counting will be effective after any call contained "use_buckets" of true.


  

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Done.

   // It will install mmap, munmap, mremap, sbrk hooks
   // and initialize arena_ and our hook and locks, hence one can use
   // MemoryRegionMap::Lock()/Unlock() to manage the locks.
   // Uses Lock/Unlock inside.
-  static void Init(int max_stack_depth);
+  static void Init(int max_stack_depth, bool use_buckets);
 
   // Try to shutdown this module undoing what Init() did.
   // Returns true iff could do full shutdown (or it was not attempted).
@@ -99,6 +103,9 @@ class MemoryRegionMap {
   // the number of Init() calls.
   static bool Shutdown();
 
+  // Return true if MemoryRegionMap is initialized and working.

[jar (doing other things), 2013/03/08 04:43:31]

I understand what initialized means, but I'm unclear on "working."  Is there a
way this could "break" and stop working?  ...or is there a way it can be
disabled?

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Changed the comment to:

Return true if MemoryRegionMap is initialized and recording, i.e. when
then number of Init() calls are more than the number of Shutdown() calls.

+  static bool IsWorking();
+
   // Locks to protect our internal data structures.
   // These also protect use of arena_ if our Init() has been done.
   // The lock is recursive.
@@ -118,6 +125,10 @@ class MemoryRegionMap {
     DISALLOW_COPY_AND_ASSIGN(LockHolder);
   };
 
+  // Profile stats.
+  typedef HeapProfileStats Stats;
+  typedef HeapProfileBucket Bucket;

[jar (doing other things), 2013/03/08 04:43:31]

What is the advantage of defining smaller typedef names? I expect a type like
"Stats" to only be confusing.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Removed them.

+
   // A memory region that we know about through malloc_hook-s.
   // This is essentially an interface through which MemoryRegionMap
   // exports the collected data to its clients.  Thread-compatible.
@@ -214,6 +225,15 @@ class MemoryRegionMap {
   // Returns success. Uses Lock/Unlock inside.
   static bool FindAndMarkStackRegion(uintptr_t stack_top, Region* result);
 
+  // Iterate over the buckets which store mmap and munmap counts per stack
+  // trace.  It calls "callback" for each bucket, and passes "arg" to it.
+  template<class Type>
+  static void IterateBuckets(void (*callback)(const Bucket*, Type), Type arg);
+
+  // Get the bucket for the caller stack trace "key" of depth "depth"
+  // creating the bucket if needed.

[jar (doing other things), 2013/03/08 04:43:31]

I couldn't understand what this comment meant.
I have no clue about |depth|, but might guess it is stack depth, but not sure.
I have no idea what |key| is.
I have no idea why we pass an array of const pointers to const  void values.
I don't know how to tell how large the array is.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Changed the comment.  The details of the bucket table is described in
heap-profile-stats.h.

btw, the bucket table is the "core" data structure of the heap profiler in
tcmalloc (gperftools).  We can't understand the heap profiler without
understanding the bucket table.

+  static Bucket* GetBucket(int depth, const void* const key[]);
+
  private:  // our internal types ==============================================
 
   // Region comparator for sorting with STL
@@ -295,6 +315,21 @@ class MemoryRegionMap {
   // Total size of all unmapped pages so far
   static int64 unmap_size_;
 
+  // Bucket hash table.
+  static Bucket** bucket_table_;

[jar (doing other things), 2013/03/08 04:43:31]

Can you clarify in the comment what this data structure is? The comment says
"hash table," but it appears to be a rather free-form data structure (pointer to
pointer to buckets) with no explanation of how the table is used, indexed,
traversed, etc.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Done.

+  static int num_buckets_;
+
+  // Number of unprocessed bucket inserts.

[jar (doing other things), 2013/03/08 04:43:31]

It is not clear what "unprocessed buckets" are, presumably as opposed to
"processed."

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Done.

+  static int saved_buckets_count_;
+
+  // Unprocessed inserts (must be big enough to hold all mmaps that can be
+  // caused by a GetBucket call).
+  // Bucket has no constructor, so that c-tor execution does not interfere
+  // with the any-time use of the static memory behind saved_buckets.
+  static Bucket saved_buckets_[20];

[jar (doing other things), 2013/03/08 04:43:31]

Please justify the magic number.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Done.

+
+  static const void* saved_buckets_keys_[20][kMaxStackDepth];
+
   // helpers ==================================================================
 
   // Helper for FindRegion and FindAndMarkStackRegion:
@@ -308,6 +343,10 @@ class MemoryRegionMap {
   // by calling insert_func on them.
   inline static void HandleSavedRegionsLocked(
                        void (*insert_func)(const Region& region));

[jar (doing other things), 2013/03/08 04:43:31]

nit: indent should 4 characters.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

We should not change the existing code that makes unnecessary difference from
the original.  Such difference makes us unhappy to roll this library.

We should follow the Chromium Coding Style guide which says
"Try to follow any per-file conventions you notice when modifying existing
code."
http://dev.chromium.org/developers/coding-style

Reasonable rationale (our public guideline, not personal liking) is required to
break the existing conventions.

+
+  // Handle buckets saved by GetBucket into a tmp static array.
+  inline static void HandleSavedBucketsLocked();

[jar (doing other things), 2013/03/08 04:43:31]

The word "Handle" is a poor choice for a verb.
Better might be:
ProcessSavedBacketsLocked().

Unlike the call on line 344, where the "handling" is defined to be "call the
function," you need to indicate what the processing is.

Can you justify why you're using the inline directive? (other than copying
questionable patterns on the previous lines?)

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Sounds reasonable.  Finally renamed it to Restore-() and added comments.

Removed inline.

+
   // Wrapper around DoInsertRegionLocked
   // that handles the case of recursive allocator calls.
   inline static void InsertRegionLocked(const Region& region);
@@ -319,6 +358,11 @@ class MemoryRegionMap {
   // (called from our munmap/mremap/sbrk hooks).
   static void RecordRegionRemoval(const void* start, size_t size);
 
+  // Record deletion of a memory region in a bucket.

[jar (doing other things), 2013/03/08 04:43:31]

You need to explain what the arguments mean.  

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Done.

+  static void RecordRegionRemovalInBucket(int depth,
+                                          const void* const key[],
+                                          size_t size);
+
   // Hooks for MallocHook
   static void MmapHook(const void* result,
                        const void* start, size_t size,
@@ -337,4 +381,14 @@ class MemoryRegionMap {
   DISALLOW_COPY_AND_ASSIGN(MemoryRegionMap);
 };
 
+template <class Type>
+void MemoryRegionMap::IterateBuckets(

[jar (doing other things), 2013/03/08 04:43:31]

nit: this is a line continuation, and should be indented 4, or probably more
reasonably, put on the tail of the previous line.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

This newline style (template in a single line, and no indent in the second line)
is a very common convention *for templates* in Chromium.

Examples or a clear guideline in Chromium are required to change it.  I can show
many examples of this style like:
http://src.chromium.org/viewvc/chrome/trunk/src/base/template_util.h?view=markup
http://src.chromium.org/viewvc/chrome/trunk/src/sync/notifier/invalidator_tes...
http://src.chromium.org/viewvc/chrome/trunk/src/base/test/sequenced_task_runn...
http://src.chromium.org/viewvc/chrome/trunk/src/cc/layer_iterator.h?view=markup
http://src.chromium.org/viewvc/chrome/trunk/src/base/bind.h?view=markup

+    void (*callback)(const Bucket*, Type), Type arg) {

[jar (doing other things), 2013/03/08 04:43:31]

nit: provide meaningful variable names for all arguments, including the
prototypes, such in the argument list for this callback.  |arg| is not a good
variable name.  In this case, perhaps |instance| would be better.


...also... are you copying this pattern from somewhere else?  Although something
closs to this pattern is perhaps more common in C (without a template), in C++
it is much more common to see member functions called, and |this| passed
implicitly (I guess here you're passing the full blown instance, rather than a
pointer or a ref though :-/  ).  Is there a reason for this pattern (using a
callback function)?  ... passing an instance of a type?

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

This |arg| is just an argument passed to |callback|.  It may not be a kind of
instances.  Renamed it to |callback_arg|.
Yes, I know it's not so common in C++, and it's the same approach that
tcmalloc's addressmap-inl.h does.

+  for (int b = 0; b < kHashTableSize; b++) {

[jar (doing other things), 2013/03/08 04:43:31]

nit: Why do you want the source code in the header?

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Written at:
https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro...

+    for (Bucket* x = bucket_table_[b]; x != NULL; x = x->next) {

[jar (doing other things), 2013/03/08 04:43:31]

nit: avoid use of single letter varible names such as |x| an |b|.

[Dai Mikurube (NOT FULLTIME), 2013/03/12 08:24:10]

Done.

fyi in general, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
has many "for (int i = 0; i < size; i++)".

+      callback(x, arg);
+    }
+  }
+}
+
 #endif  // BASE_MEMORY_REGION_MAP_H_