[Tracing] Introduce HeapDumpWriter helper class

[Tracing] Introduce HeapDumpWriter helper class

The heap profiler in chrome://tracing keeps track of allocations in an
|AllocationRegister|. When a heap dump is requested, a summary of the
allocation register should be written to the trace log. (It is not
possible to dump every single allocation, some processes have peeks as
high as 400k outstanding allocations.) The |HeapDumpWriter| class
analyzes all allocations and writes a condensed heap dump to the trace
log.

At the moment the logic is very simple. All allocations are grouped by
backtrace, and the number of bytes allocated per backtrace is written.
In the future this will be extended with the ability to group by
different pieces of context. Furthermore, a smarter algorithm can be
introduced that writes as much information as possible given a fixed
budget of trace log space.

The heap dump writer is designed to be independent of the
|StackFrameDeduplicator|. There could be a deduplicator per dump
provider, or a global deduplicator. By taking it as an argument,
the dumper works either way.

BUG=524631
Committed: https://crrev.com/51d9e66be7968f1b8844f44effe066708a0536d6
Cr-Commit-Position: refs/heads/master@{#356282}

[Ruud van Asseldonk, 2015-10-20 15:57:54]

ruuda@google.com changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-10-22 14:41:01]

I think this patch needs a rebase?
I had deja-vus while reviewing this, in fact some changes that I see here in the
diff are already live in trunk.

[Ruud van Asseldonk, 2015-10-22 14:49:08]

On 2015/10/22 14:41:01, Primiano Tucci wrote:
> I think this patch needs a rebase?
> I had deja-vus while reviewing this, in fact some changes that I see here in
the
> diff are already live in trunk.

I moved a few things around for more logical ordering in the files.
(|StackFrameDeduplicator| was inserted right between some methods of
|AllocationContextTracker|.)

[Ruud van Asseldonk, 2015-10-22 14:51:03]

On 2015/10/22 14:49:08, Ruud van Asseldonk wrote:
> On 2015/10/22 14:41:01, Primiano Tucci wrote:
> > I think this patch needs a rebase?
> > I had deja-vus while reviewing this, in fact some changes that I see here in
> the
> > diff are already live in trunk.
> 
> I moved a few things around for more logical ordering in the files.
> (|StackFrameDeduplicator| was inserted right between some methods of
> |AllocationContextTracker|.)

And I made |Backtrace| a first-class citizen, so that's why it touches some
other files as well. Better do this now before usage is more widespread.

[Primiano Tucci (use gerrit), 2015-10-26 11:52:07]

Awesome. The more I review your code the more I see all the pieces coming
together.
Thanks for all the hard work, some comments.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:32: return
SuperFastHash(reinterpret_cast<const char*>(backtrace.frames),
be really careful with SuperFastHash. I got bitten in the past* as it is easy to
collide if two stacks differ only for the last 4 chars, which might be the case
here. If unsure use sha1 and we can always use a faster hash later if we hit
performance problems.

* see [chromium-dev] base::Hash(), how robust is it supposed to be

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:72: struct BASE_EXPORT
Backtrace {
not really sure this is more readable than before. I'd probably keep that inside
the AllocationContext

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:83: class BASE_EXPORT
BacktraceHash {
+er (Hasher)?

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:83: class BASE_EXPORT
BacktraceHash {
A search in the codebase suggests that the way we usually deal with this is by
defining at the end of the file a specialization of hash in the std namespace.
See
https://code.google.com/p/chromium/codesearch#chromium/src/cc/surfaces/surfac...
as an example

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context_unittest.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context_unittest.cc:234: Backtrace
bt = {
If the problem is breaking lines here you can just add a "using" on top of this
file

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_heap_dumper.h (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
I'd probably pick a stronger name for this, which explains what it does.
HeapDumper IMHO is a bit generic and might sound as "something which collects
and dumps the heap", while this is specifically for the dumping part.
How about HeapDumpSeralizer or something like that?

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:30:
HeapDumper(scoped_refptr<TracedValue> traced_value,
This feels a bit an unnatural pattern: you take an empty TracedValue at ctor
time, and you don't do anything with that until you call the WriteHeapDump
method.
Since TracedValue can be created everywhere and are easily movable, shouldn't
you don't take any traced value and either:
- Create one in the WriteHeapDump function and return it out.
- Create one as a private field in this class and return it from WriteHeapDump()
?

The current signatures are a bit cryptic

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:31: StackFrameDeduplicator*
sf_deduplicator);
either deduplicator or stack_frame_deduplicator

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:37: void Fill(const
AllocationRegister& allocation_register);
As discussed offline would be clearer to expose only a AddAllocation(Context,
size) method here and do the loop outside.
In this way you make clear that this class is orthogonal to the fact that the
allocator uses or not the AllocationRegister

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:51: // Helper for generating the
|stackFrames| dictionary. Borrowed, not owned.
Borrowed doesn't make clear if you plan to transfer ownership (in which case you
should use a scoped_ptr or not).
I'd say "Not owned. the caller is resposible to keep the SFD instance alive for
the lifetime of this class"

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:55: hash_map<Backtrace, size_t,
BacktraceHash> acc_by_backtrace_;
what about bytes_by_backtrace_?

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:59: std::string buffer_;
smart, I see what you are doing here  ;-)

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:149: heap_dumps_[absolute_name] =
heap_dump;
add a DCHECK_EQ(0u, heap_dumps_.count(absolute_name)) ?

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:215: for (auto name_and_dump :
heap_dumps_)
const auto& ?

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:20: #include
"base/trace_event/trace_event_argument.h"
can't TracedValue be fwd declared here?

[Ruud van Asseldonk, 2015-10-26 14:51:27]

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:32: return
SuperFastHash(reinterpret_cast<const char*>(backtrace.frames),
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> be really careful with SuperFastHash. I got bitten in the past* as it is easy
to
> collide if two stacks differ only for the last 4 chars, which might be the
case
> here. If unsure use sha1 and we can always use a faster hash later if we hit
> performance problems.
> 
> * see [chromium-dev] base::Hash(), how robust is it supposed to be

I saw that discussion. Insertion in the hash table is done while a lock for the
allocation register is held. All allocations from all threads are blocked on
this, so performance is critical here. I have not benchmarked SHA1 against
SuperFastHash, but I did compare |std::map| and |base::hash_map|, where for map
the comparison is a |memcmp| of the backtrace. The result was that a hash table
is about 30% faster. Do you think it is worthwhile to investigate performance of
different hashes?

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:72: struct BASE_EXPORT
Backtrace {
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> not really sure this is more readable than before. I'd probably keep that
inside
> the AllocationContext

Going to leave it outside for now to avoid rightward drift.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:83: class BASE_EXPORT
BacktraceHash {
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> +er (Hasher)?

Would be called |hash<Backtrace>| then.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.h:83: class BASE_EXPORT
BacktraceHash {
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> A search in the codebase suggests that the way we usually deal with this is by
> defining at the end of the file a specialization of hash in the std namespace.
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/surfaces/surfac...
> as an example

Done.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context_unittest.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context_unittest.cc:234: Backtrace
bt = {
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> If the problem is breaking lines here you can just add a "using" on top of
this
> file

That is what I did at first but then I though I might as well extract the class
instead of adding the using everywhere.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_heap_dumper.h (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> I'd probably pick a stronger name for this, which explains what it does.
> HeapDumper IMHO is a bit generic and might sound as "something which collects
> and dumps the heap", while this is specifically for the dumping part.
> How about HeapDumpSeralizer or something like that?

Renamed to |HeapDumpWriter|.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:30:
HeapDumper(scoped_refptr<TracedValue> traced_value,
On 2015/10/26 11:52:07, Primiano Tucci wrote:
> This feels a bit an unnatural pattern: you take an empty TracedValue at ctor
> time, and you don't do anything with that until you call the WriteHeapDump
> method.
> Since TracedValue can be created everywhere and are easily movable, shouldn't
> you don't take any traced value and either:
> - Create one in the WriteHeapDump function and return it out.
> - Create one as a private field in this class and return it from
WriteHeapDump()
> ?
> 
> The current signatures are a bit cryptic

Made it a member instead, and return it from |WriteHeapDump|. (It is nice to
have a member for |WriteSize|.)

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:31: StackFrameDeduplicator*
sf_deduplicator);
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> either deduplicator or stack_frame_deduplicator

Done.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:37: void Fill(const
AllocationRegister& allocation_register);
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> As discussed offline would be clearer to expose only a AddAllocation(Context,
> size) method here and do the loop outside.
> In this way you make clear that this class is orthogonal to the fact that the
> allocator uses or not the AllocationRegister

I agree that would be much cleaner. Done.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:51: // Helper for generating the
|stackFrames| dictionary. Borrowed, not owned.
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> Borrowed doesn't make clear if you plan to transfer ownership (in which case
you
> should use a scoped_ptr or not).
> I'd say "Not owned. the caller is resposible to keep the SFD instance alive
for
> the lifetime of this class"

Done.

I don't know about you, but to me "borrowing" implies the intention to return it
:) So that would mean no transfer of ownership. (In Rust a borrowed pointer is a
thing.)

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:55: hash_map<Backtrace, size_t,
BacktraceHash> acc_by_backtrace_;
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> what about bytes_by_backtrace_?

If you wish.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_heap_dumper.h:59: std::string buffer_;
On 2015/10/26 11:52:06, Primiano Tucci wrote:
> smart, I see what you are doing here  ;-)

The credit is yours, I stole this from
|MemoryAllocatorDump::string_conversion_buffer_| :)

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:149: heap_dumps_[absolute_name] =
heap_dump;
On 2015/10/26 11:52:07, Primiano Tucci wrote:
> add a DCHECK_EQ(0u, heap_dumps_.count(absolute_name)) ?

Done.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:215: for (auto name_and_dump :
heap_dumps_)
On 2015/10/26 11:52:07, Primiano Tucci wrote:
> const auto& ?

Done.

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:20: #include
"base/trace_event/trace_event_argument.h"
On 2015/10/26 11:52:07, Primiano Tucci wrote:
> can't TracedValue be fwd declared here?

Yes it can. Fixed.

[Ruud van Asseldonk, 2015-10-26 20:30:19]

Description was changed from

==========
[Tracing] Introduce HeapDumper helper class

The heap profiler in chrome://tracing keeps track of allocations in an
|AllocationRegister|. When a heap dump is requested, a summary of the
allocation register should be written to the trace log. (It is not
possible to dump every single allocation, some processes have peeks as
high as 400k outstanding allocations.) The |HeapDumper| class analyzes
all allocations and writes a condensed heap dump to the trace log.

At the moment the logic is very simple. All allocations are grouped by
backtrace, and the number of bytes allocated per backtrace is written.
In the future this will be extended with the ability to group by
different pieces of context. Furthermore, a smarter algorithm can be
introduced that writes as much information as possible given a fixed
budget of trace log space.

The heap dumper is designed to be independent of the
|StackFrameDeduplicator|. There could be a deduplicator per dump
provider, or a global deduplicator. By taking it as an argument, the
dumper works either way.

BUG=524631
==========

to

==========
[Tracing] Introduce HeapDumpWriter helper class

The heap profiler in chrome://tracing keeps track of allocations in an
|AllocationRegister|. When a heap dump is requested, a summary of the
allocation register should be written to the trace log. (It is not
possible to dump every single allocation, some processes have peeks as
high as 400k outstanding allocations.) The |HeapDumpWriter| class
analyzes all allocations and writes a condensed heap dump to the trace
log.

At the moment the logic is very simple. All allocations are grouped by
backtrace, and the number of bytes allocated per backtrace is written.
In the future this will be extended with the ability to group by
different pieces of context. Furthermore, a smarter algorithm can be
introduced that writes as much information as possible given a fixed
budget of trace log space.

The heap dump writer is designed to be independent of the
|StackFrameDeduplicator|. There could be a deduplicator per dump
provider, or a global deduplicator. By taking it as an argument,
the dumper works either way.

BUG=524631
==========

[Primiano Tucci (use gerrit), 2015-10-26 20:57:57]

LGTM with comments addressed

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1419633004/diff/80001/base/trace_event/memory...
base/trace_event/memory_profiler_allocation_context.cc:32: return
SuperFastHash(reinterpret_cast<const char*>(backtrace.frames),
On 2015/10/26 14:51:26, Ruud van Asseldonk wrote:
> On 2015/10/26 11:52:06, Primiano Tucci wrote:
> > be really careful with SuperFastHash. I got bitten in the past* as it is
easy
> to
> > collide if two stacks differ only for the last 4 chars, which might be the
> case
> > here. If unsure use sha1 and we can always use a faster hash later if we hit
> > performance problems.
> > 
> > * see [chromium-dev] base::Hash(), how robust is it supposed to be
> 
> I saw that discussion. Insertion in the hash table is done while a lock for
the
> allocation register is held. All allocations from all threads are blocked on
> this, so performance is critical here. I have not benchmarked SHA1 against
> SuperFastHash, but I did compare |std::map| and |base::hash_map|, where for
map
> the comparison is a |memcmp| of the backtrace. The result was that a hash
table
> is about 30% faster. Do you think it is worthwhile to investigate performance
of
> different hashes?

Ok, didn't realize that after all, if you end up having some collision you just
have a slightly slower path. In my case it was different as I was expecting to
now have collisions.
So, even if SuperFastHash should suck systematically, everything will always
work (just maybe a bit slower)

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:83: class BASE_EXPORT
BacktraceHash {
I think you forgot to remove this.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:215: using result_type =
uint32_t;
why these levels of indirection using "using" ?
Can you inline them below? Is it just for wrapping?

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.cc:34:
HeapDumpWriter::~HeapDumpWriter() {}
add a blank line before this

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.cc:63: for (auto bt_sz =
sorted_by_backtrace.begin();
I typically see this as "auto it" in the codebase, to give a hint about the fact
that this ia a iterator.
bt_sz is a bit cryptic :)

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.cc:69:
traced_value_->SetInteger("bt", bt_index);
were you not planning to make the indexes strings instead of integers?
I.e. shouldn't this be SetString?

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_heap_dump_writer.h (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:30: // unique access to it
during the lifetime of the dump writer.
s/unique/exclusive/

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:34: // Inserts information
from which the heap dump will be generated. This method
The first period is pleonastic and doesn't tell anything more than the method
signature itself.
Just remove it and keep the rest.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:39: // Aggregates
allocations and writes a "heap" array to a traced value.
add: See goo.gl/XXXX for the actual format

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:216:
value->BeginDictionary("heap_dumps");
I thought you were going to call this "heaps" ?

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:218:
value->SetValueWithCopiedName(name_and_dump.first, *name_and_dump.second);
and have a "entries" here?

[Primiano Tucci (use gerrit), 2015-10-26 20:58:11]

Ah also, memory_profiler_allocation_context.h will need some cleanup at some
point.

[Ruud van Asseldonk, 2015-10-27 10:50:18]

Addressed comments. Can you please have a look at the changes in
|HeapDumpWriter| Primiano?

On 2015/10/26 20:58:11, Primiano Tucci wrote:
> Ah also, memory_profiler_allocation_context.h will need some cleanup at some
> point.

I agree. Let's do a refactoring round after we land V1 of the heap profiler
(just a few CLs left).

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:83: class BASE_EXPORT
BacktraceHash {
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> I think you forgot to remove this.

Fixed.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:215: using result_type =
uint32_t;
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> why these levels of indirection using "using" ?
> Can you inline them below? Is it just for wrapping?

This was for symmetry with |std::hash|, but I violated the contract already by
using |uint32_t| instead of |size_t|, and everything appears to work fine
without the usings. Removed.

This did made me think though, I think the return value gets converted to
|size_t| in the hash map. This could have undesired side effects. We could drop
the |SuperFastHash| and do something like here:

https://chromium.googlesource.com/chromium/src/+/6918fc7520a84/base/container...

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_heap_dump_writer.cc (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.cc:34:
HeapDumpWriter::~HeapDumpWriter() {}
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> add a blank line before this

Done.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.cc:63: for (auto bt_sz =
sorted_by_backtrace.begin();
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> I typically see this as "auto it" in the codebase, to give a hint about the
fact
> that this ia a iterator.
> bt_sz is a bit cryptic :)

Done. I called it |bt_sz| because iterator dereferences to a (backtrace, size)
pair, though I admit that "bt" and "sz" isn't really helpful if you don't know
that.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.cc:69:
traced_value_->SetInteger("bt", bt_index);
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> were you not planning to make the indexes strings instead of integers?
> I.e. shouldn't this be SetString?

Yes, I didn't update this one yet. Fixed.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
File base/trace_event/memory_profiler_heap_dump_writer.h (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:30: // unique access to it
during the lifetime of the dump writer.
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> s/unique/exclusive/

Done.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:34: // Inserts information
from which the heap dump will be generated. This method
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> The first period is pleonastic and doesn't tell anything more than the method
> signature itself.
> Just remove it and keep the rest. 

Done.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:39: // Aggregates
allocations and writes a "heap" array to a traced value.
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> add: See goo.gl/XXXX for the actual format

We don't have a document for this yet, except the draft in the presentation.
Will add a section to the design doc and reference here.

Edit: https://goo.gl/jYN4Zn

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:216:
value->BeginDictionary("heap_dumps");
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> I thought you were going to call this "heaps" ?

See patchset 7.

https://codereview.chromium.org/1419633004/diff/100001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:218:
value->SetValueWithCopiedName(name_and_dump.first, *name_and_dump.second);
On 2015/10/26 20:57:57, Primiano Tucci wrote:
> and have a "entries" here?

See patchset 7. (Entries are written by the |HeapDumpWriter|.)

[Primiano Tucci (use gerrit), 2015-10-27 11:56:20]

LGTM with some final nits.
Would be nice to cover these new changes to ProcessMemoryDump with a test, but
you can do that in a follow up CL once all the pieces land.

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.cc:76: void
DestructAllocationContextTracker(void* alloc_ctx_tracker) {
This should be moved perhaps in the anonymous namespace

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:126: // The allocation
context is context metadata that is kept for every allocation
s/allocation context/AllocationContext/

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:207: uint32_t
operator()(const base::trace_event::Backtrace& backtrace) const;
I think you should return size_t here and just let the cast in the
implementation pad it, but not expose the 32-vs-size_t hole outside.

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
File base/trace_event/memory_profiler_heap_dump_writer.h (right):

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:15: #include
"base/trace_event/trace_event_argument.h"
Forward declare this if you can please, trace_event_argument can be a
problematic header.

[Ruud van Asseldonk, 2015-10-27 11:59:31]

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.cc (right):

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.cc:76: void
DestructAllocationContextTracker(void* alloc_ctx_tracker) {
On 2015/10/27 11:56:20, Primiano Tucci wrote:
> This should be moved perhaps in the anonymous namespace

Can do when refactoring, other CLs touch this piece too and want to avoid
conflicts.

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
File base/trace_event/memory_profiler_allocation_context.h (right):

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:126: // The allocation
context is context metadata that is kept for every allocation
On 2015/10/27 11:56:20, Primiano Tucci wrote:
> s/allocation context/AllocationContext/

Done.

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_allocation_context.h:207: uint32_t
operator()(const base::trace_event::Backtrace& backtrace) const;
On 2015/10/27 11:56:20, Primiano Tucci wrote:
> I think you should return size_t here and just let the cast in the
> implementation pad it, but not expose the 32-vs-size_t hole outside.

Done.

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
File base/trace_event/memory_profiler_heap_dump_writer.h (right):

https://codereview.chromium.org/1419633004/diff/140001/base/trace_event/memor...
base/trace_event/memory_profiler_heap_dump_writer.h:15: #include
"base/trace_event/trace_event_argument.h"
On 2015/10/27 11:56:20, Primiano Tucci wrote:
> Forward declare this if you can please, trace_event_argument can be a
> problematic header.

Done.

[Ruud van Asseldonk, 2015-10-27 12:00:20]

The CQ bit was checked by ruuda@google.com

[Ruud van Asseldonk, 2015-10-27 12:00:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1419633004/#ps160001
(title: "Address nits")

[commit-bot: I haz the power, 2015-10-27 12:00:36]

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

[commit-bot: I haz the power, 2015-10-27 13:20:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-27 13:21:30]

Patchset 9 (id:??) landed as
https://crrev.com/51d9e66be7968f1b8844f44effe066708a0536d6
Cr-Commit-Position: refs/heads/master@{#356282}