Bug 155188 – Web Inspector: Add a way to create a Heap Snapshot

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Add a way to create a Heap Snapshot.

HeapAgent.snapshot => (error, timestamp, snapshotData)

[Radar WebKit Bug Importer]

<rdar://problem/25042222>

[Joseph Pecoraro]

Created attachment 273322 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 273324 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 273334 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 273341 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 273341 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=273341&action=review

very clean. r=me

> LayoutTests/inspector/heap/snapshot.html:22
> +                resolve();

I'm assuming these magic numbers come from builtins / injected scripts?

> Source/JavaScriptCore/inspector/protocol/Heap.json:18
> +            "type": "string"

I am tempted to add JSON and base64 to the protocol, if we add more uses (which we will).

> Source/WebInspectorUI/UserInterface/Models/HeapSnapshot.js:88
> +            let nodePayload = nodes[i];

I assume this loop body is what led you to complain about slow array restructuring.

> Source/WebInspectorUI/UserInterface/Models/HeapSnapshot.js:116
> +        console.assert(rootNode, "Node with identifier 0 is the artificial <root> node.");

s/artificial/synthetic/

> Source/WebInspectorUI/UserInterface/Models/HeapSnapshotEdge.js:30
> +        this.from = from;

Might be worth adding a comment saying this is a directed edge between two HeapSnapshotNodes, 'to' and 'from'. Adding assertions to that effect is probably a perf problem.

[Joseph Pecoraro]

Comment on attachment 273341 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=273341&action=review

>> LayoutTests/inspector/heap/snapshot.html:22
>> +                resolve();
> 
> I'm assuming these magic numbers come from builtins / injected scripts?

Nope, the magic numbers are just low ball estimates to ensure they are not 0. The heap size should be at least 1kb for a page, with at least 100 objects (all the builtin APIs), and at least one "Window".

>> Source/JavaScriptCore/inspector/protocol/Heap.json:18
>> +            "type": "string"
> 
> I am tempted to add JSON and base64 to the protocol, if we add more uses (which we will).

Yeah, I considered this! Worth adding in the future for sure.

>> Source/WebInspectorUI/UserInterface/Models/HeapSnapshot.js:88
>> +            let nodePayload = nodes[i];
> 
> I assume this loop body is what led you to complain about slow array restructuring.

Correct.

>> Source/WebInspectorUI/UserInterface/Models/HeapSnapshotEdge.js:30
>> +        this.from = from;
> 
> Might be worth adding a comment saying this is a directed edge between two HeapSnapshotNodes, 'to' and 'from'. Adding assertions to that effect is probably a perf problem.

Yes, adding an assert was a performance problem.

[Joseph Pecoraro]

Created attachment 273367 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 273367 [details]
[PATCH] For Landing

Clearing flags on attachment: 273367

Committed r197822: <http://trac.webkit.org/changeset/197822>