[tracing] Support new heap dump format

[tracing] Support new heap dump format

Add support for the new heap dump format.

See:
BUG=catapult:#2947
BUG=chromium:708930
Review-Url: https://codereview.chromium.org/2635023002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/239f08ec63f401050f448ae3ca4cba48606682cf

[hjd, 2017-01-20 16:39:57]

Description was changed from

==========
[DONT SUBMIT] Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

to

==========
[DONT SUBMIT] Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

[hjd, 2017-02-03 15:50:40]

Description was changed from

==========
[DONT SUBMIT] Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

to

==========
Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

[hjd, 2017-02-03 15:50:52]

Description was changed from

==========
Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

to

==========
[tracing] Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

[hjd, 2017-02-03 15:51:07]

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

[hjd, 2017-02-03 15:51:08]

ptal, thanks!

[lpy, 2017-02-08 23:30:00]

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

[lpy, 2017-02-08 23:30:01]

LGTM.

It would be nice if you could add tests for importers, but you can do it in a
separate patch so that this won't block you.

+fmeawad@ since I am not an owner.

[DmitrySkiba, 2017-02-08 23:38:33]

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

[DmitrySkiba, 2017-02-08 23:38:34]

Please not commit just yet, there is a bug that 'Object Type' pane always shows
all types, regardless of the level I'm in. 

Also, heavy modes always crash tracing tab with OOM.

[hjd, 2017-02-10 11:05:53]

On 2017/02/08 23:38:34, DmitrySkiba wrote:
> Please not commit just yet, there is a bug that 'Object Type' pane always
shows
> all types, regardless of the level I'm in. 
> 
> Also, heavy modes always crash tracing tab with OOM.

@lpy Thanks for the review! I'll see if I can add some tests while I'm fixing
the bugs
Dmitry found.

@Dmitry

hmm, I can't reproduce the Object Type pane bug with either your trace [1] or my
own.
Could you share some more detailed steps?

I repo'ed the OOM, do you think that it happens on smaller dumps than
previously?
I wouldn't have expected this CL to have that much effect (other than the fact
that
Chrome is reporting more fine grained allocations under this scheme).
I have a few other CL's I'm working on that should improve the situation, I'll
try and land those first and see if it improves.


[1]: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view

[DmitrySkiba, 2017-02-13 19:01:44]

On 2017/02/10 11:05:53, hjd wrote:
> On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > Please not commit just yet, there is a bug that 'Object Type' pane always
> shows
> > all types, regardless of the level I'm in. 
> > 
> > Also, heavy modes always crash tracing tab with OOM.
> 
> @lpy Thanks for the review! I'll see if I can add some tests while I'm fixing
> the bugs
> Dmitry found.
> 
> @Dmitry
> 
> hmm, I can't reproduce the Object Type pane bug with either your trace [1] or
my
> own.
> Could you share some more detailed steps?
> 
> I repo'ed the OOM, do you think that it happens on smaller dumps than
> previously?
> I wouldn't have expected this CL to have that much effect (other than the fact
> that
> Chrome is reporting more fine grained allocations under this scheme).
> I have a few other CL's I'm working on that should improve the situation, I'll
> try and land those first and see if it improves.
> 
> 
> [1]: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view

Actually, I can repro Object Type issue with old format in recent Chrome
(56.0.2924.87). Steps are super-simple:

1. Open trace file
2. Go to Object Type pane, click on any type
3. Object Type pane becomes empty (cannot break down by object type any further)
4. Yet the caption for the tab still shows old object type count (i.e. count of
root object types)

Variation of this:

1. At the top level, click on random stack frame
2. Switch to Object Type tab
3. Object type pane shows several types, yet the count in the tab still shows
old number (number of root types)

I.e. the issue is that counter in Object Type tab doesn't reflect actual number
of entries in the tab.

Regarding OOM - I think you're right, and this is because traces are bigger in
the new format. BTW, I have some thoughts on how to merge stack frames from
different processes (which after symbolization are very similar) - this should
reduce trace size, but I guess we can discuss / maybe implement them later.

LGTM.

[hjd, 2017-02-14 12:47:48]

On 2017/02/13 19:01:44, DmitrySkiba wrote:
> On 2017/02/10 11:05:53, hjd wrote:
> > On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > > Please not commit just yet, there is a bug that 'Object Type' pane always
> > shows
> > > all types, regardless of the level I'm in. 
> > > 
> > > Also, heavy modes always crash tracing tab with OOM.
> > 
> > @lpy Thanks for the review! I'll see if I can add some tests while I'm
fixing
> > the bugs
> > Dmitry found.
> > 
> > @Dmitry
> > 
> > hmm, I can't reproduce the Object Type pane bug with either your trace [1]
or
> my
> > own.
> > Could you share some more detailed steps?
> > 
> > I repo'ed the OOM, do you think that it happens on smaller dumps than
> > previously?
> > I wouldn't have expected this CL to have that much effect (other than the
fact
> > that
> > Chrome is reporting more fine grained allocations under this scheme).
> > I have a few other CL's I'm working on that should improve the situation,
I'll
> > try and land those first and see if it improves.
> > 
> > 
> > [1]: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
> 
> Actually, I can repro Object Type issue with old format in recent Chrome
> (56.0.2924.87). Steps are super-simple:
> 
> 1. Open trace file
> 2. Go to Object Type pane, click on any type
> 3. Object Type pane becomes empty (cannot break down by object type any
further)
> 4. Yet the caption for the tab still shows old object type count (i.e. count
of
> root object types)

ah I see, I have a CL out that fixes this but charliea and I are
discussing the best way to deal with Polymer problems.
https://codereview.chromium.org/2653693002/

> Variation of this:
> 
> 1. At the top level, click on random stack frame
> 2. Switch to Object Type tab
> 3. Object type pane shows several types, yet the count in the tab still shows
> old number (number of root types)
> 
> I.e. the issue is that counter in Object Type tab doesn't reflect actual
number
> of entries in the tab.
> 
> Regarding OOM - I think you're right, and this is because traces are bigger in
> the new format. BTW, I have some thoughts on how to merge stack frames from
> different processes (which after symbolization are very similar) - this should
> reduce trace size, but I guess we can discuss / maybe implement them later.

Sounds great :)

> LGTM.

[hjd, 2017-03-17 12:38:27]

On 2017/02/14 12:47:48, hjd wrote:
> On 2017/02/13 19:01:44, DmitrySkiba wrote:
> > On 2017/02/10 11:05:53, hjd wrote:
> > > On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > > > Please not commit just yet, there is a bug that 'Object Type' pane
always
> > > shows
> > > > all types, regardless of the level I'm in. 
> > > > 
> > > > Also, heavy modes always crash tracing tab with OOM.
> > > 
> > > @lpy Thanks for the review! I'll see if I can add some tests while I'm
> fixing
> > > the bugs
> > > Dmitry found.
> > > 
> > > @Dmitry
> > > 
> > > hmm, I can't reproduce the Object Type pane bug with either your trace [1]
> or
> > my
> > > own.
> > > Could you share some more detailed steps?
> > > 
> > > I repo'ed the OOM, do you think that it happens on smaller dumps than
> > > previously?
> > > I wouldn't have expected this CL to have that much effect (other than the
> fact
> > > that
> > > Chrome is reporting more fine grained allocations under this scheme).
> > > I have a few other CL's I'm working on that should improve the situation,
> I'll
> > > try and land those first and see if it improves.
> > > 
> > > 
> > > [1]: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
> > 
> > Actually, I can repro Object Type issue with old format in recent Chrome
> > (56.0.2924.87). Steps are super-simple:
> > 
> > 1. Open trace file
> > 2. Go to Object Type pane, click on any type
> > 3. Object Type pane becomes empty (cannot break down by object type any
> further)
> > 4. Yet the caption for the tab still shows old object type count (i.e. count
> of
> > root object types)
> 
> ah I see, I have a CL out that fixes this but charliea and I are
> discussing the best way to deal with Polymer problems.
> https://codereview.chromium.org/2653693002/
> 
> > Variation of this:
> > 
> > 1. At the top level, click on random stack frame
> > 2. Switch to Object Type tab
> > 3. Object type pane shows several types, yet the count in the tab still
shows
> > old number (number of root types)
> > 
> > I.e. the issue is that counter in Object Type tab doesn't reflect actual
> number
> > of entries in the tab.
> > 
> > Regarding OOM - I think you're right, and this is because traces are bigger
in
> > the new format. BTW, I have some thoughts on how to merge stack frames from
> > different processes (which after symbolization are very similar) - this
should
> > reduce trace size, but I guess we can discuss / maybe implement them later.
> 
> Sounds great :)
> 
> > LGTM.

Updated to work with the latest Chrome side patch set. Mostly smallish changes.
Fadi could you ptal for an OWNERS stamp, thanks!

[fmeawad, 2017-03-17 16:31:44]

On 2017/03/17 12:38:27, hjd wrote:
> On 2017/02/14 12:47:48, hjd wrote:
> > On 2017/02/13 19:01:44, DmitrySkiba wrote:
> > > On 2017/02/10 11:05:53, hjd wrote:
> > > > On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > > > > Please not commit just yet, there is a bug that 'Object Type' pane
> always
> > > > shows
> > > > > all types, regardless of the level I'm in. 
> > > > > 
> > > > > Also, heavy modes always crash tracing tab with OOM.
> > > > 
> > > > @lpy Thanks for the review! I'll see if I can add some tests while I'm
> > fixing
> > > > the bugs
> > > > Dmitry found.
> > > > 
> > > > @Dmitry
> > > > 
> > > > hmm, I can't reproduce the Object Type pane bug with either your trace
[1]
> > or
> > > my
> > > > own.
> > > > Could you share some more detailed steps?
> > > > 
> > > > I repo'ed the OOM, do you think that it happens on smaller dumps than
> > > > previously?
> > > > I wouldn't have expected this CL to have that much effect (other than
the
> > fact
> > > > that
> > > > Chrome is reporting more fine grained allocations under this scheme).
> > > > I have a few other CL's I'm working on that should improve the
situation,
> > I'll
> > > > try and land those first and see if it improves.
> > > > 
> > > > 
> > > > [1]: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
> > > 
> > > Actually, I can repro Object Type issue with old format in recent Chrome
> > > (56.0.2924.87). Steps are super-simple:
> > > 
> > > 1. Open trace file
> > > 2. Go to Object Type pane, click on any type
> > > 3. Object Type pane becomes empty (cannot break down by object type any
> > further)
> > > 4. Yet the caption for the tab still shows old object type count (i.e.
count
> > of
> > > root object types)
> > 
> > ah I see, I have a CL out that fixes this but charliea and I are
> > discussing the best way to deal with Polymer problems.
> > https://codereview.chromium.org/2653693002/
> > 
> > > Variation of this:
> > > 
> > > 1. At the top level, click on random stack frame
> > > 2. Switch to Object Type tab
> > > 3. Object type pane shows several types, yet the count in the tab still
> shows
> > > old number (number of root types)
> > > 
> > > I.e. the issue is that counter in Object Type tab doesn't reflect actual
> > number
> > > of entries in the tab.
> > > 
> > > Regarding OOM - I think you're right, and this is because traces are
bigger
> in
> > > the new format. BTW, I have some thoughts on how to merge stack frames
from
> > > different processes (which after symbolization are very similar) - this
> should
> > > reduce trace size, but I guess we can discuss / maybe implement them
later.
> > 
> > Sounds great :)
> > 
> > > LGTM.
> 
> Updated to work with the latest Chrome side patch set. Mostly smallish
changes.
> Fadi could you ptal for an OWNERS stamp, thanks!

Can you add a test file in tracing/test_data/

[hjd, 2017-03-17 16:40:29]

On 2017/03/17 16:31:44, fmeawad wrote:
> On 2017/03/17 12:38:27, hjd wrote:
> > On 2017/02/14 12:47:48, hjd wrote:
> > > On 2017/02/13 19:01:44, DmitrySkiba wrote:
> > > > On 2017/02/10 11:05:53, hjd wrote:
> > > > > On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > > > > > Please not commit just yet, there is a bug that 'Object Type' pane
> > always
> > > > > shows
> > > > > > all types, regardless of the level I'm in. 
> > > > > > 
> > > > > > Also, heavy modes always crash tracing tab with OOM.
> > > > > 
> > > > > @lpy Thanks for the review! I'll see if I can add some tests while I'm
> > > fixing
> > > > > the bugs
> > > > > Dmitry found.
> > > > > 
> > > > > @Dmitry
> > > > > 
> > > > > hmm, I can't reproduce the Object Type pane bug with either your trace
> [1]
> > > or
> > > > my
> > > > > own.
> > > > > Could you share some more detailed steps?
> > > > > 
> > > > > I repo'ed the OOM, do you think that it happens on smaller dumps than
> > > > > previously?
> > > > > I wouldn't have expected this CL to have that much effect (other than
> the
> > > fact
> > > > > that
> > > > > Chrome is reporting more fine grained allocations under this scheme).
> > > > > I have a few other CL's I'm working on that should improve the
> situation,
> > > I'll
> > > > > try and land those first and see if it improves.
> > > > > 
> > > > > 
> > > > > [1]: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
> > > > 
> > > > Actually, I can repro Object Type issue with old format in recent Chrome
> > > > (56.0.2924.87). Steps are super-simple:
> > > > 
> > > > 1. Open trace file
> > > > 2. Go to Object Type pane, click on any type
> > > > 3. Object Type pane becomes empty (cannot break down by object type any
> > > further)
> > > > 4. Yet the caption for the tab still shows old object type count (i.e.
> count
> > > of
> > > > root object types)
> > > 
> > > ah I see, I have a CL out that fixes this but charliea and I are
> > > discussing the best way to deal with Polymer problems.
> > > https://codereview.chromium.org/2653693002/
> > > 
> > > > Variation of this:
> > > > 
> > > > 1. At the top level, click on random stack frame
> > > > 2. Switch to Object Type tab
> > > > 3. Object type pane shows several types, yet the count in the tab still
> > shows
> > > > old number (number of root types)
> > > > 
> > > > I.e. the issue is that counter in Object Type tab doesn't reflect actual
> > > number
> > > > of entries in the tab.
> > > > 
> > > > Regarding OOM - I think you're right, and this is because traces are
> bigger
> > in
> > > > the new format. BTW, I have some thoughts on how to merge stack frames
> from
> > > > different processes (which after symbolization are very similar) - this
> > should
> > > > reduce trace size, but I guess we can discuss / maybe implement them
> later.
> > > 
> > > Sounds great :)
> > > 
> > > > LGTM.
> > 
> > Updated to work with the latest Chrome side patch set. Mostly smallish
> changes.
> > Fadi could you ptal for an OWNERS stamp, thanks!
> 
> Can you add a test file in tracing/test_data/

Hmm I tried to add Dmitry's file, I get this message:
Not uploading the current file for
tracing/test_data/sampling_heap_dump_format.json.gz because it's too large.
It's ~4.2mb which is smaller than some of the other trace files in test_data.
Is there some special way to upload large files?

[fmeawad, 2017-03-17 16:58:18]

On 2017/03/17 16:40:29, hjd wrote:
> On 2017/03/17 16:31:44, fmeawad wrote:
> > On 2017/03/17 12:38:27, hjd wrote:
> > > On 2017/02/14 12:47:48, hjd wrote:
> > > > On 2017/02/13 19:01:44, DmitrySkiba wrote:
> > > > > On 2017/02/10 11:05:53, hjd wrote:
> > > > > > On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > > > > > > Please not commit just yet, there is a bug that 'Object Type' pane
> > > always
> > > > > > shows
> > > > > > > all types, regardless of the level I'm in. 
> > > > > > > 
> > > > > > > Also, heavy modes always crash tracing tab with OOM.
> > > > > > 
> > > > > > @lpy Thanks for the review! I'll see if I can add some tests while
I'm
> > > > fixing
> > > > > > the bugs
> > > > > > Dmitry found.
> > > > > > 
> > > > > > @Dmitry
> > > > > > 
> > > > > > hmm, I can't reproduce the Object Type pane bug with either your
trace
> > [1]
> > > > or
> > > > > my
> > > > > > own.
> > > > > > Could you share some more detailed steps?
> > > > > > 
> > > > > > I repo'ed the OOM, do you think that it happens on smaller dumps
than
> > > > > > previously?
> > > > > > I wouldn't have expected this CL to have that much effect (other
than
> > the
> > > > fact
> > > > > > that
> > > > > > Chrome is reporting more fine grained allocations under this
scheme).
> > > > > > I have a few other CL's I'm working on that should improve the
> > situation,
> > > > I'll
> > > > > > try and land those first and see if it improves.
> > > > > > 
> > > > > > 
> > > > > > [1]:
https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
> > > > > 
> > > > > Actually, I can repro Object Type issue with old format in recent
Chrome
> > > > > (56.0.2924.87). Steps are super-simple:
> > > > > 
> > > > > 1. Open trace file
> > > > > 2. Go to Object Type pane, click on any type
> > > > > 3. Object Type pane becomes empty (cannot break down by object type
any
> > > > further)
> > > > > 4. Yet the caption for the tab still shows old object type count (i.e.
> > count
> > > > of
> > > > > root object types)
> > > > 
> > > > ah I see, I have a CL out that fixes this but charliea and I are
> > > > discussing the best way to deal with Polymer problems.
> > > > https://codereview.chromium.org/2653693002/
> > > > 
> > > > > Variation of this:
> > > > > 
> > > > > 1. At the top level, click on random stack frame
> > > > > 2. Switch to Object Type tab
> > > > > 3. Object type pane shows several types, yet the count in the tab
still
> > > shows
> > > > > old number (number of root types)
> > > > > 
> > > > > I.e. the issue is that counter in Object Type tab doesn't reflect
actual
> > > > number
> > > > > of entries in the tab.
> > > > > 
> > > > > Regarding OOM - I think you're right, and this is because traces are
> > bigger
> > > in
> > > > > the new format. BTW, I have some thoughts on how to merge stack frames
> > from
> > > > > different processes (which after symbolization are very similar) -
this
> > > should
> > > > > reduce trace size, but I guess we can discuss / maybe implement them
> > later.
> > > > 
> > > > Sounds great :)
> > > > 
> > > > > LGTM.
> > > 
> > > Updated to work with the latest Chrome side patch set. Mostly smallish
> > changes.
> > > Fadi could you ptal for an OWNERS stamp, thanks!
> > 
> > Can you add a test file in tracing/test_data/
> 
> Hmm I tried to add Dmitry's file, I get this message:
> Not uploading the current file for
> tracing/test_data/sampling_heap_dump_format.json.gz because it's too large.
> It's ~4.2mb which is smaller than some of the other trace files in test_data.
> Is there some special way to upload large files?

Can you send me the file for testing for now, and I will figure out the large
file issue later (I believe it is an issue with rietveld)

[hjd, 2017-03-17 17:00:08]

On 2017/03/17 16:58:18, fmeawad wrote:
> On 2017/03/17 16:40:29, hjd wrote:
> > On 2017/03/17 16:31:44, fmeawad wrote:
> > > On 2017/03/17 12:38:27, hjd wrote:
> > > > On 2017/02/14 12:47:48, hjd wrote:
> > > > > On 2017/02/13 19:01:44, DmitrySkiba wrote:
> > > > > > On 2017/02/10 11:05:53, hjd wrote:
> > > > > > > On 2017/02/08 23:38:34, DmitrySkiba wrote:
> > > > > > > > Please not commit just yet, there is a bug that 'Object Type'
pane
> > > > always
> > > > > > > shows
> > > > > > > > all types, regardless of the level I'm in. 
> > > > > > > > 
> > > > > > > > Also, heavy modes always crash tracing tab with OOM.
> > > > > > > 
> > > > > > > @lpy Thanks for the review! I'll see if I can add some tests while
> I'm
> > > > > fixing
> > > > > > > the bugs
> > > > > > > Dmitry found.
> > > > > > > 
> > > > > > > @Dmitry
> > > > > > > 
> > > > > > > hmm, I can't reproduce the Object Type pane bug with either your
> trace
> > > [1]
> > > > > or
> > > > > > my
> > > > > > > own.
> > > > > > > Could you share some more detailed steps?
> > > > > > > 
> > > > > > > I repo'ed the OOM, do you think that it happens on smaller dumps
> than
> > > > > > > previously?
> > > > > > > I wouldn't have expected this CL to have that much effect (other
> than
> > > the
> > > > > fact
> > > > > > > that
> > > > > > > Chrome is reporting more fine grained allocations under this
> scheme).
> > > > > > > I have a few other CL's I'm working on that should improve the
> > > situation,
> > > > > I'll
> > > > > > > try and land those first and see if it improves.
> > > > > > > 
> > > > > > > 
> > > > > > > [1]:
> https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
> > > > > > 
> > > > > > Actually, I can repro Object Type issue with old format in recent
> Chrome
> > > > > > (56.0.2924.87). Steps are super-simple:
> > > > > > 
> > > > > > 1. Open trace file
> > > > > > 2. Go to Object Type pane, click on any type
> > > > > > 3. Object Type pane becomes empty (cannot break down by object type
> any
> > > > > further)
> > > > > > 4. Yet the caption for the tab still shows old object type count
(i.e.
> > > count
> > > > > of
> > > > > > root object types)
> > > > > 
> > > > > ah I see, I have a CL out that fixes this but charliea and I are
> > > > > discussing the best way to deal with Polymer problems.
> > > > > https://codereview.chromium.org/2653693002/
> > > > > 
> > > > > > Variation of this:
> > > > > > 
> > > > > > 1. At the top level, click on random stack frame
> > > > > > 2. Switch to Object Type tab
> > > > > > 3. Object type pane shows several types, yet the count in the tab
> still
> > > > shows
> > > > > > old number (number of root types)
> > > > > > 
> > > > > > I.e. the issue is that counter in Object Type tab doesn't reflect
> actual
> > > > > number
> > > > > > of entries in the tab.
> > > > > > 
> > > > > > Regarding OOM - I think you're right, and this is because traces are
> > > bigger
> > > > in
> > > > > > the new format. BTW, I have some thoughts on how to merge stack
frames
> > > from
> > > > > > different processes (which after symbolization are very similar) -
> this
> > > > should
> > > > > > reduce trace size, but I guess we can discuss / maybe implement them
> > > later.
> > > > > 
> > > > > Sounds great :)
> > > > > 
> > > > > > LGTM.
> > > > 
> > > > Updated to work with the latest Chrome side patch set. Mostly smallish
> > > changes.
> > > > Fadi could you ptal for an OWNERS stamp, thanks!
> > > 
> > > Can you add a test file in tracing/test_data/
> > 
> > Hmm I tried to add Dmitry's file, I get this message:
> > Not uploading the current file for
> > tracing/test_data/sampling_heap_dump_format.json.gz because it's too large.
> > It's ~4.2mb which is smaller than some of the other trace files in
test_data.
> > Is there some special way to upload large files?
> 
> Can you send me the file for testing for now, and I will figure out the large
> file issue later (I believe it is an issue with rietveld)

Here you go: https://drive.google.com/file/d/0B_Hmi138MnbJZldBV3psWHhWT3M/view
:)

[hjd, 2017-03-22 15:39:56]

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

[Primiano Tucci (use gerrit), 2017-03-22 15:53:52]

Fadi, Hector any update on this?
Would be nice to move the ball forward here. 
Is there anything blocking here?

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
File tracing/tracing/extras/importer/heap_dump_trace_event_importer.html
(right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/heap_dump_trace_event_importer.html:4: Copyright
2017 The Chromium Authors. All rights reserved.
I think the rule is to never update years for existing files.

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
File tracing/tracing/extras/importer/streaming_event_expander.html (right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/streaming_event_expander.html:103: * Extracts
data from a 'streaming p event'.
IMHO would be great if this description could be a bit more concrete and reflect
what is really happening here.
I read this description seveal times but couldn't quite figure out:
1. What a "streaming p event" is?
2. what this class generally does. The names are IMHO a biit too generic, all
this seems to be about maps, group, metadata, inflation and expansion. Which I
could get if we were talking about //base or STL, but I think that this does
something way more specific and would be great if the names did reflect that.

[fmeawad, 2017-03-23 20:43:58]

LGTM, 

We need to figure out how to upload the test_data file, and I was wondering if
you can add a test case to trace_event_importer_test.html

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
File tracing/tracing/extras/importer/heap_dump_trace_event_importer.html
(right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/heap_dump_trace_event_importer.html:4: Copyright
2017 The Chromium Authors. All rights reserved.
On 2017/03/22 15:53:52, Primiano Tucci wrote:
> I think the rule is to never update years for existing files.

The original file was moved to legacy_, so in practice this is a new file, I am
fine with either.

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
File tracing/tracing/extras/importer/streaming_event_expander.html (right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/streaming_event_expander.html:103: * Extracts
data from a 'streaming p event'.
On 2017/03/22 15:53:52, Primiano Tucci wrote:
> IMHO would be great if this description could be a bit more concrete and
reflect
> what is really happening here.
> I read this description seveal times but couldn't quite figure out:
> 1. What a "streaming p event" is?
> 2. what this class generally does. The names are IMHO a biit too generic, all
> this seems to be about maps, group, metadata, inflation and expansion. Which I
> could get if we were talking about //base or STL, but I think that this does
> something way more specific and would be great if the names did reflect that.
+1
Can this class be used to import non memory related events/dumps. If not please
rename to include memory in the class name.

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:2649: var
processTypeMap = this.objectTypeNameMap_[pid];
nit: let

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:2656: var importer =
new LegacyHeapDumpTraceEventImporter(this.model_,
nit: var not needed since the variable is already defined in the function scope,
also the else case does not have the var.
Or remove the top definition and depend on hoisting.

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/model/...
File tracing/tracing/model/heap_dump.html (right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/model/...
tracing/tracing/model/heap_dump.html:25: heapDump, leafStackFrame,
objectTypeName, size, count, valuesAreSelf) {
nit: valuesAreSelf is not very descriptive, can you add @param or change the
name?

[hjd, 2017-04-07 12:57:14]

Description was changed from

==========
[tracing] Add support for streaming heap profile format

First pass at adding support for the new format.

BUG=catapult:#2947
==========

to

==========
[tracing] Support new heap dump format

Add support for the new heap dump format.

See:
BUG=catapult:#2947
BUG=708930
==========

[hjd, 2017-04-11 15:18:16]

Description was changed from

==========
[tracing] Support new heap dump format

Add support for the new heap dump format.

See:
BUG=catapult:#2947
BUG=708930
==========

to

==========
[tracing] Support new heap dump format

Add support for the new heap dump format.

See:
BUG=catapult:#2947
BUG=chromium:708930
==========

[DmitrySkiba, 2017-05-19 18:33:41]

hjd@, when do you think you can land this?

[hjd, 2017-05-22 15:43:43]

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
File tracing/tracing/extras/importer/trace_event_importer.html (right):

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:2649: var
processTypeMap = this.objectTypeNameMap_[pid];
On 2017/03/23 20:43:57, fmeawad wrote:
> nit: let

Done, thanks!

https://codereview.chromium.org/2635023002/diff/180001/tracing/tracing/extras...
tracing/tracing/extras/importer/trace_event_importer.html:2656: var importer =
new LegacyHeapDumpTraceEventImporter(this.model_,
On 2017/03/23 20:43:57, fmeawad wrote:
> nit: var not needed since the variable is already defined in the function
scope,
> also the else case does not have the var.
> Or remove the top definition and depend on hoisting.

Done, Thanks!

[hjd, 2017-05-22 15:48:23]

On 2017/05/19 18:33:41, DmitrySkiba wrote:
> hjd@, when do you think you can land this?

Just rebased. This works with
https://drive.google.com/file/d/0B_Hmi138MnbJQllJdEdHUWcwMm8/view?usp=sharing
I will add the extra tests tomorrow, test with the old format and do one more
pass and then it should be ready to land.

[DmitrySkiba, 2017-05-23 17:38:08]

On 2017/05/22 15:48:23, hjd wrote:
> On 2017/05/19 18:33:41, DmitrySkiba wrote:
> > hjd@, when do you think you can land this?
> 
> Just rebased. This works with
> https://drive.google.com/file/d/0B_Hmi138MnbJQllJdEdHUWcwMm8/view?usp=sharing
> I will add the extra tests tomorrow, test with the old format and do one more
> pass and then it should be ready to land.

Do you think you'll be able to land it this week? I want to finish this whole
thing this week.

[hjd, 2017-05-26 13:51:36]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-05-26 13:51:37]

The patchset sent to the CQ was uploaded after l-g-t-m from lpy@chromium.org,
dskiba@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2635023002/#ps300001
(title: "fix bug''fix bug")

[commit-bot: I haz the power, 2017-05-26 13:51:41]

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-26 14:15:39]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495806696746240,
"parent_rev": "91b60843fbfce5b81e9bc92bdfcba85fd26ed22e", "commit_rev":
"239f08ec63f401050f448ae3ca4cba48606682cf"}

[commit-bot: I haz the power, 2017-05-26 14:15:45]

Description was changed from

==========
[tracing] Support new heap dump format

Add support for the new heap dump format.

See:
BUG=catapult:#2947
BUG=chromium:708930
==========

to

==========
[tracing] Support new heap dump format

Add support for the new heap dump format.

See:
BUG=catapult:#2947
BUG=chromium:708930
Review-Url: https://codereview.chromium.org/2635023002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-05-26 14:15:46]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...