Add JS heap snapshotting infrastructure to Telemetry (part 1)

Add JS heap snapshotting functionality to Telemetry (part 1)

This retrieves a JS heap snapshot from the Dev Tools but
doesn't parse it yet.

BUG=274456
NOTRY=true

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

[marja, 2013-08-16 14:17:37]

Hi nduca,

So I started working on this JS heap snapshotting in Telemetry.

A question:

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:240:
tab._inspector_backend)
This is illegal; where can I get the inspector backend from? The tracing backend
doesn't need it, since it sends the requests itself, but for this we need to use
the connection opened by the inspector backend; if we try to open another one,
dev tools won't accept it.

[nduca, 2013-08-17 01:24:44]

neat, some thoughts

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:238:
def TakeJSHeapSnapshot(self, tab):
canyou make this a member of the web_contents.py class instead of the browser
object? snapshots are a per-tab(or extension/app) thing, and web contents is the
place for that thing. In there, you might just be able to make the
TakeJSHeapSnapshot be a method on web contents  and not have a backend object of
your own... just have WebContents.TakeJSHeapSNapshot that returs the parsed heap
object.

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/browser.py:325: def TakeJSHeapSnapshot(self,
tab):
one thing you could do early on is land telemetry.heap_snapshot.model.py with a
Model class in it that, at first has nothing in it. You could define this method
as returning that, and set it up to take a constructor with the heap snapshot
results. Then you could have inside telemetry.heap_snapshot.model a
inspector_heap_snapshot_importer.py that the model object uses to convert the .
Again, all empty at front, but could be filled in later. Again, copying the
timeline.Model concept of importers vs model that we have already.

[marja, 2013-08-22 16:37:09]

Thanks for having a look, could you have another look?

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:238:
def TakeJSHeapSnapshot(self, tab):
On 2013/08/17 01:24:44, nduca (ooo untill 8-26) wrote:
> canyou make this a member of the web_contents.py class instead of the browser
> object? snapshots are a per-tab(or extension/app) thing, and web contents is
the
> place for that thing. In there, you might just be able to make the
> TakeJSHeapSnapshot be a method on web contents  and not have a backend object
of
> your own... just have WebContents.TakeJSHeapSNapshot that returs the parsed
heap
> object.

Done.

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/22923013/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/browser.py:325: def TakeJSHeapSnapshot(self,
tab):
On 2013/08/17 01:24:44, nduca (ooo untill 8-26) wrote:
> one thing you could do early on is land telemetry.heap_snapshot.model.py with
a
> Model class in it that, at first has nothing in it. You could define this
method
> as returning that, and set it up to take a constructor with the heap snapshot
> results. Then you could have inside telemetry.heap_snapshot.model a
> inspector_heap_snapshot_importer.py that the model object uses to convert the
.
> Again, all empty at front, but could be filled in later. Again, copying the
> timeline.Model concept of importers vs model that we have already.

Ok for the model part; I'm not sure if we need the "importer" abstraction here
(but that can be decided in a follow-up cl)..

[nduca, 2013-08-28 21:51:07]

looks solid but puzzled by the syncrquestwithhandler

https://codereview.chromium.org/22923013/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/inspector_backend.py (right):

https://codereview.chromium.org/22923013/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/inspector_backend.py:279: 
bit confused about this... does the reply not come back synchronously or
something?

[marja, 2013-08-29 13:12:04]

https://codereview.chromium.org/22923013/diff/11001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/inspector_backend.py (right):

https://codereview.chromium.org/22923013/diff/11001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/inspector_backend.py:279: 
On 2013/08/28 21:51:07, nduca wrote:
> bit confused about this... does the reply not come back synchronously or
> something?

I had missed that InspectorBackend has this RegisterDomain thing. In the latest
patch set I have removed this part of the code and use RegisterDomain.

[nduca, 2013-08-29 23:53:05]

https://codereview.chromium.org/22923013/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py (right):

https://codereview.chromium.org/22923013/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py:13: def
TakeJSHeapSnapshot(self, timeout=120):
As far as I can tell, this is synchronous, no?

Why don't you unregister the domain at line 31? It seems like the flow is
you request the snapshot in 28... then you get a flurry of notifications, and
then the getHeapSnapshot call constructs the model from the notifications
received at that point. Leaving the domain registered seems incorrect...

[marja, 2013-08-30 06:46:58]

Ah yes, need to unregister too.

https://codereview.chromium.org/22923013/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py (right):

https://codereview.chromium.org/22923013/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py:13: def
TakeJSHeapSnapshot(self, timeout=120):
On 2013/08/29 23:53:05, nduca wrote:
> As far as I can tell, this is synchronous, no?
> 
> Why don't you unregister the domain at line 31? It seems like the flow is
> you request the snapshot in 28... then you get a flurry of notifications, and
> then the getHeapSnapshot call constructs the model from the notifications
> received at that point. Leaving the domain registered seems incorrect...

Done.

[nduca, 2013-08-31 06:26:12]

https://codereview.chromium.org/22923013/diff/20001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py (right):

https://codereview.chromium.org/22923013/diff/20001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py:37: def
_OnNotification(self, res):
this can probabbly be a local method in TakeJSHeapSnapsho.

And, at that point, you could move this all to the
inspector_backend.TakeJSHeapSnapshot.The only reason we'd make you pull
something to a backend is if the backend itself does severe magic, which the
tracing one does [unlike this one]. Make sense?

lgtm though!

[marja, 2013-09-02 07:39:04]

Thanks for review!

https://codereview.chromium.org/22923013/diff/20001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py (right):

https://codereview.chromium.org/22923013/diff/20001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/js_heap_snapshot_backend.py:37: def
_OnNotification(self, res):
On 2013/08/31 06:26:13, nduca wrote:
> this can probabbly be a local method in TakeJSHeapSnapsho.
> 
> And, at that point, you could move this all to the
> inspector_backend.TakeJSHeapSnapshot.The only reason we'd make you pull
> something to a backend is if the backend itself does severe magic, which the
> tracing one does [unlike this one]. Make sense?
> 
> lgtm though!

Done.

[commit-bot: I haz the power, 2013-09-02 07:45:40]

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

[commit-bot: I haz the power, 2013-09-02 07:45:53]

Change committed as 220814