[tracing] Switch to new heap dump format.

[tracing] Switch to new heap dump format.

This CL switches tracing to a new heap dump format, that offers the
following advantages:

1. Dumps include all the information collected by Chrome's heap profiler.

2. The format is simpler and more compact.

3. The format can easily be extended to include additional per-entry data.

4. The format allows for post-processing (see recategorization from
   crrev.com/2906413002 as an example).


BUG=708930
Review-Url: https://codereview.chromium.org/2650863003
Cr-Commit-Position: refs/heads/master@{#480580}
Committed: https://chromium.googlesource.com/chromium/src/+/d4a5e98235da0cc8c4fb1ae2f67826b0b480ce4c

[DmitrySkiba, 2017-01-30 19:44:10]

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

[lpy, 2017-01-30 23:36:36]

Description was changed from

==========
New heap dump format.

TODO: unittests

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

to

==========
New heap dump format.

TODO: unittests

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

[DmitrySkiba, 2017-02-14 18:31:15]

PTAL

[Primiano Tucci (use gerrit), 2017-02-15 18:55:02]

On 2017/02/14 18:31:15, DmitrySkiba wrote:
> PTAL

sorry for the delay, manage to do *almost* all my codereviews today, % this one.
will get to this as first thing tomorrow morning.

[Primiano Tucci (use gerrit), 2017-02-17 17:07:05]

Thanks  a lot for this. I'm very excited by the code being ditched here.
Everything looks great, I have only some minor comments.

The other doubt that I have is that the incremental approach, right now, will
break the heap profiler in ring buffer mode.
At least we shoudl add a CHECK somewhere saying: "sorry, cant use heap profiler
in ring buffer mode"

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

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:44: hash_map<AggregationKey,
AllocationMetrics, AggregationKey::Hasher>
note: at some point we should fix the deduplicators as per discussion in
crbug.com/686208 .
I'd do as a separate CL to keep this easier to review. I think the only think we
need is a further hashtable in each deduplicator that maps (const char*) ->
(string hash). That will still be extremely fast and deal with string identity.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:46: for (const auto&
allocation : allocation_register) {
would be very nice if here there was a comment with a rough summary of the heap
dump format and a link to the doc.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:64: for (const auto&
key_and_metrics : metrics_by_key) {
I think the rest of the code in this folder doesn't have braces when unnecessary
(e.g. single line for-loops) I find it helps to keep files more compact and
easier to glance and scroll

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

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_stack_frame_deduplicator.cc:99:
traced_value->SetInteger("parent", frame_node.parent_frame_index);
do we have the guarantee that the parent is always emitted (or has been emitted
by the previous snapshots)?

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

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_type_name_deduplicator.h:41:
std::vector<TypeMap::const_iterator> new_type_ids_;
smart ;-)

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:725:
heap_dump_value->SetInteger("version", 1);
I think that historically we avoided any sort of versioning in tracing. The deal
is that when you change something is either:
- incremental changes that can still be parsed by the old UI
- you change the name of the key so that old version of the UI will ignore this
completely.

The problem with the version approach, is that we definitely run into cases
where the UI is older than the trace format. So the day that you will introduce
version "2" here, there will be people out there running a version of the UI
that doesn't know about the existance of version 2 and will still try to parse
this as version 1.
As such let's remove the versioning  and keep using the same pattern

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:753:
TraceLog::GetCategoryGroupEnabled(kTraceCategory), "heap_profile",
this is the only thing where I'm not fully sure. Why do we want to emit another
dump type if, at the end, this is generated synchronously with the memory-infra
dump and there are dependencies between the two?
I don't think this will make that much of a difference in terms of code reusing
in the importer.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1293: class HeapDumpProvider :
public MemoryDumpProvider {
shouldn't these be in the *heap_profiler_something*unittest ?

[DmitrySkiba, 2017-02-23 07:17:18]

dskiba@chromium.org changed reviewers:
+ hjd@chromium.org

[DmitrySkiba, 2017-02-23 07:17:19]

Overall things TODO:

1. Tests
2. Optimizing format for size
3. Public format doc (use/extend Hector's, or add a section to the current one?)

hjd@: Please comment on versioning and separate 'P' events.

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

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:44: hash_map<AggregationKey,
AllocationMetrics, AggregationKey::Hasher>
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> note: at some point we should fix the deduplicators as per discussion in
> crbug.com/686208 .
> I'd do as a separate CL to keep this easier to review. I think the only think
we
> need is a further hashtable in each deduplicator that maps (const char*) ->
> (string hash). That will still be extremely fast and deal with string
identity. 

Acknowledged.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:46: for (const auto&
allocation : allocation_register) {
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> would be very nice if here there was a comment with a rough summary of the
heap
> dump format and a link to the doc.

Hmm, we too many docs about new format. Maybe let's just modify the old one?

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_heap_dump_writer.cc:64: for (const auto&
key_and_metrics : metrics_by_key) {
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> I think the rest of the code in this folder doesn't have braces when
unnecessary
> (e.g. single line for-loops) I find it helps to keep files more compact and
> easier to glance and scroll

Done.

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

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/heap_p...
base/trace_event/heap_profiler_stack_frame_deduplicator.cc:99:
traced_value->SetInteger("parent", frame_node.parent_frame_index);
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> do we have the guarantee that the parent is always emitted (or has been
emitted
> by the previous snapshots)?

Yes, because we dump nodes_ sequentially, and in there parents always precede
children.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:725:
heap_dump_value->SetInteger("version", 1);
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> I think that historically we avoided any sort of versioning in tracing. The
deal
> is that when you change something is either:
> - incremental changes that can still be parsed by the old UI
> - you change the name of the key so that old version of the UI will ignore
this
> completely.
> 
> The problem with the version approach, is that we definitely run into cases
> where the UI is older than the trace format. So the day that you will
introduce
> version "2" here, there will be people out there running a version of the UI
> that doesn't know about the existance of version 2 and will still try to parse
> this as version 1.
> As such let's remove the versioning  and keep using the same pattern

My position is exactly the opposite - versioning is better all around:

1. With versioning old UI can gracefully handle new traces by explaining why
certain features are missing, and how to fix that (upgrade to the latest
version). With key renaming those features will just be missing, without any
explanation.

2. Versioning means code can be sure certain keys will be there. With key
renaming code had to be written defensively, with fallback paths in mind.

Are there any drawbacks to having a version field?

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:753:
TraceLog::GetCategoryGroupEnabled(kTraceCategory), "heap_profile",
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> this is the only thing where I'm not fully sure. Why do we want to emit
another
> dump type if, at the end, this is generated synchronously with the
memory-infra
> dump and there are dependencies between the two?
> I don't think this will make that much of a difference in terms of code
reusing
> in the importer.

I think we discussed this exact concern several times. Last times Hector said
that this would simplify things.

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1293: class HeapDumpProvider :
public MemoryDumpProvider {
On 2017/02/17 17:07:05, Primiano Tucci wrote:
> shouldn't these be in the *heap_profiler_something*unittest ?

This needs to be removed, it's a code I used during the implementation. I also
need to fix all other tests / write new ones.

[lpy, 2017-02-23 07:23:42]

On 2017/02/23 07:17:19, DmitrySkiba wrote:
> 3. Public format doc (use/extend Hector's, or add a section to the current
one?)

For public doc, I would recommend to check our trace event format doc here:
https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...

And follow the convention, and when we have the new doc, we mark the old format
as deprecated and link the new format in it.

[lpy, 2017-02-23 07:24:59]

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

[hjd, 2017-02-23 12:33:28]

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:725:
heap_dump_value->SetInteger("version", 1);
On 2017/02/23 07:17:19, DmitrySkiba wrote:
> On 2017/02/17 17:07:05, Primiano Tucci wrote:
> > I think that historically we avoided any sort of versioning in tracing. The
> deal
> > is that when you change something is either:
> > - incremental changes that can still be parsed by the old UI
> > - you change the name of the key so that old version of the UI will ignore
> this
> > completely.
> > 
> > The problem with the version approach, is that we definitely run into cases
> > where the UI is older than the trace format. So the day that you will
> introduce
> > version "2" here, there will be people out there running a version of the UI
> > that doesn't know about the existance of version 2 and will still try to
parse
> > this as version 1.
> > As such let's remove the versioning  and keep using the same pattern
> 
> My position is exactly the opposite - versioning is better all around:
> 
> 1. With versioning old UI can gracefully handle new traces by explaining why
> certain features are missing, and how to fix that (upgrade to the latest
> version). With key renaming those features will just be missing, without any
> explanation.
> 
> 2. Versioning means code can be sure certain keys will be there. With key
> renaming code had to be written defensively, with fallback paths in mind.
> 
> Are there any drawbacks to having a version field?

I was motivated to ask Dmitry to add the version key to make keeping backwards
compatibility simpler. We already support two older versions of the format and
the code for that is a little gnarly [1-3]. I hoped that adding an explicit
version key would remove some of the guesswork (e.g. "key x is present but key y
is missing so I guess this is the middle format when we had type info but still
used sum totals?"). The design of the reader hopefully makes it trivial to be
backwards compatible forever.

Like Dmitry I think that when we make breaking changes it is better for the UI
to call this out rather than to silently ignore them. However I hadn't really
considered forward compatibility (incremental changes that can still be parsed
by the old UI). With a version field forward compatible changes would still be
possible (to make them explicit we could even have semvar style version numbers
"[breakingVersion].[compatibleVersion]") but it's true that having a version
field encourages people to make breaking changes where before they might have
made forwards compatible ones.

I'm sensitive to the fact that this would be the first versioned thing in the
tracing format - but I think if we're ever going to version any part of the
trace format it would be this - the thing we're versioning is basically a wire
format, it has no semantic content, it's only describing a transformation from a
struct-of-arrays to an array-of-structs. Significantly we can modify the
'logical' heap dump format without bumping this version number but we can also
optimise how we send that data (by bumping the version number) without changing
the logical format as understood by the heap dump specific part of the importer.

...that's my 2c anyway ;P

[1]:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/e...
[2]:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/e...
[3]:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/e...

https://codereview.chromium.org/2650863003/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:753:
TraceLog::GetCategoryGroupEnabled(kTraceCategory), "heap_profile",
On 2017/02/23 07:17:19, DmitrySkiba wrote:
> On 2017/02/17 17:07:05, Primiano Tucci wrote:
> > this is the only thing where I'm not fully sure. Why do we want to emit
> another
> > dump type if, at the end, this is generated synchronously with the
> memory-infra
> > dump and there are dependencies between the two?
> > I don't think this will make that much of a difference in terms of code
> reusing
> > in the importer.
> 
> I think we discussed this exact concern several times. Last times Hector said
> that this would simplify things.

As I mentioned before if we eventually want to share stack trace/type info with
V8 the a separate event is slightly neater in the tracing code (there would be a
single path for adding new stack trace/type info as opposed to two). It is also
a bit logically cleaner (a new event type as opposed to a new pseudo-event thing
which is stuffed into args). It isn't worth fighting over though, at least on
the importer side it is pretty easy to either.

[DmitrySkiba, 2017-03-01 08:20:48]

PTAL

Changes at a glance:

1. Added StringDeduplicator, and made both StackFrameDeduplicator /
TypeNameDeduplicator depend on it (and use it during export).

2. Moved everything format-related in one file, heap_profiler_event_writer.h
(former heap_profiler_heap_dump_writer.h). This also helped with unit-testing.

Current format is briefly described in heap_profiler_event_writer.h

hjd@ please check heap_profiler_event_writer.h for the current state of the
format. Can you quickly hack a change to parse it? Tomorrow (my time) I'm going
to work on symbolization. Also note that individual heap dumps are under
"allocators" node (data/allocators/malloc instead of data/malloc).

Still TODO:

1. unittests for deduplicators. They are mostly done, I just need to clean them
up. I'll submit tomorrow.
2. Documentation. I want to have something minimal in
heap_profiler_event_writer.h, and more expanded version somewhere in a doc.
3. Ring buffer mode. I'll add a CHECK() in this CL, but better way to solve it
(in a follow-up CL?) is to dump deduplicators non-incrementally if we're in the
ring mode.
4. HeapProfiler::breakdown_threshold_bytes needs to go. Probably in a follow-up
CL though.

[DmitrySkiba, 2017-03-02 19:56:55]

On 2017/03/01 08:20:48, DmitrySkiba wrote:
> PTAL
> 
> Changes at a glance:
> 
> 1. Added StringDeduplicator, and made both StackFrameDeduplicator /
> TypeNameDeduplicator depend on it (and use it during export).
> 
> 2. Moved everything format-related in one file, heap_profiler_event_writer.h
> (former heap_profiler_heap_dump_writer.h). This also helped with unit-testing.
> 
> Current format is briefly described in heap_profiler_event_writer.h
> 
> hjd@ please check heap_profiler_event_writer.h for the current state of the
> format. Can you quickly hack a change to parse it? Tomorrow (my time) I'm
going
> to work on symbolization. Also note that individual heap dumps are under
> "allocators" node (data/allocators/malloc instead of data/malloc).
> 
> Still TODO:
> 
> 1. unittests for deduplicators. They are mostly done, I just need to clean
them
> up. I'll submit tomorrow.
> 2. Documentation. I want to have something minimal in
> heap_profiler_event_writer.h, and more expanded version somewhere in a doc.
> 3. Ring buffer mode. I'll add a CHECK() in this CL, but better way to solve it
> (in a follow-up CL?) is to dump deduplicators non-incrementally if we're in
the
> ring mode.
> 4. HeapProfiler::breakdown_threshold_bytes needs to go. Probably in a
follow-up
> CL though.

Guys, PTAL.

[hjd, 2017-03-02 19:58:43]

On 2017/03/02 19:56:55, DmitrySkiba wrote:
> On 2017/03/01 08:20:48, DmitrySkiba wrote:
> > PTAL
> > 
> > Changes at a glance:
> > 
> > 1. Added StringDeduplicator, and made both StackFrameDeduplicator /
> > TypeNameDeduplicator depend on it (and use it during export).
> > 
> > 2. Moved everything format-related in one file, heap_profiler_event_writer.h
> > (former heap_profiler_heap_dump_writer.h). This also helped with
unit-testing.
> > 
> > Current format is briefly described in heap_profiler_event_writer.h
> > 
> > hjd@ please check heap_profiler_event_writer.h for the current state of the
> > format. Can you quickly hack a change to parse it? Tomorrow (my time) I'm
> going
> > to work on symbolization. Also note that individual heap dumps are under
> > "allocators" node (data/allocators/malloc instead of data/malloc).
> > 
> > Still TODO:
> > 
> > 1. unittests for deduplicators. They are mostly done, I just need to clean
> them
> > up. I'll submit tomorrow.
> > 2. Documentation. I want to have something minimal in
> > heap_profiler_event_writer.h, and more expanded version somewhere in a doc.
> > 3. Ring buffer mode. I'll add a CHECK() in this CL, but better way to solve
it
> > (in a follow-up CL?) is to dump deduplicators non-incrementally if we're in
> the
> > ring mode.
> > 4. HeapProfiler::breakdown_threshold_bytes needs to go. Probably in a
> follow-up
> > CL though.
> 
> Guys, PTAL.

I'm updating the tracing side patch.

[DmitrySkiba, 2017-03-07 22:45:09]

PTAL.

I added unittests and DCHECK for continuous mode.

[Primiano Tucci (use gerrit), 2017-03-09 11:47:42]

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

[Primiano Tucci (use gerrit), 2017-03-09 11:47:45]

Thanks! I looked through all the code (a bit less tests) and it looks all great.
My only comments are:
- naming of the Export... functions (see comments inline)
- IMHO the heap dump should still be part of the memory-infra dump and not a
standalone event. Maybe we discussed thatin some other email thread and I lost
track of that?

For all the rest I only have very minor comments. This patch looks great. Thanks
a lot for all the hard work and your patience.
Once the #if 0 in the tests are gone and the UI is landed, this can land from my
viewpoint.

Maybe in the meantime, if +ssid has some spare cycles, might be good to have a
second eye on this. I don't have any design/architectural concerns, but given
the size of the patch it might be we both missed some silly thing.

https://codereview.chromium.org/2650863003/diff/120001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2650863003/diff/120001/base/BUILD.gn#newcode981
base/BUILD.gn:981: "trace_event/heap_profiler_string_deduplicator.cc",
remember to update also the gn bootstrap.py

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
Stop living in the past :P (2015 -> 2017)

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:11: #include
"base/containers/hash_tables.h"
I think this is really just for legacy code, this header internally says:

// Use std::unordered_map instead.
etc

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:27: struct AggregationKey {
I think you can just:
struct AggregationKey : public std::pair<int,int> {
  using Hasher = base::IntPairHash<int>;
  int backtrace_id() const { return this->first; }
  int type_id() const { return this->second; }
}

and halve the lines here

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:48: hash_map<AggregationKey,
AllocationMetrics, AggregationKey::Hasher>
The comment on hash_map says // Use std::unordered_map instead.
I think hash_map is a pre-c++11 thing only

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:80:
saturated_cast<int>(key_and_metrics.second.count));
nice touch of class :)

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2017 :)

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:17: Heap profile event data
format.
this comment is really awesome. thanks a lot, really helped the review.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:24: 1. Call ExportHeapDump() on
allocation registers and accumulate results
only have one small comment about naming: you have two "Export" functions here,
but only one really does the export, because, if I am understanding correctly,
they work kinda in a pipeline (i.e. this ExportHeapDump "exports" only to the
next ExportHeapProfileEventData().
How about calling this: SummarizeHeapDump() or AggregateHeapDump() and the next
one SerializeHeapProfiler{EventData,Dump,Snapshot} ?

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:27: contained in
MemoryDumpSessionState are used in the process.
s/in the process/in this stage/

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:45: <allocator>: {
maybe, just to clarify, I'ld s/<allocator>/[malloc, partition_alloc, blinkgc]

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:49: "sizes":  [<size1>,          
<size1>,           ...]
I guess the 2nd one is size2?

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:57: "parent": <parent_id>,
not sure if this is going to affect too much the previous discussions, so feel
free to ignore this comment.
Since we are redesigning this and there will be lot of nodes here, how about
calling the keys:
"parent" -> "p"
"name_sid" -> "s" (or "n")

that would save quite some bytes in the trace no?

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:93: using ExportedHeapDumpsMap =
yeah the more I look at this the more I think that this IMHO should be
SerializedHeapDumpsMap

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer_unittest.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.cc:36:
frames_.push_back({StackFrame::FromTraceEventName(nullptr), -1});
you say 0 above but push -1 here?

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.cc:110:
traced_value->SetInteger("name_sid", name_string_id);
see my comment above about shortening these to "n" and "p"  (or similar) to work
around JSON sillyness

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.h:52:
StackFrameDeduplicator(StringDeduplicator* string_deduplicator);
+explicit, otherwise this ctor will become too super magical :)
Also add a comment to explain that we expect the StringDeduplicator to have the
same lifetime of this class and why we need to shared this.

Initially I was about to suggest here: "why don't we have one StringDeduplicator
owned by each deduplicator?" but then realized that the IDs would collide and
would be a mess to reconcile them. So might be worth spending very few lines to
explain this as it is not immediately obvious..

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc
(right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc:36:
!dictionary->GetList("", &entries)) {
Minor thing: I think that if instead of GetList you do Remove("", &entires) you
can avoid the deepcopy and just return the value that you removed from the outer
dict.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_string_deduplicator.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.cc:23: if (!string.data()) {
should this also be + || string.empty() to reuse the key also for empty strings?
Or we just expect that to not happen frequently enough?

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.cc:31:
strings_.push_back(string.as_string());
I think that if you do
strings_.emplace_back(string.data()) here you might avoid an unnecessary string
copy here.
Alternatively, push_back(std::move(string.as_string)) but I never remember if
strings are move types and, even if they are, I wouldn't put my hand on fire
they are implemented sanely in all STL implementations :)

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_string_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.h:28: using Strings =
std::vector<std::string>;
considering that you use this only in two places, might be more readable if you
just eplicitly use std::vector in strings() and |strings_|

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_type_name_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_type_name_deduplicator.h:25:
TypeNameDeduplicator(StringDeduplicator* string_deduplicator);
+explicit

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:734: const size_t kHeapProfileNumArgs =
1;
I need to dig the email thread about this discussion. What is the reason to not
keep having the heap dumps as part of the memory-infra dump? At the end they are
emitted at the same time and the names (malloc, partition_alloc, blink_gc) must
match the other columns.
So don't feel there is too much separation here.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:805:
DCHECK(trace_config.GetTraceRecordMode() != RECORD_CONTINUOUSLY)
I think  this should be a LOG_IF(ERROR, trace_config.GetTraceRecordMode() !=
RECORD_CONTINUOUSLY) not a dcheck.
would be great (but let's keep for another patchset) if we could inject some
metadata in the trace that generates a UI message in the traceviewer importer
that says something like: "you did record a trace with heap profiling in
continuous mode but that's really not supported".
Maybe can you file a bug abobut this and cc hjd?

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:27: #if 0
uh? I think this is a leftover

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:93: #if 0
ditto

[ssid, 2017-03-13 03:38:01]

I have 2 concerns:
Does the trace size increase in pseudo mode because of string deduplicator?
Why did we decide to dump ids incrementally when we can't support some tracing
modes? Is there a plan for continuous mode?

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:122:
traced_value->BeginArray("strings");
Have you checked if we don't regress on the trace size in pseudo stack mode,
because of the string table? If we do maybe we could disable the string
deduplicator for this mode?

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2017 all files

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:58: "name_sid": <name_string_id>
Have you considered naming this just sid? name doesnt add much information here?

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.h:67: void
ExportIncrementally(TracedValue* traced_value);
Okay the code here assumes that the previous calls to ExportIncrementally() has
added the stack frames to trace. In case we are running "RECORD_CONTINUOUSLY"
then, there is a chance that the previous trace events are ignored to keep the
new trace events in buffer.
Now lets assume the user starts tracing with all default categories and
memory-infra, then within few seconds the buffer gets full and the TraceLog
starts discarding events. So, we will end up with a few memory dumps in the end,
but without any reference to the stack frames.
The idea of having the strings in metadata ensures that they will surely be
available.

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_string_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.h:31: const Strings&
strings() const { return strings_; }
this doesn't seem to be used anywhere

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

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:756: values, TRACE_EVENT_FLAG_HAS_ID);
+1 to add this to same trace event as above.

[DmitrySkiba, 2017-03-14 22:12:48]

https://codereview.chromium.org/2650863003/diff/120001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2650863003/diff/120001/base/BUILD.gn#newcode981
base/BUILD.gn:981: "trace_event/heap_profiler_string_deduplicator.cc",
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> remember to update also the gn bootstrap.py 

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> Stop living in the past :P (2015 -> 2017)

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:11: #include
"base/containers/hash_tables.h"
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> I think this is really just for legacy code, this header internally says:
> 
> // Use std::unordered_map instead.
> etc

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:27: struct AggregationKey {
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> I think you can just:
> struct AggregationKey : public std::pair<int,int> {
>   using Hasher = base::IntPairHash<int>;
>   int backtrace_id() const {&nbsp;return this->first; }
>   int type_id() const {&nbsp;return this->second; }
> }
> 
> and halve the lines here 

I started with that (using tuple), but I didn't like the fact that I had to add
a ctor with named arguments. With explicit ctor with named arguments it's hard
to correlate ctor arguments to variables. Besides, manual struct is easier to
extend with new variables.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:48: hash_map<AggregationKey,
AllocationMetrics, AggregationKey::Hasher>
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> The comment on hash_map says // Use std::unordered_map instead.
> I think hash_map is a pre-c++11 thing only

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> 2017 :)

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:24: 1. Call ExportHeapDump() on
allocation registers and accumulate results
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> only have one small comment about naming: you have two "Export" functions
here,
> but only one really does the export, because, if I am understanding correctly,
> they work kinda in a pipeline (i.e. this ExportHeapDump "exports" only to the
> next ExportHeapProfileEventData().
> How about calling this: SummarizeHeapDump() or AggregateHeapDump() and the
next
> one SerializeHeapProfiler{EventData,Dump,Snapshot} ?

Done. Going with Export -> Serialize.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:27: contained in
MemoryDumpSessionState are used in the process.
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> s/in the process/in this stage/

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:45: <allocator>: {
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> maybe, just to clarify, I'ld s/<allocator>/[malloc, partition_alloc, blinkgc]

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:49: "sizes":  [<size1>,          
<size1>,           ...]
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> I guess the 2nd one is size2?

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:57: "parent": <parent_id>,
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> not sure if this is going to affect too much the previous discussions, so feel
> free to ignore this comment.
> Since we are redesigning this and there will be lot of nodes here, how about
> calling the keys:
> "parent" -> "p"
> "name_sid" -> "s" (or "n")
> 
> that would save quite some bytes in the trace no?

Yeah, we need to bikeshed that. We can do a pass after everything else is
settled.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:93: using ExportedHeapDumpsMap =
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> yeah the more I look at this the more I think that this IMHO should be
> SerializedHeapDumpsMap

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.cc:36:
frames_.push_back({StackFrame::FromTraceEventName(nullptr), -1});
On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> you say 0 above but push -1 here? 

Oh, the joy of unnamed constants :) This -1 means 'no parent'. I replaced it
(and other occurrences) with explicit kInvalidFrameIndex constant.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.h:52:
StackFrameDeduplicator(StringDeduplicator* string_deduplicator);
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> +explicit, otherwise this ctor will become too super magical :)
> Also add a comment to explain that we expect the StringDeduplicator to have
the
> same lifetime of this class and why we need to shared this.
> 
> Initially I was about to suggest here: "why don't we have one
StringDeduplicator
> owned by each deduplicator?" but then realized that the IDs would collide and
> would be a mess to reconcile them. So might be worth spending very few lines
to
> explain this as it is not immediately obvious..

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc
(right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator_unittest.cc:36:
!dictionary->GetList("", &entries)) {
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> Minor thing: I think that if instead of GetList you do Remove("", &entires)
you
> can avoid the deepcopy and just return the value that you removed from the
outer
> dict.

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_string_deduplicator.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.cc:23: if (!string.data()) {
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> should this also be + || string.empty() to reuse the key also for empty
strings?
> Or we just expect that to not happen frequently enough?

NULL is the only value that means "no type name", so it's handled specially.
Empty string doesn't make much sense as a type name, but still, it's something,
while NULL is nothing.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.cc:31:
strings_.push_back(string.as_string());
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> I think that if you do
> strings_.emplace_back(string.data()) here you might avoid an unnecessary
string
> copy here.
> Alternatively, push_back(std::move(string.as_string)) but I never remember if
> strings are move types and, even if they are, I wouldn't put my hand on fire
> they are implemented sanely in all STL implementations :) 

push_back() has && variant, which is used in this case, so push_back() behaves
the same as emplace_back() would.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_string_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.h:28: using Strings =
std::vector<std::string>;
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> considering that you use this only in two places, might be more readable if
you
> just eplicitly use std::vector in strings() and |strings_|

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_type_name_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_type_name_deduplicator.h:25:
TypeNameDeduplicator(StringDeduplicator* string_deduplicator);
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> +explicit

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:805:
DCHECK(trace_config.GetTraceRecordMode() != RECORD_CONTINUOUSLY)
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> I think  this should be a LOG_IF(ERROR, trace_config.GetTraceRecordMode() !=
> RECORD_CONTINUOUSLY) not a dcheck.
> would be great (but let's keep for another patchset) if we could inject some
> metadata in the trace that generates a UI message in the traceviewer importer
> that says something like: "you did record a trace with heap profiling in
> continuous mode but that's really not supported".
> Maybe can you file a bug abobut this and cc hjd?

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:27: #if 0
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> uh? I think this is a leftover

Done.

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:93: #if 0
On 2017/03/09 11:47:45, Primiano Tucci (slow - perf) wrote:
> ditto

Done.

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:122:
traced_value->BeginArray("strings");
On 2017/03/13 03:38:00, ssid wrote:
> Have you checked if we don't regress on the trace size in pseudo stack mode,
> because of the string table? If we do maybe we could disable the string
> deduplicator for this mode?

String table actually improves trace size. 40s Linux trace in pseudo mode is 122
KiB smaller (1471809 -> 1345959).

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2017/03/13 03:38:00, ssid wrote:
> 2017 all files

Done.

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:58: "name_sid": <name_string_id>
On 2017/03/13 03:38:00, ssid wrote:
> Have you considered naming this just sid? name doesnt add much information
here?

I think it should be just 'name', but we can bikeshed once all other issues are
solved.

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.h:67: void
ExportIncrementally(TracedValue* traced_value);
On 2017/03/13 03:38:00, ssid wrote:
> Okay the code here assumes that the previous calls to ExportIncrementally()
has
> added the stack frames to trace. In case we are running "RECORD_CONTINUOUSLY"
> then, there is a chance that the previous trace events are ignored to keep the
> new trace events in buffer.
> Now lets assume the user starts tracing with all default categories and
> memory-infra, then within few seconds the buffer gets full and the TraceLog
> starts discarding events. So, we will end up with a few memory dumps in the
end,
> but without any reference to the stack frames.
> The idea of having the strings in metadata ensures that they will surely be
> available.

Yup, filed crbug.com/701052 for this. I don't think that heap profiling was ever
useable in continuous mode, because either trace ends up being too big (because
it has many detailed dumps), or we just die in the process of finalizing a trace
(again, because we have too many dumps).

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_string_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_string_deduplicator.h:31: const Strings&
strings() const { return strings_; }
On 2017/03/13 03:38:00, ssid wrote:
> this doesn't seem to be used anywhere

Done.

[Primiano Tucci (use gerrit), 2017-03-15 20:41:40]

Ok LGTM (plz fix the description though), so you can also get other owners
approvalz.

At the same time not-LG... I mean: don't tick commit before the UI is ready and
landed or you will cause global unhappiness in one click :)

hjd: nothing, but I just want you to feel observed :P

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.h (right):

https://codereview.chromium.org/2650863003/diff/120001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.h:57: "parent": <parent_id>,
On 2017/03/14 22:12:47, DmitrySkiba wrote:
> On 2017/03/09 11:47:44, Primiano Tucci (slow - perf) wrote:
> > not sure if this is going to affect too much the previous discussions, so
feel
> > free to ignore this comment.
> > Since we are redesigning this and there will be lot of nodes here, how about
> > calling the keys:
> > "parent" -> "p"
> > "name_sid" -> "s" (or "n")
> > 
> > that would save quite some bytes in the trace no?
> 
> Yeah, we need to bikeshed that. We can do a pass after everything else is
> settled.

Alright. Agree that's definitely not a blocker for this CL.

[ssid, 2017-03-16 02:09:20]

lgtm, please look at the comments, that should be done in all dumps.

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:122:
traced_value->BeginArray("strings");
On 2017/03/14 22:12:48, DmitrySkiba wrote:
> On 2017/03/13 03:38:00, ssid wrote:
> > Have you checked if we don't regress on the trace size in pseudo stack mode,
> > because of the string table? If we do maybe we could disable the string
> > deduplicator for this mode?
> 
> String table actually improves trace size. 40s Linux trace in pseudo mode is
122
> KiB smaller (1471809 -> 1345959).

That's awesome!

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
File base/trace_event/heap_profiler_stack_frame_deduplicator.h (right):

https://codereview.chromium.org/2650863003/diff/140001/base/trace_event/heap_...
base/trace_event/heap_profiler_stack_frame_deduplicator.h:67: void
ExportIncrementally(TracedValue* traced_value);
On 2017/03/14 22:12:48, DmitrySkiba wrote:
> On 2017/03/13 03:38:00, ssid wrote:
> > Okay the code here assumes that the previous calls to ExportIncrementally()
> has
> > added the stack frames to trace. In case we are running
"RECORD_CONTINUOUSLY"
> > then, there is a chance that the previous trace events are ignored to keep
the
> > new trace events in buffer.
> > Now lets assume the user starts tracing with all default categories and
> > memory-infra, then within few seconds the buffer gets full and the TraceLog
> > starts discarding events. So, we will end up with a few memory dumps in the
> end,
> > but without any reference to the stack frames.
> > The idea of having the strings in metadata ensures that they will surely be
> > available.
> 
> Yup, filed crbug.com/701052 for this. I don't think that heap profiling was
ever
> useable in continuous mode, because either trace ends up being too big
(because
> it has many detailed dumps), or we just die in the process of finalizing a
trace
> (again, because we have too many dumps).

Yes I think there is just one case where we can enable all categories and
memory-infra. Like the recent trace qaz@ had sent. There is a good chance that
we hit the limit on buffer there and we will still be under the size limit since
we have very few memory dumps. Agreed it's a rare case.

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/mallo...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/mallo...
base/trace_event/malloc_dump_provider.cc:290: // However, when enabled, the
overhead of the heap profiler should be always
I think this comment should be moved to process_memory_dump.cc

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

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:254:
allocation_register.EstimateTraceMemoryOverhead(&overhead);
This part should be outside the if condition which states DETAILED dump.
For all kinds of dump we want to estimate memory overehad and dump it under
malloc. See the my previous comment.

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:122: LOG(INFO) << "About to
merge";
Isn't the test logs going to help with debugging here. Do we need this info?
Does the bots print this info in logs?

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:145: // memory overhead.
Can you add asserts that these overhead dumps exists?

[Primiano Tucci (use gerrit), 2017-03-16 14:23:56]

So I talked today with hjd: let's keep having the "heaps" as part of the
existing memory infra-dump and just call it "heaps_v2" to handle the versioning.
Can you plz update the patch while he does the UI?

[DmitrySkiba, 2017-03-17 04:01:38]

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

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:254:
allocation_register.EstimateTraceMemoryOverhead(&overhead);
On 2017/03/16 02:09:20, ssid wrote:
> This part should be outside the if condition which states DETAILED dump.
> For all kinds of dump we want to estimate memory overehad and dump it under
> malloc. See the my previous comment.

This is exactly how it's working now.

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:122: LOG(INFO) << "About to
merge";
On 2017/03/16 02:09:20, ssid wrote:
> Isn't the test logs going to help with debugging here. Do we need this info?
> Does the bots print this info in logs?

Done. Thanks for noticing!

https://codereview.chromium.org/2650863003/diff/160001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:145: // memory overhead.
On 2017/03/16 02:09:20, ssid wrote:
> Can you add asserts that these overhead dumps exists?

That's an interesting test, but I don't think it's appropriate here. Here we're
testing that merging works, so I don't even know why we're asserting exact
number of allocator_dumps.

[DmitrySkiba, 2017-03-17 04:03:03]

On 2017/03/16 14:23:56, Primiano Tucci (slow - perf) wrote:
> So I talked today with hjd: let's keep having the "heaps" as part of the
> existing memory infra-dump and just call it "heaps_v2" to handle the
versioning.
> Can you plz update the patch while he does the UI?

Done. Sample trace from Linux is here:
https://drive.google.com/file/d/0B_Hmi138MnbJQllJdEdHUWcwMm8/view?usp=sharing

[hjd, 2017-03-17 12:24:26]

On 2017/03/17 04:03:03, DmitrySkiba wrote:
> On 2017/03/16 14:23:56, Primiano Tucci (slow - perf) wrote:
> > So I talked today with hjd: let's keep having the "heaps" as part of the
> > existing memory infra-dump and just call it "heaps_v2" to handle the
> versioning.
> > Can you plz update the patch while he does the UI?
> 
> Done. Sample trace from Linux is here:
> https://drive.google.com/file/d/0B_Hmi138MnbJQllJdEdHUWcwMm8/view?usp=sharing

Okay, I've updated the UI, seems to work with that file.

[Primiano Tucci (use gerrit), 2017-03-17 19:00:30]

Great thanks both.
dskiba@ can you add the other OWNERS here? So as as soon as I review hjd patch
(monday) we'll land all the things \o/

[DmitrySkiba, 2017-04-12 00:47:47]

Description was changed from

==========
New heap dump format.

TODO: unittests

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

to

==========
[tracing] Switch to new heap dump format.

This CL switches tracing to a new heap dump format, that offers the
following advantages:

1. Dumps include all the information collected by Chrome's heap profiler.

2. The format is simpler and more compact.

3. The format can easily be extended to include additional per-entry data.

4. The format allows for post-processing (see recategorization from
   crrev.com/2810523002 as an example).


BUG=708930
==========

[DmitrySkiba, 2017-04-12 00:52:41]

dskiba@chromium.org changed reviewers:
+ haraken@chromium.org, mark@chromium.org

[DmitrySkiba, 2017-04-12 00:52:42]

haraken@: please review third_party/WebKit/Source/*

mark@: please review base/BUILD.gn

[Mark Mentovai, 2017-04-12 02:40:17]

base/BUILD.gn LGTM

[haraken, 2017-04-12 07:21:56]

WebKit LGTM

[DmitrySkiba, 2017-05-19 19:37:27]

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

[commit-bot: I haz the power, 2017-05-19 19:38:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 22:43:47]

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

[commit-bot: I haz the power, 2017-05-19 22:43:48]

Dry run: This issue passed the CQ dry run.

[DmitrySkiba, 2017-05-26 23:56:38]

Primiano, please review changes related to ShardedAllocationRegister:

1. ShardedAllocationRegister::VisitAllocations is introduced.

2. ShardedAllocationRegister doesn't calculate shim metrics anymore, instead
MallocDumpProvider uses VisitAllocations() to calculate them.

[Primiano Tucci (use gerrit), 2017-05-30 18:50:53]

Not fully sure I understand the need of VisitAllocations, seems overly
complicated for what we need. 
Can you explain why we can't just update the UpdateAndReturnsMetrics method?

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:48: using MetricsMap =
std::unordered_map<AggregationKey, AllocationMetrics,
brett's PSA (see
https://chromium.googlesource.com/chromium/src/+/master/base/containers/READM...)
seems to suggest that unordererd_map makes sense only for lot of lookups. is it
the case here? should we have just a map otherwise?

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:750: bool tracing_still_enabled;
I think this is a rebase leftover. we don't want this code anymore, this will
break some UMA stuff now.

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:779: LOG_IF(ERROR,
trace_config.GetTraceRecordMode() != RECORD_CONTINUOUSLY)
FATAL (which causes a crash) or D/VLOG. any other logging is just spam.

Also I think the condition is reversed, shouldn't this be
trace_config.GetTraceRecordMode() == RECORD_CONTINUOUSLY ?
(You see, LOG(ERROR) are useless even for us two)

[DmitrySkiba, 2017-06-05 00:50:18]

We can't update UpdateAndReturnsMetrics() because the thing it would update is
now an internal detail of SerializeHeapDump().

Previously SerializeHeapDump() just iterated AllocationRegister.
ShardedAllocationRegister doesn't support iteration, so naturally I had to
refactor UpdateAndReturnsMetrics().

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/heap_...
File base/trace_event/heap_profiler_event_writer.cc (right):

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/heap_...
base/trace_event/heap_profiler_event_writer.cc:48: using MetricsMap =
std::unordered_map<AggregationKey, AllocationMetrics,
On 2017/05/30 18:50:53, Primiano Tucci wrote:
> brett's PSA (see
>
https://chromium.googlesource.com/chromium/src/+/master/base/containers/READM...)
> seems to suggest that unordererd_map makes sense only for lot of lookups. is
it
> the case here? should we have just a map otherwise?

Potentially there can be a lot of lookups, if, for example, a function (i.e.
same backtrace_id) allocated many times.

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:750: bool tracing_still_enabled;
On 2017/05/30 18:50:53, Primiano Tucci wrote:
> I think this is a rebase leftover. we don't want this code anymore, this will
> break some UMA stuff now.

Nice catch! Done.

https://codereview.chromium.org/2650863003/diff/310001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:779: LOG_IF(ERROR,
trace_config.GetTraceRecordMode() != RECORD_CONTINUOUSLY)
On 2017/05/30 18:50:53, Primiano Tucci wrote:
> FATAL (which causes a crash) or D/VLOG. any other logging is just spam.
> 
> Also I think the condition is reversed, shouldn't this be
> trace_config.GetTraceRecordMode() == RECORD_CONTINUOUSLY ?
> (You see, LOG(ERROR) are useless even for us two)

I think it was your idea to use LOG_IF :)

"
Primiano Tucci 2017/03/09 11:47:45
I think  this should be a LOG_IF(ERROR, trace_config.GetTraceRecordMode() !=
RECORD_CONTINUOUSLY) not a dcheck.
"

Anyway, I'll make it a DLOG_IF and fix the condition.

[DmitrySkiba, 2017-06-05 00:51:26]

Description was changed from

==========
[tracing] Switch to new heap dump format.

This CL switches tracing to a new heap dump format, that offers the
following advantages:

1. Dumps include all the information collected by Chrome's heap profiler.

2. The format is simpler and more compact.

3. The format can easily be extended to include additional per-entry data.

4. The format allows for post-processing (see recategorization from
   crrev.com/2810523002 as an example).


BUG=708930
==========

to

==========
[tracing] Switch to new heap dump format.

This CL switches tracing to a new heap dump format, that offers the
following advantages:

1. Dumps include all the information collected by Chrome's heap profiler.

2. The format is simpler and more compact.

3. The format can easily be extended to include additional per-entry data.

4. The format allows for post-processing (see recategorization from
   crrev.com/2906413002 as an example).


BUG=708930
==========

[Primiano Tucci (use gerrit), 2017-06-07 19:43:58]

LGTM, please send a psa to memory-dev just as a heads up in case some odd corner
case we didn't test is broken.

[DmitrySkiba, 2017-06-09 21:38:16]

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

[commit-bot: I haz the power, 2017-06-09 21:38:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-10 00:02:53]

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

[commit-bot: I haz the power, 2017-06-10 00:02:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[DmitrySkiba, 2017-06-19 18:27:14]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2017-06-19 18:27:14]

The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org,
mark@chromium.org, haraken@chromium.org, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2650863003/#ps390001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-19 18:27:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 21:10:19]

CQ is committing da patch.

Bot data: {"patchset_id": 390001, "attempt_start_ts": 1497896834162730,
"parent_rev": "b3a950fabc1d0ef5f737e0b59b5073e7fb561783", "commit_rev":
"d4a5e98235da0cc8c4fb1ae2f67826b0b480ce4c"}

[commit-bot: I haz the power, 2017-06-19 21:10:32]

Description was changed from

==========
[tracing] Switch to new heap dump format.

This CL switches tracing to a new heap dump format, that offers the
following advantages:

1. Dumps include all the information collected by Chrome's heap profiler.

2. The format is simpler and more compact.

3. The format can easily be extended to include additional per-entry data.

4. The format allows for post-processing (see recategorization from
   crrev.com/2906413002 as an example).


BUG=708930
==========

to

==========
[tracing] Switch to new heap dump format.

This CL switches tracing to a new heap dump format, that offers the
following advantages:

1. Dumps include all the information collected by Chrome's heap profiler.

2. The format is simpler and more compact.

3. The format can easily be extended to include additional per-entry data.

4. The format allows for post-processing (see recategorization from
   crrev.com/2906413002 as an example).


BUG=708930

Review-Url: https://codereview.chromium.org/2650863003
Cr-Commit-Position: refs/heads/master@{#480580}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4a5e98235da0cc8c4fb1ae2f678...
==========

[commit-bot: I haz the power, 2017-06-19 21:10:34]

Committed patchset #21 (id:390001) as
https://chromium.googlesource.com/chromium/src/+/d4a5e98235da0cc8c4fb1ae2f678...

[pdr., 2017-06-19 23:43:15]

On 2017/06/19 at 21:10:34, commit-bot wrote:
> Committed patchset #21 (id:390001) as
https://chromium.googlesource.com/chromium/src/+/d4a5e98235da0cc8c4fb1ae2f678...

I think this patch may have caused a failure:
https://bugs.chromium.org/p/chromium/issues/detail?id=734810