Bug 155571 – Web Inspector: HeapSnapshots are slow and use too much memory

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Operate on HeapSnapshot data within Workers.

Processing very large HeapSnapshots could cause long UI Hangs in the Inspector while recording a Timeline. Just the JSON.parse time for a 30MB JSON string could be 100ms, and then allocating objects per-Node and per-Edge could result in a 1.5-2s hang during a recording. This would result in a poor experience while recording. Move that work into a Worker and communicate with the worker in chunks where appropriate.

Likewise, we were creating many many objects (1 per Node, 1 per Edge) while processing the snapshot. We can reduce the number of allocations by instead having a number of large arrays of data. Blink / Legacy WebKit Heap Snapshot code used this approach. We can adopt that approach and also make use of the already implemented dominator calculation using that format.

May plan for workers is:

UserInterface/Proxies (not Models, because it depends on Workers and should be constructed with that in mind)
  - HeapSnapshotWorkerProxy - singleton that spawns the Web Worker and sends/receives messages
  - HeapSnapshotProxy - data for the snapshot, and async methods for querying for nodes
  - HeapSnapshotNodeProxy - data for the node, and async methods for querying relations
  - HeapSnapshotEdgeProxy - data for the edge
  - HeapSnapshotDiffProxy - behaves like a HeapSnapshotProxy with a little extra data

UserInterface/Workers/HeapSnapshot (this code is only loaded in the Worker, not the Page. e.g. new Worker("Workers/HeapSnapshot/HeapSnapshotWorker.js") and importScripts)
  - HeapSnapshotWorker.js - Worker main, sends/receives messages to the WorkerProxy
  - HeapSnapshot.js - actual impl of a HeapSnapshot logic

* WORKER INTERFACE
There is a simple RPC with callback mechanism between the Main (Proxy) and Worker objects.

WorkerProxy: (Main side)
  - performAction => sendMessage with actionName + actionArguments => call callback
  - callMethod => sendMessage with objectId + methodName + methodArguments => call callback

Worker:
  - sendEvent => sendMessage with eventName + eventData => dispatchEvent on the WorkerProxy

Proxy methods end up being simple boilerplate for dispatching and deserialization. No caching at this time.

[Radar WebKit Bug Importer]

<rdar://problem/25207991>

[Joseph Pecoraro]

Created attachment 274252 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:172
> +outer:  for (let nodeIndex = 0; nodeIndex < nodes.length; nodeIndex += nodeFieldCount) {

I don't know the best style for this. The outer could be indented and basically be "outer: for" instead of the weirdness here.

[Timothy Hatcher]

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

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

Very clean! We should consider this model for making new workers.

> LayoutTests/inspector/heap/getPreview.html:56
> +                    workerProxy.createSnapshot(snapshotStringData, ({objectId, snapshot: serializedSnapshot}) => {

Slick destructuring.

>> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:172
>> +outer:  for (let nodeIndex = 0; nodeIndex < nodes.length; nodeIndex += nodeFieldCount) {
> 
> I don't know the best style for this. The outer could be indented and basically be "outer: for" instead of the weirdness here.

I would put them on separate lines and indent outer:.

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:180
> +                    continue outer;

Mind = Blown

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/198353>

[Joseph Pecoraro]

Follow-up fix for the test after some post-review handling of <root>:
<https://trac.webkit.org/changeset/198355>