Classify memory usage by allocated type in Deep Memory Profiler.

Classify memory usage by allocated type in Deep Memory Profiler.


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

[Dai Mikurube (NOT FULLTIME), 2012-08-14 06:02:19]

Hi Marc, Jochen,

Could you take a look at it 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.)

[jochen (gone - plz use gerrit), 2012-08-14 11:17:53]

I'm not a good person to review this, I don't really understand what's going on
there

[M-A Ruel, 2012-08-19 02:40:00]

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right):

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/deep-heap-profile.cc:452: if (type == NULL)
Mixing #if and conditions without bracket is a recipe for subtle errors. I
recommend using brackets.

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right):

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/deep-heap-profile.h:95: const std::type_info*
type;
Note that type_info won't be useful on Windows since RTTI is disabled IIRC.

http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
File tools/deep_memory_profiler/dmprof (right):

http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
tools/deep_memory_profiler/dmprof:940: if buckets[bucket].typeinfo != None:
if buckets[bucket].typeinfo:
?

http://codereview.chromium.org/10825075/diff/10006/tools/find_runtime_symbols...
File tools/find_runtime_symbols/find_runtime_symbols.py (right):

http://codereview.chromium.org/10825075/diff/10006/tools/find_runtime_symbols...
tools/find_runtime_symbols/find_runtime_symbols.py:120: if isinstance(address,
str):
if isinstance(address, basestring):

[Dai Mikurube (NOT FULLTIME), 2012-08-20 10:35:27]

Thanks for your comments, Marc.

Updated the patch set.

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right):

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/deep-heap-profile.cc:452: if (type == NULL)
On 2012/08/19 02:40:00, Marc-Antoine Ruel wrote:
> Mixing #if and conditions without bracket is a recipe for subtle errors. I
> recommend using brackets.

Done.

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right):

http://codereview.chromium.org/10825075/diff/10006/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/deep-heap-profile.h:95: const std::type_info*
type;
On 2012/08/19 02:40:00, Marc-Antoine Ruel wrote:
> Note that type_info won't be useful on Windows since RTTI is disabled IIRC.

I know.  PROFILING_ALLOCATED_TYPE is always used with RTTI enabled.  (See
http://codereview.chromium.org/10411047/)
In addition, this option is available only on Linux now.

http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
File tools/deep_memory_profiler/dmprof (right):

http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
tools/deep_memory_profiler/dmprof:940: if buckets[bucket].typeinfo != None:
On 2012/08/19 02:40:00, Marc-Antoine Ruel wrote:
> if buckets[bucket].typeinfo:
> ?

Actually, buckets[bucket].typeinfo can be 0, but it should hit this 'if'.

http://codereview.chromium.org/10825075/diff/10006/tools/find_runtime_symbols...
File tools/find_runtime_symbols/find_runtime_symbols.py (right):

http://codereview.chromium.org/10825075/diff/10006/tools/find_runtime_symbols...
tools/find_runtime_symbols/find_runtime_symbols.py:120: if isinstance(address,
str):
On 2012/08/19 02:40:00, Marc-Antoine Ruel wrote:
> if isinstance(address, basestring):

Done.

[M-A Ruel, 2012-08-20 18:12:13]

lgtm

http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
File tools/deep_memory_profiler/dmprof (right):

http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
tools/deep_memory_profiler/dmprof:940: if buckets[bucket].typeinfo != None:
On 2012/08/20 10:35:27, Dai Mikurube wrote:
> On 2012/08/19 02:40:00, Marc-Antoine Ruel wrote:
> > if buckets[bucket].typeinfo:
> > ?
> 
> Actually, buckets[bucket].typeinfo can be 0, but it should hit this 'if'.

Ok

[Dai Mikurube (NOT FULLTIME), 2012-08-21 03:28:58]

On 2012/08/20 18:12:13, Marc-Antoine Ruel wrote:
> lgtm
> 
>
http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
> File tools/deep_memory_profiler/dmprof (right):
> 
>
http://codereview.chromium.org/10825075/diff/10006/tools/deep_memory_profiler...
> tools/deep_memory_profiler/dmprof:940: if buckets[bucket].typeinfo != None:
> On 2012/08/20 10:35:27, Dai Mikurube wrote:
> > On 2012/08/19 02:40:00, Marc-Antoine Ruel wrote:
> > > if buckets[bucket].typeinfo:
> > > ?
> > 
> > Actually, buckets[bucket].typeinfo can be 0, but it should hit this 'if'.
> 
> Ok

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

[Dai Mikurube (NOT FULLTIME), 2012-08-22 02:42:41]

Hi Marc,

Sorry, I additionally fixed "dmprof" for cases that no typeinfo are given in
heap dump files.

[Dai Mikurube (NOT FULLTIME), 2012-08-23 10:32:57]

Is the change in the Patch Set #8 still ok?  If so, I'll commit it later (after
10411047).

[M-A Ruel, 2012-08-23 19:16:54]

Index: tools/find_runtime_symbols/proc_maps.py
diff --git a/tools/find_runtime_symbols/parse_proc_maps.py
b/tools/find_runtime_symbols/proc_maps.py
old mode 100755
new mode 100644

FYI, note that the CQ won't correctly remove the +x bit, it's a bug in it.

lgtm

[Dai Mikurube (NOT FULLTIME), 2012-08-24 02:48:53]

On 2012/08/23 19:16:54, Marc-Antoine Ruel wrote:
> Index: tools/find_runtime_symbols/proc_maps.py
> diff --git a/tools/find_runtime_symbols/parse_proc_maps.py
> b/tools/find_runtime_symbols/proc_maps.py
> old mode 100755
> new mode 100644
> 
> FYI, note that the CQ won't correctly remove the +x bit, it's a bug in it.
> 
> lgtm

Thanks for the information.  I'll commit it by git cl dcommit.

[Dai Mikurube (NOT FULLTIME), 2012-08-29 04:42:41]

Just renamed some names and macros due to changes in
http://codereview.chromium.org/10411047/.  If no objections, I'll commit it
after 10411047 as is.

[Dai Mikurube (NOT FULLTIME), 2012-09-13 08:26:58]

Hi Marc,

Sorry for requesting review for this change again.  I just changed a JSON file:
policy.t0.json.  Could you take a look?

I noticed that this change can be committed separately from
http://codereview.chromium.org/10411047/.  I'm planning to commit it if it's ok.

[Dai Mikurube (NOT FULLTIME), 2012-09-13 10:49:46]

Correction: I won't commit it until the base patch is landed.

[M-A Ruel, 2012-09-17 17:42:32]

On 2012/09/13 08:26:58, Dai Mikurube wrote:
> Hi Marc,
> 
> Sorry for requesting review for this change again.  I just changed a JSON
file:
> policy.t0.json.  Could you take a look?

rubberstamp the json file.

[commit-bot: I haz the power, 2012-09-26 05:21:12]

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

[Dai Mikurube (NOT FULLTIME), 2012-09-26 05:28:56]

Thanks, Marc.  It'll be committed soon.

[commit-bot: I haz the power, 2012-09-26 13:30:18]

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

[commit-bot: I haz the power, 2012-09-26 16:37:25]

Change committed as 158812