Heap Profiling for Oilpan

Heap Profiling for Oilpan

This CL enables heap profiling for Oilpan (BlinkGC) with --enable-heap-profiling.

BUG=585063
Committed: https://crrev.com/818f6933fe0e6081de6bf7d4143f73b4198335f2
Cr-Commit-Position: refs/heads/master@{#375860}

[hajimehoshi, 2016-02-16 10:52:41]

Description was changed from

==========
Heap Profiling for Oilpan

BUG=
==========

to

==========
Heap Profiling for Oilpan

BUG=
==========

[hajimehoshi, 2016-02-16 10:52:41]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org, primiano@chromium.org

[hajimehoshi, 2016-02-16 10:54:11]

Description was changed from

==========
Heap Profiling for Oilpan

BUG=
==========

to

==========
Heap Profiling for Oilpan

This CL enables heap profiling for Oilpan (BlinkGC) with
--enable-heap-profiling.

BUG=585063
==========

[hajimehoshi, 2016-02-16 11:02:07]

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:66: //
TODO(hajimehoshi): This doesn't work causing assert error:
[process_memory_dump.cc(149)] Check failed: 0ul ==
allocator_dumps_.count(mad->absolute_name()) (0 vs. 1)
primiano: I simply copied this dump heap usage logic from 
PartitionAllocMemoryDumpProvider::onMemoryDump but this doesn't work with
assertion error as the comment says. It looks like onMemoryDump in
BlinkGCMemoryDumpProvider is much different from that in
PartitionAllocMemoryDumpProvider and BlinkGCMemoryDumpProvider. Would you tell
me what I should fix?

[Primiano Tucci (use gerrit), 2016-02-16 11:16:42]

awesome. Told you that was supposed to be easy ;-)

non-owner LGTM with few nits and a suggestion on a one line fix to solve the
DCHECK

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:66: //
TODO(hajimehoshi): This doesn't work causing assert error:
[process_memory_dump.cc(149)] Check failed: 0ul ==
allocator_dumps_.count(mad->absolute_name()) (0 vs. 1)
On 2016/02/16 11:02:07, hajimehoshi wrote:
> primiano: I simply copied this dump heap usage logic from 
> PartitionAllocMemoryDumpProvider::onMemoryDump but this doesn't work with
> assertion error as the comment says. It looks like onMemoryDump in
> BlinkGCMemoryDumpProvider is much different from that in
> PartitionAllocMemoryDumpProvider and BlinkGCMemoryDumpProvider. Would you tell
> me what I should fix?

Oh I think I know what it is: is the overhead dumping in
web_process_memory_dump_impl.cc (WebProcessMemoryDumpImpl::dumpHeapUsage())
which always dumps the overhead estimation with 
overhead.DumpInto("tracing/heap_profiler")

Before this CL you have only one tracing/heap_profiler (for partition alloc),
now you have two.

Simply change the code in WebProcessMemoryDumpImpl::dumpHeapUsage and make it
strcat the |allocator_name| when you do the overhead dumping, so it becomes
something like "tracing/blink_gc_heap_profiler" and
"tracing/partition_alloc_heap_profiler".
SHould be a one line fix

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:98: }
similarly to the other CL, I think you might want to tear down the allocation
register here?

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:122:
context.type_name = "(other)";
I think you can leave it empty, and will just appear as unknown

[hajimehoshi, 2016-02-17 09:19:40]

Thank you!

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:66: //
TODO(hajimehoshi): This doesn't work causing assert error:
[process_memory_dump.cc(149)] Check failed: 0ul ==
allocator_dumps_.count(mad->absolute_name()) (0 vs. 1)
On 2016/02/16 11:16:42, Primiano Tucci wrote:
> On 2016/02/16 11:02:07, hajimehoshi wrote:
> > primiano: I simply copied this dump heap usage logic from 
> > PartitionAllocMemoryDumpProvider::onMemoryDump but this doesn't work with
> > assertion error as the comment says. It looks like onMemoryDump in
> > BlinkGCMemoryDumpProvider is much different from that in
> > PartitionAllocMemoryDumpProvider and BlinkGCMemoryDumpProvider. Would you
tell
> > me what I should fix?
> 
> Oh I think I know what it is: is the overhead dumping in
> web_process_memory_dump_impl.cc (WebProcessMemoryDumpImpl::dumpHeapUsage())
> which always dumps the overhead estimation with 
> overhead.DumpInto("tracing/heap_profiler")
> 
> Before this CL you have only one tracing/heap_profiler (for partition alloc),
> now you have two.
> 
> Simply change the code in WebProcessMemoryDumpImpl::dumpHeapUsage and make it
> strcat the |allocator_name| when you do the overhead dumping, so it becomes
> something like "tracing/blink_gc_heap_profiler" and
> "tracing/partition_alloc_heap_profiler".
> SHould be a one line fix

Thanks, done.

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:98: }
On 2016/02/16 11:16:42, Primiano Tucci wrote:
> similarly to the other CL, I think you might want to tear down the allocation
> register here?

In PartitionAllocMemoryDumpProvider, no tearing down happens. Which CL did you
mean?

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:122:
context.type_name = "(other)";
On 2016/02/16 11:16:42, Primiano Tucci wrote:
> I think you can leave it empty, and will just appear as unknown

Done.

[hajimehoshi, 2016-02-17 09:20:09]

hajimehoshi@chromium.org changed reviewers:
+ kinuko@chromium.org

[hajimehoshi, 2016-02-17 09:20:25]

+kinuko for content/child

[haraken, 2016-02-17 09:23:05]

LGTM

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:98: }
On 2016/02/16 11:16:42, Primiano Tucci wrote:
> similarly to the other CL, I think you might want to tear down the allocation
> register here?

Would it make sense to support disabling the heap profiling? Given that
'--enable-heap-profiling' is the only way to enable the heap profiling (and it
doesn't make sense to disable it later), it would make more sense to change the
signature of this method to onHeapProfilingEnabled() (without taking the
|enabled| parameter)?

[hajimehoshi, 2016-02-17 09:57:39]

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:98: }
On 2016/02/17 09:23:05, haraken wrote:
> On 2016/02/16 11:16:42, Primiano Tucci wrote:
> > similarly to the other CL, I think you might want to tear down the
allocation
> > register here?
> 
> Would it make sense to support disabling the heap profiling? Given that
> '--enable-heap-profiling' is the only way to enable the heap profiling (and it
> doesn't make sense to disable it later), it would make more sense to change
the
> signature of this method to onHeapProfilingEnabled() (without taking the
> |enabled| parameter)?
> 

+1 to haraken

[haraken, 2016-02-17 10:12:29]

On 2016/02/17 09:57:39, hajimehoshi wrote:
>
https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
> (right):
> 
>
https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:98: }
> On 2016/02/17 09:23:05, haraken wrote:
> > On 2016/02/16 11:16:42, Primiano Tucci wrote:
> > > similarly to the other CL, I think you might want to tear down the
> allocation
> > > register here?
> > 
> > Would it make sense to support disabling the heap profiling? Given that
> > '--enable-heap-profiling' is the only way to enable the heap profiling (and
it
> > doesn't make sense to disable it later), it would make more sense to change
> the
> > signature of this method to onHeapProfilingEnabled() (without taking the
> > |enabled| parameter)?
> > 
> 
> +1 to haraken

Either way, we can address the issue in a follow-up CL (if primiano@ likes the
idea). You can go ahead with this CL.

[Primiano Tucci (use gerrit), 2016-02-17 10:13:46]

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1701813003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:98: }
On 2016/02/17 09:57:38, hajimehoshi wrote:
> On 2016/02/17 09:23:05, haraken wrote:
> > On 2016/02/16 11:16:42, Primiano Tucci wrote:
> > > similarly to the other CL, I think you might want to tear down the
> allocation
> > > register here?
> > 
> > Would it make sense to support disabling the heap profiling? Given that
> > '--enable-heap-profiling' is the only way to enable the heap profiling (and
it
> > doesn't make sense to disable it later), it would make more sense to change
> the
> > signature of this method to onHeapProfilingEnabled() (without taking the
> > |enabled| parameter)?
> > 
> 
> +1 to haraken

You are right. The only motivation I see for tear-down is that after you finish
the tracing session you go back to a normal performance/memory state, freeing
out hooks and memory used for metadata.

https://codereview.chromium.org/1701813003/diff/20001/content/child/web_proce...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1701813003/diff/20001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:181: std::string base_name =
base::StringPrintf("tracing/%s", allocator_name);
I'd probably keep heap_profiler prefix, to make it clear that it is overhead for
heap_profiler, i.e. "tracing/heap_profiler_%s"

[hajimehoshi, 2016-02-17 10:34:04]

https://codereview.chromium.org/1701813003/diff/20001/content/child/web_proce...
File content/child/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1701813003/diff/20001/content/child/web_proce...
content/child/web_process_memory_dump_impl.cc:181: std::string base_name =
base::StringPrintf("tracing/%s", allocator_name);
On 2016/02/17 10:13:46, Primiano Tucci wrote:
> I'd probably keep heap_profiler prefix, to make it clear that it is overhead
for
> heap_profiler, i.e. "tracing/heap_profiler_%s"

Done.

[kinuko, 2016-02-17 10:49:14]

content/ lgtm

[hajimehoshi, 2016-02-17 10:54:19]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-02-17 10:54:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1701813003/#ps40001
(title: "Change base_name")

[commit-bot: I haz the power, 2016-02-17 10:55:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701813003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701813003/40001

[commit-bot: I haz the power, 2016-02-17 12:04:09]

Description was changed from

==========
Heap Profiling for Oilpan

This CL enables heap profiling for Oilpan (BlinkGC) with
--enable-heap-profiling.

BUG=585063
==========

to

==========
Heap Profiling for Oilpan

This CL enables heap profiling for Oilpan (BlinkGC) with
--enable-heap-profiling.

BUG=585063
==========

[commit-bot: I haz the power, 2016-02-17 12:04:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-17 12:05:32]

Description was changed from

==========
Heap Profiling for Oilpan

This CL enables heap profiling for Oilpan (BlinkGC) with
--enable-heap-profiling.

BUG=585063
==========

to

==========
Heap Profiling for Oilpan

This CL enables heap profiling for Oilpan (BlinkGC) with
--enable-heap-profiling.

BUG=585063

Committed: https://crrev.com/818f6933fe0e6081de6bf7d4143f73b4198335f2
Cr-Commit-Position: refs/heads/master@{#375860}
==========

[commit-bot: I haz the power, 2016-02-17 12:05:33]

Patchset 3 (id:??) landed as
https://crrev.com/818f6933fe0e6081de6bf7d4143f73b4198335f2
Cr-Commit-Position: refs/heads/master@{#375860}