A deeper heap profile dumper in third_party/tcmalloc/chromium.

third_party/tcmalloc/chromium/src/deep-heap-profile.h

Index: third_party/tcmalloc/chromium/src/deep-heap-profile.h
diff --git a/third_party/tcmalloc/chromium/src/deep-heap-profile.h b/third_party/tcmalloc/chromium/src/deep-heap-profile.h
new file mode 100644
index 0000000000000000000000000000000000000000..680e59b4ccc04c7b7edeb42c5e5310e767269150
--- /dev/null
+++ b/third_party/tcmalloc/chromium/src/deep-heap-profile.h
@@ -0,0 +1,190 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// ---
+// Author: Sainbayar Sukhbaatar
+//         Dai Mikurube
+//
+// This file contains a class DeepHeapProfile and its public function
+// DeepHeapProfile::FillOrderedProfile() which works as an alternative of

[jar (doing other things), 2012/01/13 20:37:23]

I found the name "Deep" HeapProfile pretty obscure.  This intro gives me the
answer (I think), in that we include memory-residence in the report.  What other
OS-level information is there?

If that is the only piece of data, perhaps you could provide a name that is
better than "Deep."   Perhaps it should be a HeapResidencyProfile.  I don't
know... but I'm sure there can be a better name.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

I thought about that, but actually, it is planned to make a tool set called Deep
Memory(Heap) Profiler for Chromium, and some more information will be dumped
additionally from this extra dumper for the profiler.  (This change is the first
step.)

I guess they are not good
1) to add many files in this third_party/tcmalloc/chromium directory, and
2) to execute dump-related operations in distributed classes and files for
performance.

In order to encapsulate the extra dumper code in a single file, and to avoid a
misleading file name and frequent renaming in future, I chose to use the name
deep-heap-profile and DeepHeapProfile for the file and class.

If naming for the current dumper code, HeapResidencyProfile sounds good.

+// HeapProfileTable::FillOrderedProfile().
+//
+// DeepHeapProfile::FillOrderedProfile() dumps more detailed information about
+// heap usage, which includes OS-level information such as whether the memory
+// block is actually in memory, or not.  DeepHeapProfile::FillOrderedProfile()
+// uses logged data in HeapProfileTable as one of its data sources.
+// DeepHeapProfile works only when its FillOrderedProfile() is called.  It has
+// overhead when dumping, but no overhead when logging.
+//
+// It currently works only on Linux.  It just delegates to HeapProfileTable in
+// non-Linux environments.
+
+
+#ifndef BASE_DEEP_HEAP_PROFILE_H_
+#define BASE_DEEP_HEAP_PROFILE_H_
+
+#include "config.h"
+
+#if defined(__linux__)
+#define DEEP_HEAP_PROFILE 1
+#endif
+
+#include "addressmap-inl.h"
+#include "heap-profile-table.h"
+
+class DeepHeapProfile {
+ public:
+  typedef HeapProfileTable::Bucket Bucket;
+  typedef HeapProfileTable::AllocationMap AllocationMap;
+  typedef HeapProfileTable::AllocValue AllocValue;
+  typedef HeapProfileTable::Stats Stats;
+
+  // Construct a DeepHeapProfile instance.  It works as a wrapper of
+  // HeapProfileTable.
+  //
+  // 'heap_profile' is a pointer to HeapProfileTable.  DeepHeapProfile reads
+  // data in 'heap_profile' and forwards operations to 'heap_profile' if
+  // DeepHeapProfile is not available (non-Linux).
+  // 'prefix' is a prefix of dumped file names.
+  DeepHeapProfile(HeapProfileTable* heap_profile, const char* prefix);
+  ~DeepHeapProfile();
+
+  // Fill deep profile data into buffer 'buffer' of size 'size', and return the
+  // actual size occupied by the dump in 'buffer'.  It works as an alternative
+  // of HeapProfileTable::FillOrderedProfile.
+  //
+  // The profile buckets are dumped in the decreasing order of currently
+  // allocated bytes.  We do not provision for 0-terminating 'buffer'.
+  int FillOrderedProfile(char buffer[], int buffer_size);
+
+ private:
+#ifdef DEEP_HEAP_PROFILE
+  struct DeepBucket {
+    Bucket* bucket;
+    size_t committed_size;
+    int   id;         // Unique ID of the bucket.
+    bool  is_logged;  // True if the stracktrace is logged to a file.
+  };
+
+  typedef AddressMap<DeepBucket> DeepBucketMap;
+
+  struct PageState {

[jar (doing other things), 2012/01/13 20:37:23]

I'm hopeful that this struct is only used briefly in interfaces, and it is not a
memory use issues (and should be made into a bit field).

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Sorry, I don't understand what you mean...  You mean this structure is not
required?  

[jar (doing other things), 2012/01/26 20:19:02]

I was surprised to see a struct with 5 bools, rather than one bitfield with 5
one-bit fields used.  I then wondered whether this was used often (with a memory
footprint that we care about), or trivially (one or two instances? only for
argument passing of short lived instances??).

On 2012/01/19 11:54:14, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/01/30 12:54:53]

Got it, thanks.  This is used only by ReadProcPagemap, and we have only one
PageState instance simultaneously, which is used repeatedly.  It's not
memory-consuming.  I'll change it if we use many PageState instances
simutaneously.

On 2012/01/26 20:19:02, jar wrote:

+    bool is_committed;  // Currently, we use only this
+    bool is_present;
+    bool is_swapped;
+    bool is_shared;
+    bool is_mmap;
+  };
+
+  struct RegionStats {
+    size_t virtual_bytes;
+    size_t committed_bytes;
+  };
+
+  struct GlobalStats {
+    RegionStats total;
+    RegionStats file_mapped;
+    RegionStats anonymous;

[jar (doing other things), 2012/01/13 20:37:23]

I have no idea what anonymous means here.  Please add some comments to this, and
a few other obscure slots, like "other," record_tcmalloc," and "record_mmap."

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Done.  These regions should be classified more precisely later for more detailed
profiling, for example [heap] and [stack] regions, but it's enough for now.

+    RegionStats other;
+    RegionStats record_mmap;
+    RegionStats record_tcmalloc;
+  };
+
+  // Get the DeepBucket object corresponding to the given 'bucket'.
+  // DeepBucket is an extension to Bucket which is declared above.
+  DeepBucket* GetDeepBucket(Bucket* bucket);
+
+  // Reset committed_size member variables in DeepBucket objects to 0.

[jar (doing other things), 2012/01/13 20:37:23]

I'm unclear on the relationship between DeepBucket and the bucket_table
argument.  I don't understand how many such objects are going to be processed,
and am confused by the pointer to pointer to Bucket... which perhaps suggests an
array of pointers to buckets(?) or perhaps you're passing in the & of a given
Bucket*, but that seems strange, becauuse I don't expect you to change the
Bucket* (based on the description).

In general, we only pass in a pointer if we intend to be able to modify the the
value.  I suspect you really meant to pass something like a const Bucket*&...
but I can't tell yet.. and it would be nice if the comment and prototype clued
me in better.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

'Bucket**' is a data structure used in the original heap-profile-table.cc for
hash tables.  (For example, see "Bucket* GetBucket(..., Bucket** table)",
"Bucket** MakeSortedBucketList()" and "Bucket** table_" in
heap-profile-table.h.)

We can use typedef for Bucket**, but I think keeping it Bucket** is better for
consistency.

[jar (doing other things), 2012/01/26 20:19:02]

I think this must be part of newer merges, as I don't recall this stuff, and
need to read more. (It could also be that I'm senile, and/or forgot about it).

On 2012/01/19 11:54:14, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/01/30 12:54:53]

Bucket** is a very internal structure of heap-profile-table, not exposed out of
heap-profile-table.  I think it might be a reason why you don't know it. 
deep-heap-profile reads Bucket** as it's a friend of heap-profile-table.

On 2012/01/26 20:19:02, jar wrote:

+  void ResetCommittedSize(Bucket** bucket_table);
+
+  // Fill bucket data in 'table' into buffer 'buffer' of size 'size', and return

[jar (doing other things), 2012/01/13 20:37:23]

nit: If you wish to refer to an argument during your comment, please surround it
with vertical bars.  If you are refering to an argument, please spell it
identically to the real arg.

for example, possibly:    'table' ---> |bucket_table|

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Like some other styling issues, I followed the existing code style such as
heap-profile-table.{h|cc}.  What do you think which we should follow:
google-perftools (third_party/tcmalloc) or Chromium here?

I kept "'" in the updated patch for now.

[jar (doing other things), 2012/01/26 20:19:02]

In new files, always try to use newer style.  When in old files, making minor
internal changes, stick with the pre-existing style.

Since this is a brand new file, I'd suggest using current style.

There is a second question of where we should land new files that are not part
of the upstream TCMalloc code. I think it is generally easier to do merges if we
avoid adding our files to the list... but I haven't been staring at this code in
a while, and don't recall exactly where we hung our add-on files.  I think you
should find some shim code, and probably try to land your files around that...
unless you are planning on up-streaming the changes RSN.

On 2012/01/19 11:54:14, Dai Mikurube wrote:

[Dai Mikurube (NOT FULLTIME), 2012/01/30 12:54:53]

Got it, agreed.  Changed the style to |argument|-style.


Your second question is a good point to be discussed.  I thought about moving it
out of third_party, actually.

The reason why I finally chose to locate this code in third_party/tcmalloc is
because :
1) It has to read some internal data structures in heap-profile-table.
2) For (1), deep-heap-profile must be a friend of heap-profile-table, or
heap-profile-table must disclose Bucket** and AllocationMap by some getter
functions.
3) I chose the former since :
3-a) 'friend' affects less than disclosing getters in terms of security-like
issues.  friend allows access only to deep-heap-profile.
3-b) 'friend' changes less lines of code in the original heap-profile*.{cc|h}. 
Less changes in the original code makes us happy when merging upstream versions
of tcmalloc in future.


On 2012/01/26 20:19:02, jar wrote:

+  // the size occupied by the bucket data in 'buffer'.  'bucket_length' is the
+  // offset for 'buffer' to start filling.  Statistics in 'stats' is updated if
+  // it's not NULL.
+  int FillBucketTable(Bucket** bucket_table, char buffer[], int buffer_size,

[jar (doing other things), 2012/01/13 20:37:23]

Here again, the comment is made confusing by the prototype.  I don't understand
why you're passing in a pointer to pointer to Bucket.  The comment suggests this
is only a data source... so I would have expected some const ref to clarify this
fact.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

The same as above comment in Line 98.  Another option might be adding comments
for Bucket** for all functions using Bucket**.  What do you think?

+                      int bucket_length, HeapProfileTable::Stats* stats);
+
+  // Open /proc/pid/pagemap and store its file descriptor in pagemap_fd_.
+  void OpenProcPagemap();
+
+  // Seek the offset of the open pagemap file pagemap_fd_.

[jar (doing other things), 2012/01/13 20:37:23]

nit: "Seek the ..." --> "Seek to the  ..."

What does the return value signify?

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Modified the function and added a comment.  It returns true if succeeded.

+  bool SeekProcPagemap(uint64 address);
+
+  // Read a pagemap state from the current pagemap_fd_ offset.

[jar (doing other things), 2012/01/13 20:37:23]

What does the return value mean?

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Added a comment.  It returns true if succeeded.

+  bool ReadProcPagemap(PageState* state);
+
+  // Get a committed (resident + swapped) size in the memory region
+  // from 'addr' to 'addr + size'.

[jar (doing other things), 2012/01/13 20:37:23]

nit: clearer English, plus please spell argument names.

Suggest for example:
Returns the number of committed or resident bytes of the memory region starting
at |address| and ending at |address + size - 1| inclusive.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Thanks for a nice example.  I used your example with a little changes.  (e.g.
used "'" as replied above.)

+  size_t GetCommittedSize(uint64 address, size_t size);
+
+  // Initialize 'virtual_bytes' and 'committed_bytes' in RegionStats.
+  void InitRegionStats(RegionStats* stats);

[jar (doing other things), 2012/01/13 20:37:23]

This looks like it should be a method on RegionStats.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Made it RegionStats::Initialize(), and made *ProcPagemap and GetCommittedSize
static.

+
+  // Add the virtual size (end - start) and the committed size in the region

[jar (doing other things), 2012/01/13 20:37:23]

nit: "committed size" --> "committed byte count"

nit: The use of the word "Add" confused me, because it seem to say "Add the two
values together."   Better would be:

Update |stats| to include the tallies of virtual_size and committed bytes in the
region from |start_adress| to |end_address - 1| inclusive.

(note that your comment is especially unclear about whether the end_address is
part of the region, or just beyond teh end of the region, but I'm taking a guess
here.

Why are you sometimes passing in start and length, and other times passing in
start and end??

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Decided to use your recommendation, and changed to describe all regions by pairs
of 'first_address'es and 'last_address'es inclusive.

+  // to 'stats'.
+  void RecordRegionStats(uint64 start_address, uint64 end_address,

[jar (doing other things), 2012/01/13 20:37:23]

This also looks like a method on RegionStats.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Made it RegionStats::Record().

+                         RegionStats* stats);
+
+  // Compute the global statistics from /proc/self/maps and pagemap, and
+  // store them in stats_.
+  void GetGlobalStats();
+
+  // Record the committed memory size of each allocation and mmap.
+  static void RecordAlloc(const void* pointer, AllocValue* alloc_value,

[jar (doing other things), 2012/01/13 20:37:23]

I can't tell what this function does from the comment... and I'm especially at a
loss about what the arguments are for.

[Dai Mikurube (NOT FULLTIME), 2012/01/19 11:54:14]

Modified the comment as below :
Record both virtual and committed byte counts of malloc and mmap regions as
callback functions for AllocationMap::Iterate().

+                          DeepHeapProfile* deep_profile);
+  static void RecordMMap(const void* pointer, AllocValue* alloc_value,
+                         DeepHeapProfile* deep_profile);
+  void RecordAllAllocs();
+
+  // Fill re-formatted /proc/self/maps into 'buffer' of size 'buffer_size'.
+  void WriteMapsToFile(char buffer[], int buffer_size);
+
+  // Fill a bucket (a bucket id and its corresponding calling stack) into
+  // 'buffer' of size 'buffer_size'.
+  int FillBucketForBucketFile(const DeepBucket* deep_bucket,
+                              char buffer[],
+                              int buffer_size);
+
+  // Write a bucket table 'table' into a file of 'bucket_fd'.
+  void WriteBucketsTableToBucketFile(Bucket** table, RawFD bucket_fd);
+
+  // Write both malloc and mmap bucket tables into "a bucket file".
+  void WriteBucketsToBucketFile();
+
+  // Fill a (deep) bucket into 'buffer' from the offset 'used_in_buffer'.
+  // Add the sizes to 'profile_stats' if it's not NULL.
+  static int UnparseBucket(const DeepBucket& b,
+                           char* buffer, int used_in_buffer, int buffer_size,
+                           const char* extra,
+                           Stats* profile_stats);
+
+  // Fill statistics of a region into 'buffer'.
+  static int UnparseRegionStats(const RegionStats* stats,
+                                const char* name,
+                                char* buffer,
+                                int used_in_buffer,
+                                int buffer_size);
+
+  // Fill global statistics into 'buffer'.
+  int UnparseGlobalStats(char* buffer, int used_in_buffer, int buffer_size);
+
+#endif  // DEEP_HEAP_PROFILE
+  HeapProfileTable* heap_profile_;
+#ifdef DEEP_HEAP_PROFILE
+
+  int pagemap_fd_;         // File descriptor of /proc/self/pagemap.
+  pid_t most_recent_pid_;  // Process ID of the last dump.  This could change.
+  GlobalStats stats_;      // Stats about total memory.
+  int dump_count_;         // The number of dumps.
+  char* filename_prefix_;  // Output file prefix.
+  char* profiler_buffer_;  // Buffer we use many times.
+
+  int bucket_id_;
+  DeepBucketMap* deep_bucket_map_;
+#endif  // DEEP_HEAP_PROFILE
+
+  DISALLOW_COPY_AND_ASSIGN(DeepHeapProfile);
+};
+
+#endif  // BASE_DEEP_HEAP_PROFILE_H_