Add JS heap snapshotting functionality to Telemetry (part 2)

Add JS heap snapshotting functionality to Telemetry (part 2)

This CL adds code which parses the JS heap snapshot retrieved from the
Dev tools.

BUG=274456
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223392

[marja, 2013-09-13 11:18:52]

nduca, ptal

Sorry, this CL is a bit frightening, but it's because the js heap snapshot
format is a bit obscure.

This code is essentially from jsleakcheck. I had to restructure it a bit, since
we didn't have a pure parser class in Jsleakcheck (the same class was fetching
the data and parsing it). I also removed some obsolete code which is only needed
for older Chromium versions, and refactored it a tiny bit (such as moved some
private functions to inner functions, if they were called from one place only,
but nothing significant).

The original code is here:
https://code.google.com/p/leak-finder-for-javascript/source/browse/src/leak_f....
It has been reviewed (even readability reviewed!) before.

[nduca, 2013-09-15 15:28:33]

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/jsheap/data.py (right):

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:2: # Use of this source code is
governed by a BSD-style license that can be
i messed this up in the previous review. core/jsheap should be called core/heap/
 --- there are other heap snapshots that i want to include in this system,
namely the tcmalloc heap snapshot. So please rename the directory and update
accordingly as part of this patch if you dont mind.

Same thing applies throughout this. We need to clearly define which classes are
ChromeJsHeap specific vs common... Node and Model i think are common, everything
else is specific to jsheap use.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:6: 
file named data.py has a class named Node? node.py if you must.

Also, is Node a general data type supported by *all* heap snapshots? Doesn't
seem like it to me.

Seems like Node is more like a LiveHeapObject. And Edge is RetainingEdge.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:30: self.node_id = node_id
what is an identifier? is it globally unique? is it just some pointer? is it
genreated by the 

it can probably be .id

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:36: self.js_name = ''
just call this name

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:64: class Edge(object):
different file

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:87: name_string: str, the
JavaScript attribute name this Edge represents.
please remove the javascript mention here. its just a type name of the edge.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:93: self.type_string = type_string
based on the ToJavascript below, it seems that this is an enum. Or not?

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/jsheap/model.py (right):

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/model.py:9: self.nodes =
parser.JsHeapSnapshotParser(raw_data).GetNodes()
it bugs me how little documentation there is on the model.

What is nodes? Is it the root nodes? Is it all the nodes?

Update the name to be precise, and ensure that there is a docstring.

If you pick a more precise name than node, that might help. LiveHeapObject,
all_live_heap_objects, etc.

Also, I'd really prefer big arrays like this to seal this in  @property fields,
e.g self._nods and @property for it.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/jsheap/parser.py (right):

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/parser.py:1: # Copyright 2013 The Chromium
Authors. All rights reserved.
can you call this

chrome_js_heap_snapshot_parser.py
and
ChromJsHeapSnapshotParser

its chrome specific if yout hink about it

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/parser.py:9: class
JsHeapSnapshotParser(object):
please add a
@staticmethod
CanImport(raw_data)
  that returns true if it is recognized as a  correct jsheapsnapshot

Then modify the Model.__init__() to first check that static method and throw the
correct error message, and if its supported, actually construct the
JsHeapSnapshotParser.

I'd like to have this designed with the correct abstractions so that we could in
theory add other snapshot formats.

[nduca, 2013-09-15 16:47:03]

But lgtm when you feel you've figured out which bits of feedback above you feel
are reasonable and applied those. Better we land and iterate a few times than
feel too tied down to my design sense. :)

[marja, 2013-09-16 11:12:47]

Thanks for the review! Based on the comments, it looked like we're reasonably
well on the same page, so I'm committing this. I can do a follow-up CL if you
have more comments.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/jsheap/data.py (right):

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:2: # Use of this source code is
governed by a BSD-style license that can be
On 2013/09/15 15:28:33, nduca wrote:
> i messed this up in the previous review. core/jsheap should be called
core/heap/
>  --- there are other heap snapshots that i want to include in this system,
> namely the tcmalloc heap snapshot. So please rename the directory and update
> accordingly as part of this patch if you dont mind.
> 
> Same thing applies throughout this. We need to clearly define which classes
are
> ChromeJsHeap specific vs common... Node and Model i think are common,
everything
> else is specific to jsheap use.

Done.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:6: 
On 2013/09/15 15:28:33, nduca wrote:
> file named data.py has a class named Node? node.py if you must.
> 
> Also, is Node a general data type supported by *all* heap snapshots? Doesn't
> seem like it to me.
> 
> Seems like Node is more like a LiveHeapObject. And Edge is RetainingEdge.

Done.

I kept the term "node" in the parser, since it's called a node in the heap
snapshot data. But it creates LiveHeapObjects.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:30: self.node_id = node_id
On 2013/09/15 15:28:33, nduca wrote:
> what is an identifier? is it globally unique? is it just some pointer? is it
> genreated by the 
> 
> it can probably be .id

This is a numeric ID which is used for identifying the node in the heap snapshot
data. I cannot use .id, pylint won't let me (LiveHeapObject.__init__: Redefining
built-in 'id') -> renamed to object_id.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:36: self.js_name = ''
On 2013/09/15 15:28:33, nduca wrote:
> just call this name

Done.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:64: class Edge(object):
On 2013/09/15 15:28:33, nduca wrote:
> different file

Done.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:87: name_string: str, the
JavaScript attribute name this Edge represents.
On 2013/09/15 15:28:33, nduca wrote:
> please remove the javascript mention here. its just a type name of the edge.

Done.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:93: self.type_string = type_string
On 2013/09/15 15:28:33, nduca wrote:
> based on the ToJavascript below, it seems that this is an enum. Or not?

At this point yes (the js heap snapshot metadata lists the possible edge types).
But if we want to keep it generic, I'd keep it as a string, so that different
heap snapshots can have different edge types, and we don't need to decide what
are the possible edge types for all snapshots (for now).

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/data.py:107: def ToJavaScript(self):
This is JavaScript-specific, what to do with this? For now, I added this as a
static function in JsHeapSnapshotParser (which is definitely
JavaScript-specific), and the same for ToJavaScript for LiveHeapObject.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/jsheap/model.py (right):

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/model.py:9: self.nodes =
parser.JsHeapSnapshotParser(raw_data).GetNodes()
On 2013/09/15 15:28:33, nduca wrote:
> it bugs me how little documentation there is on the model.
> 
> What is nodes? Is it the root nodes? Is it all the nodes?
> 
> Update the name to be precise, and ensure that there is a docstring.
> 
> If you pick a more precise name than node, that might help. LiveHeapObject,
> all_live_heap_objects, etc.
> 
> Also, I'd really prefer big arrays like this to seal this in  @property
fields,
> e.g self._nods and @property for it.

Added @property, changed it to all_live_heap_objects, added a doc string.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/jsheap/parser.py (right):

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/parser.py:1: # Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/09/15 15:28:33, nduca wrote:
> can you call this
> 
> chrome_js_heap_snapshot_parser.py
> and
> ChromJsHeapSnapshotParser
> 
> its chrome specific if yout hink about it

Done.

https://codereview.chromium.org/24076016/diff/1002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/jsheap/parser.py:9: class
JsHeapSnapshotParser(object):
On 2013/09/15 15:28:33, nduca wrote:
> please add a
> @staticmethod
> CanImport(raw_data)
>   that returns true if it is recognized as a  correct jsheapsnapshot
> 
> Then modify the Model.__init__() to first check that static method and throw
the
> correct error message, and if its supported, actually construct the
> JsHeapSnapshotParser.
> 
> I'd like to have this designed with the correct abstractions so that we could
in
> theory add other snapshot formats.

Done; though checking the correctness for real would require parsing the whole
snapshot. I'm checking some metadata here, but it is, in theory, still possible
that the data is malformed, and it'll be found out when actually parsing.

https://codereview.chromium.org/24076016/diff/12001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/jsheap/model.py (left):

https://codereview.chromium.org/24076016/diff/12001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/jsheap/model.py:8: pass
I don't know why this file doesn't show up as deleted, as it should.

The raw diff says:

Index: tools/telemetry/telemetry/core/jsheap/model.py
diff --git a/tools/telemetry/telemetry/core/jsheap/model.py
b/tools/telemetry/telemetry/core/jsheap/model.py
deleted file mode 100644

(Similar to __init__.py.)

So I'm assuming it's a code review bug, and I'll fix this later if it goes
wrong.

[commit-bot: I haz the power, 2013-09-16 11:13:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/24076016/12001

[commit-bot: I haz the power, 2013-09-16 20:08:14]

Change committed as 223392