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

third_party/tcmalloc/chromium/src/memory_region_map.h

 /* Copyright (c) 2006, Google Inc.
  * All rights reserved.
  * 
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  * 
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 27 matching lines @@
 
 #ifdef HAVE_PTHREAD
 #include <pthread.h>
 #endif
 #include <stddef.h>
 #include <set>
 #include "base/stl_allocator.h"
 #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
 //  execute a bunch of mmap/munmup and compare memory map with
 //  own accounting of what those mmaps generated
 
 // Thread-safe class to collect and query the map of all memory regions
 // in a process that have been created with mmap, munmap, mremap, sbrk.
 // For each memory region, we keep track of (and provide to users)
 // the stack trace that allocated that memory region.
 // The recorded stack trace depth is bounded by
 // a user-supplied max_stack_depth parameter of Init().
 // After initialization with Init()
 // (which can happened even before global object constructor execution)
 // we collect the map by installing and monitoring MallocHook-s
 // to mmap, munmap, mremap, sbrk.
 // At any time one can query this map via provided interface.
 // For more details on the design of MemoryRegionMap
 // see the comment at the top of our .cc file.
 class MemoryRegionMap {
  private:
   // Max call stack recording depth supported by Init().  Set it to be
   // high enough for all our clients.  Note: we do not define storage
   // for this (doing that requires special handling in windows), so
   // 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 ================================================================
 
   // Every client of MemoryRegionMap must call Init() before first use,
   // and Shutdown() after last use.  This allows us to reference count
   // this (singleton) class properly.  MemoryRegionMap assumes it's the
   // only client of MallocHooks, so a client can only register other
   // MallocHooks after calling Init() and must unregister them before
   // calling Shutdown().
 
   // Initialize this module to record memory allocation stack traces.
   // Stack traces that have more than "max_stack_depth" frames
   // 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).
   // Full shutdown is attempted when the number of Shutdown() calls equals
   // 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.
   static void Lock() EXCLUSIVE_LOCK_FUNCTION(lock_);
   static void Unlock() UNLOCK_FUNCTION(lock_);
 
   // Returns true when the lock is held by this thread (for use in RAW_CHECK-s).
   static bool LockIsHeld();
 
   // Locker object that acquires the MemoryRegionMap::Lock
   // for the duration of its lifetime (a C++ scope).
   class LockHolder {
    public:
     LockHolder() { Lock(); }
     ~LockHolder() { Unlock(); }
    private:
     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.
   struct Region {
     uintptr_t start_addr;  // region start address
     uintptr_t end_addr;  // region end address
     int call_stack_depth;  // number of caller stack frames that we saved
     const void* call_stack[kMaxStackDepth];  // caller address stack array
                                              // filled to call_stack_depth size
     bool is_stack;  // does this region contain a thread's stack:
@@ skipping 76 matching lines @@
   // Returns success. Uses Lock/Unlock inside.
   static bool FindRegion(uintptr_t addr, Region* result);
 
   // Find the region that contains stack_top, mark that region as
   // a stack region, and write its data into *result if found,
   // in which case *result gets filled so that it stays fully functional
   // even when the underlying region gets removed from 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
   struct RegionCmp {
     bool operator()(const Region& x, const Region& y) const {
       return x.end_addr < y.end_addr;
     }
   };
 
   // We allocate STL objects in our own arena.
@@ skipping 61 matching lines @@
   // Recursion count for the recursive lock.
   static int recursion_count_;
   // The thread id of the thread that's inside the recursive lock.
   static pthread_t lock_owner_tid_;
 
   // Total size of all mapped pages so far
   static int64 map_size_;
   // 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:
   // returns the region covering 'addr' or NULL; assumes our lock_ is held.
   static const Region* DoFindRegionLocked(uintptr_t addr);
 
   // Verifying wrapper around regions_->insert(region)
   // To be called to do InsertRegionLocked's work only!
   inline static void DoInsertRegionLocked(const Region& region);
   // Handle regions saved by InsertRegionLocked into a tmp static array
   // 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);
 
   // Record addition of a memory region at address "start" of size "size"
   // (called from our mmap/mremap/sbrk hooks).
   static void RecordRegionAddition(const void* start, size_t size);
   // Record deletion of a memory region at address "start" of size "size"
   // (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,
                        int prot, int flags,
                        int fd, off_t offset);
   static void MunmapHook(const void* ptr, size_t size);
   static void MremapHook(const void* result, const void* old_addr,
                          size_t old_size, size_t new_size, int flags,
                          const void* new_addr);
   static void SbrkHook(const void* result, ptrdiff_t increment);
 
   // Log all memory regions; Useful for debugging only.
   // Assumes Lock() is held
   static void LogAllLocked();
 
   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_