[tracing] Track number of allocations in heap profiler

[tracing] Track number of allocations in heap profiler

This CL adds a count along with size that is tracked for each allocation
context to the trace files.

BUG=603648
Committed: https://crrev.com/1eedc59e16922772a374b00de7546e6c34a30c11
Cr-Commit-Position: refs/heads/master@{#387517}

[ssid, 2016-04-14 18:53:12]

Description was changed from

==========
track number of allocations in heap profiler.

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

to

==========
[tracing] Track number of allocations in heap profiler

This CL adds a count along with size that is tracked for each allocation
context to the trace files.

BUG=603648
==========

[ssid, 2016-04-14 18:55:40]

ssid@chromium.org changed reviewers:
+ dskiba@chromium.org, mariakhomenko@chromium.org

[ssid, 2016-04-14 18:55:40]

Can one of you take a look at this CL before I send over to london tomorrow.
Thanks!

[Dmitry Skiba, 2016-04-14 19:07:29]

dskiba@google.com changed reviewers:
+ dskiba@google.com

[Dmitry Skiba, 2016-04-14 19:07:30]

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_allocation_context.h (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_allocation_context.h:49: struct
AllocationsSizeAndCount {
You call this 'metrics' everywhere else (metrics_by_context), so lets call it
AllocationMetrics?

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:188:
metrics_by_context[alloc_size.context].count++;
Let's avoid second lookup by caching result from the first one.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:115:
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount> metricsByContext;
I wonder if we should have typedef for this, like AllocationMetricsByContext or
something, because every dump provider have to declare it.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:122:
metricsByContext[allocSize.context].count++;
Avoid second lookup.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:73:
metricsByContext[allocSize.context].count++;
Avoid second lookup.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:174: {
Shouldn't this { be on the previous line?

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:175: if
(!metrics_by_context.empty()) {
Something is wrong with formatting, I think this block got indented.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.h:81: const
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount>&
Yup, we should typedef this monstrosity.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebProcessMemoryDump.h (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebProcessMemoryDump.h:128: virtual void
dumpHeapUsage(const base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount>& metricsByContext,
base::trace_event::TraceEventMemoryOverhead& overhead, const char*
allocatorName)
OMG long line, how did it pass presubmit check?

[Primiano Tucci (use gerrit), 2016-04-14 19:37:21]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-04-14 19:37:22]

//base/trace_event LGTM % dskiba@ comments (and some extra small mine) and
Maria's if she has any, so you don't have for a cross-timezone RTT.
Thanks a lot for this patch. 

As a general suggestion, I think that now metrics_by_context, as a variable
name, is becoming a very obscure name.
Should that just be grouped_allocations ?

Thanks Dmitry for helping with this review.

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_allocation_context.h (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_allocation_context.h:49: struct
AllocationsSizeAndCount {
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> You call this 'metrics' everywhere else (metrics_by_context), so lets call it
> AllocationMetrics?

+1

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:71: // asserts that
it is present and has the expected size.
+ and count :)

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:209: // |        |  
    size   count |   size    count | size  count |
many thanks for updating this :)

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:188:
metrics_by_context[alloc_size.context].count++;
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> Let's avoid second lookup by caching result from the first one.

+1. Good suggestion ;-)

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:115:
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount> metricsByContext;
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> I wonder if we should have typedef for this, like AllocationMetricsByContext
or
> something, because every dump provider have to declare it.

IMHO typedef just make the code less readable as you have to clink another thing
to realize that this is a hashtable under the hoods.
I'll probably leave as it is, it's just 3 dump providers after all:)

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:175: if
(!metrics_by_context.empty()) {
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> Something is wrong with formatting, I think this block got indented.

Yup I think the TL;DR is don't run clang-format on blink :)

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebProcessMemoryDump.h (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebProcessMemoryDump.h:128: virtual void
dumpHeapUsage(const base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount>& metricsByContext,
base::trace_event::TraceEventMemoryOverhead& overhead, const char*
allocatorName)
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> OMG long line, how did it pass presubmit check?

Welcome to blink, where 80 col limit does not apply :)

[Maria, 2016-04-14 20:11:34]

Is there a place where size gets decremented when an object is freed?

https://codereview.chromium.org/1877313003/diff/40001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1877313003/diff/40001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:310: const
hash_map<AllocationContext, AllocationMetrics>& bytes_by_size,
metrics_by_size?

https://codereview.chromium.org/1877313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:115:
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationMetrics> metricsByContext;
not sure about blink style, but is this a long line?

[ssid, 2016-04-14 20:17:11]

Thanks addressed comments.

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_allocation_context.h (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_allocation_context.h:49: struct
AllocationsSizeAndCount {
On 2016/04/14 19:37:21, Primiano Tucci wrote:
> On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> > You call this 'metrics' everywhere else (metrics_by_context), so lets call
it
> > AllocationMetrics?
> 
> +1

Done.

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:71: // asserts that
it is present and has the expected size.
On 2016/04/14 19:37:21, Primiano Tucci wrote:
> + and count :)

Done.

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:209: // |        |  
    size   count |   size    count | size  count |
On 2016/04/14 19:37:21, Primiano Tucci wrote:
> many thanks for updating this :)

Acknowledged.

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:188:
metrics_by_context[alloc_size.context].count++;
On 2016/04/14 19:37:21, Primiano Tucci wrote:
> On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> > Let's avoid second lookup by caching result from the first one.
> 
> +1. Good suggestion ;-)

Done.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:115:
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount> metricsByContext;
On 2016/04/14 19:37:21, Primiano Tucci wrote:
> On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> > I wonder if we should have typedef for this, like AllocationMetricsByContext
> or
> > something, because every dump provider have to declare it.
> 
> IMHO typedef just make the code less readable as you have to clink another
thing
> to realize that this is a hashtable under the hoods.
> I'll probably leave as it is, it's just 3 dump providers after all:)

Acknowledged.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:122:
metricsByContext[allocSize.context].count++;
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> Avoid second lookup.

Done.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:73:
metricsByContext[allocSize.context].count++;
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> Avoid second lookup.

Done.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:174: {
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> Shouldn't this { be on the previous line?

Done.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:175: if
(!metrics_by_context.empty()) {
On 2016/04/14 19:37:22, Primiano Tucci wrote:
> On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> > Something is wrong with formatting, I think this block got indented.
> 
> Yup I think the TL;DR is don't run clang-format on blink :)

git cl format did this. I have manually formatted code to be consistent with the
files now.

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.h (right):

https://codereview.chromium.org/1877313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.h:81: const
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationsSizeAndCount>&
On 2016/04/14 19:07:30, Dmitry Skiba wrote:
> Yup, we should typedef this monstrosity.

Not adding as suggested by primiano.

https://codereview.chromium.org/1877313003/diff/40001/base/trace_event/heap_p...
File base/trace_event/heap_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1877313003/diff/40001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:310: const
hash_map<AllocationContext, AllocationMetrics>& bytes_by_size,
On 2016/04/14 20:11:34, Maria wrote:
> metrics_by_size?

Done.

https://codereview.chromium.org/1877313003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/1877313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PartitionAllocMemoryDumpProvider.cpp:115:
base::hash_map<base::trace_event::AllocationContext,
base::trace_event::AllocationMetrics> metricsByContext;
On 2016/04/14 20:11:34, Maria wrote:
> not sure about blink style, but is this a long line?

yes, long lines are allowed in blink.

[ssid, 2016-04-14 20:30:53]

ssid@chromium.org changed reviewers:
+ haraken@chromium.org, rbyers@chromium.org

[ssid, 2016-04-14 20:30:54]

+haraken for changes in dump providers and webkit.

+rbyers for 1 line change in blink api in WebProcessMemoryDump.h

PTAL, thanks.

[Maria, 2016-04-14 20:33:04]

lgtm

[Rick Byers, 2016-04-14 20:34:37]

third_party/WebKit/public LGTM

[ssid, 2016-04-14 20:46:26]

+petrcermak fyi.

[ssid, 2016-04-14 20:47:34]

Patchset #6 (id:100001) has been deleted

[Dmitry Skiba, 2016-04-14 21:29:41]

https://codereview.chromium.org/1877313003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc (right):

https://codereview.chromium.org/1877313003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/web_process_memory_dump_impl.cc:185:
allocator_name);
nit: wrong indentation

[haraken, 2016-04-14 23:49:38]

LGTM

[ssid, 2016-04-15 00:16:18]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[ssid, 2016-04-15 00:16:35]

The CQ bit was unchecked by ssid@chromium.org

[commit-bot: I haz the power, 2016-04-15 00:16:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877313003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877313003/120001

[ssid, 2016-04-15 00:22:22]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 00:23:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877313003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877313003/140001

[commit-bot: I haz the power, 2016-04-15 01:46:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 01:46:56]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-04-15 01:51:56]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-04-15 01:51:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, mariakhomenko@chromium.org, rbyers@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1877313003/#ps140001
(title: "fix indentation.")

[commit-bot: I haz the power, 2016-04-15 01:52:18]

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

[commit-bot: I haz the power, 2016-04-15 01:56:57]

Description was changed from

==========
[tracing] Track number of allocations in heap profiler

This CL adds a count along with size that is tracked for each allocation
context to the trace files.

BUG=603648
==========

to

==========
[tracing] Track number of allocations in heap profiler

This CL adds a count along with size that is tracked for each allocation
context to the trace files.

BUG=603648
==========

[commit-bot: I haz the power, 2016-04-15 01:56:58]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-04-15 01:57:52]

Description was changed from

==========
[tracing] Track number of allocations in heap profiler

This CL adds a count along with size that is tracked for each allocation
context to the trace files.

BUG=603648
==========

to

==========
[tracing] Track number of allocations in heap profiler

This CL adds a count along with size that is tracked for each allocation
context to the trace files.

BUG=603648

Committed: https://crrev.com/1eedc59e16922772a374b00de7546e6c34a30c11
Cr-Commit-Position: refs/heads/master@{#387517}
==========

[commit-bot: I haz the power, 2016-04-15 01:57:53]

Patchset 7 (id:??) landed as
https://crrev.com/1eedc59e16922772a374b00de7546e6c34a30c11
Cr-Commit-Position: refs/heads/master@{#387517}