Dump type statistics of malloc'ed objects.

Dump type statistics of malloc'ed objects.

This change adds type statistics in heap-profiler.  It dumps
"prefix.<pid>.<num>.type" along with heap-profiles (.heap),
which contains type information.

Type statistics is for example "    13:      520 @ N3WTF5MutexE",
which means that 13 objects of WTF::Mutex occupy 520 bytes.

It depends on a modified version of the LLVM/Clang compiler 
at deps/third_party/llvm-allocated-type. 


BUG=123758
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159952

[Dai Mikurube (NOT FULLTIME), 2012-08-14 07:26:33]

Hi Jim, Alexander,

Could you please take a look at this change when you have time?  (We are not in
a hurry since this change depends on http://codereview.chromium.org/10411047/. 
We need to wait for 10411047 landed.)

It changes heap-profiler and heap-profile-table, but it has no impact unless
PROFILING_ALLOCATED_TYPE is specified when building.  All changes are behind
"#if defined(PROFILING_ALLOCATED_TYPE)".

[Alexander Potapenko, 2012-08-17 11:44:51]

LGTM in general.

http://codereview.chromium.org/10830113/diff/1004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/1004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:536: len = snprintf(buf,
1023, "%6d: %8"PRId64" @ (no_typeinfo)\n",
Please use sizeof(buf) instead of an int literal.
BTW why this isn't 1024?

[Dai Mikurube (NOT FULLTIME), 2012-08-20 10:23:43]

Thank you for reviewing.

Fixed it.  I'll commit it after http://codereview.chromium.org/10411047/ is
landed.

http://codereview.chromium.org/10830113/diff/1004/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/1004/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:536: len = snprintf(buf,
1023, "%6d: %8"PRId64" @ (no_typeinfo)\n",
On 2012/08/17 11:44:51, Alexander Potapenko wrote:
> Please use sizeof(buf) instead of an int literal.
> BTW why this isn't 1024?

Done.  1023 may be typo...

[Alexander Potapenko, 2012-08-20 11:23:37]

LGTM

[jar (doing other things), 2012-08-20 21:59:09]

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:96: static const char
kAllocatedTypeStatsHeader[] = "allocated type statistics:\n";
nit: globals are prefixed by g_

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:508: const void* ptr,
AllocValue* v,
nit: one param per line.

Don't use single letter names for your variables.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:514: // In new abi,
type_info's NTBS is unique.
Use words rather than abbreviations in comments.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:531: const void* ptr,
AllocatedTypeCount* count, const DumpArgs& args) {
nit: one arg per line, wrap at paren

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:534: const char*
mangled_type_name = reinterpret_cast<const char*>(ptr);
nit: prefer static_cast

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:540: count->objects,
count->bytes, mangled_type_name);
Use a ternary expression, and you won't have to repeat these lines:

mangled_type_name ? mangled_type_name : "(no_typeinfo)"

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.h:338: : bytes(a),
objects(b) { }
nit: indent 4 spaces to a ":" for initializer.

One initializer per line.

Use meaningful names, not single letters.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.h:410: const void* ptr,
AllocatedTypeCount* size, const DumpArgs& args);
nit: prefer one arg per line, and you can wrap at the paren.

[Dai Mikurube (NOT FULLTIME), 2012-08-21 04:45:44]

Thanks for the comments, Jim.  Updated the patch set.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:96: static const char
kAllocatedTypeStatsHeader[] = "allocated type statistics:\n";
On 2012/08/20 21:59:09, jar wrote:
> nit: globals are prefixed by g_

It follows the other "static const" variables in this file for consistency.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:508: const void* ptr,
AllocValue* v,
On 2012/08/20 21:59:09, jar wrote:
> nit: one param per line.
> 
> Don't use single letter names for your variables.

Renamed it to value, but I still prefer v for consistency since all other
functions use v in this file.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:514: // In new abi,
type_info's NTBS is unique.
On 2012/08/20 21:59:09, jar wrote:
> Use words rather than abbreviations in comments.

Done.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:531: const void* ptr,
AllocatedTypeCount* count, const DumpArgs& args) {
On 2012/08/20 21:59:09, jar wrote:
> nit: one arg per line, wrap at paren

Done.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:534: const char*
mangled_type_name = reinterpret_cast<const char*>(ptr);
On 2012/08/20 21:59:09, jar wrote:
> nit: prefer static_cast

Thanks for the good catch.  I mistook.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:540: count->objects,
count->bytes, mangled_type_name);
On 2012/08/20 21:59:09, jar wrote:
> Use a ternary expression, and you won't have to repeat these lines:
> 
> mangled_type_name ? mangled_type_name : "(no_typeinfo)"

Done.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.h:338: : bytes(a),
objects(b) { }
On 2012/08/20 21:59:09, jar wrote:
> nit: indent 4 spaces to a ":" for initializer.
> 
> One initializer per line.
> 
> Use meaningful names, not single letters.

Done.

http://codereview.chromium.org/10830113/diff/6001/third_party/tcmalloc/chromi...
third_party/tcmalloc/chromium/src/heap-profile-table.h:410: const void* ptr,
AllocatedTypeCount* size, const DumpArgs& args);
On 2012/08/20 21:59:09, jar wrote:
> nit: prefer one arg per line, and you can wrap at the paren.

Done.

[Dai Mikurube (NOT FULLTIME), 2012-08-29 04:45:52]

Updated the change based on renaming in
http://codereview.chromium.org/10411047/.

[Dai Mikurube (NOT FULLTIME), 2012-09-26 06:49:52]

Hi Jim,

Could you take a look at this?  (Not in a very hurry, but I wonder if you could
review it this week.)

It's a "type statistics" dumper that uses the type_profiler introduced in
http://crrev.com/158752.

All changes are contained in "#if defined(TYPE_PROFILING)".  No impact on normal
builds.

[Dai Mikurube (NOT FULLTIME), 2012-10-01 09:06:37]

Hi,

just a reminder ping.

[jar (doing other things), 2012-10-01 21:59:44]

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:426: const char*
file_name) const {
nit: does this need to wrap here?

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:436: return;
probably want to destructor and dealloc_(type_size_map);

Better might be to move lines 433-436  up  to in front of 427, so there is no
need to cleanup on this early return.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:513: const void* key;
nit: initialize key to NULL here... then you don't need line 519-520.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:516: // type_info's
Null-Terminated Byte String (NTBS) is unique.
I don't understand the significance of this comment.  

Should it be removed?

Can we get rid of the curlies then as well?

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.h:337: TypeCount(size_t
bytes_arg, unsigned int objects_arg)
nit: constructors are always listed first in structs / classes (or at least
ahead of members).... <sigh>.... but this file seems to not comply with that
style.... so I guess it can stay as is.

[Dai Mikurube (NOT FULLTIME), 2012-10-02 06:35:12]

Thanks, Jim.  Updated the patch.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:426: const char*
file_name) const {
On 2012/10/01 21:59:44, jar wrote:
> nit: does this need to wrap here?

Ah, not.  Removed the newline.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:436: return;
On 2012/10/01 21:59:44, jar wrote:
> probably want to destructor and dealloc_(type_size_map);
> 
> Better might be to move lines 433-436  up  to in front of 427, so there is no
> need to cleanup on this early return.

Chose your suggestion.  Thanks.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:513: const void* key;
On 2012/10/01 21:59:44, jar wrote:
> nit: initialize key to NULL here... then you don't need line 519-520.

Done.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:516: // type_info's
Null-Terminated Byte String (NTBS) is unique.
On 2012/10/01 21:59:44, jar wrote:
> I don't understand the significance of this comment.  
> 
> Should it be removed?
> 
> Can we get rid of the curlies then as well?

Done.

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profile-table.h (right):

http://codereview.chromium.org/10830113/diff/26001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.h:337: TypeCount(size_t
bytes_arg, unsigned int objects_arg)
On 2012/10/01 21:59:44, jar wrote:
> nit: constructors are always listed first in structs / classes (or at least
> ahead of members).... <sigh>.... but this file seems to not comply with that
> style.... so I guess it can stay as is.

Yeah, I thought that, but I followed the others in this file.

[jar (doing other things), 2012-10-02 16:43:10]

Two last requests.... and if you're fine with them, then I'll L G T M.

http://codereview.chromium.org/10830113/diff/28003/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/28003/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:506: void
HeapProfileTable::CountUpTypeIterator(
This name bothered me... and I finally have suggestions:

TallyTypesItererator()

Note the use of the "s" so that it doesn't read like a "TypeIterator" (where the
word Type modifies Iterator, rather than being a separate noun).

http://codereview.chromium.org/10830113/diff/28003/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:526: void
HeapProfileTable::DumpTypeIterator(const void* ptr,
Here just add an "s" to "Types" to create:

DumpTypesIterator()

I tried to be similar to the other names in this file.

[Dai Mikurube (NOT FULLTIME), 2012-10-03 03:41:28]

Thanks, Jim.  Renamed these functions.

http://codereview.chromium.org/10830113/diff/28003/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/10830113/diff/28003/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:506: void
HeapProfileTable::CountUpTypeIterator(
On 2012/10/02 16:43:10, jar wrote:
> This name bothered me... and I finally have suggestions:
> 
> TallyTypesItererator()
> 
> Note the use of the "s" so that it doesn't read like a "TypeIterator" (where
the
> word Type modifies Iterator, rather than being a separate noun).

Done.

http://codereview.chromium.org/10830113/diff/28003/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:526: void
HeapProfileTable::DumpTypeIterator(const void* ptr,
On 2012/10/02 16:43:10, jar wrote:
> Here just add an "s" to "Types" to create:
> 
> DumpTypesIterator()
> 
> I tried to be similar to the other names in this file.

Done.

[jar (doing other things), 2012-10-03 16:40:35]

lgtm

[Dai Mikurube (NOT FULLTIME), 2012-10-03 16:56:59]

On 2012/10/03 16:40:35, jar wrote:
> lgtm

Thanks!  I'll be committing...

[commit-bot: I haz the power, 2012-10-03 16:57:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/10830113/35001

[commit-bot: I haz the power, 2012-10-03 19:45:05]

Change committed as 159952