Bug 194448 – Web Inspector: Import / Export Heap Snapshots

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

[Joseph Pecoraro]

Import / Export Heap Snapshots

• Should work with GCDebugging snapshots as well.

[Radar WebKit Bug Importer]

<rdar://problem/47928093>

[Joseph Pecoraro]

Created attachment 361530 [details]
[IMAGE] Import Button and Imported Title

[Joseph Pecoraro]

Created attachment 361531 [details]
[IMAGE] Export Button (not there for Diffs)

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #2)
> Created attachment 361530 [details]
> [IMAGE] Import Button and Imported Title
I think we should put the "Imported" _after_ the name of the file.  If we import a ton of heap snapshots, it would be weird (to me) to see a list of multiple "Imported - *" in a row.

Are you planning on adding drag/drop support for importing as well?  The `processJSON` function of `WI.CanvasManager` and `WI.AuditManager` (and the way in which that function gets called) is a good example of how we support this elsewhere

[Joseph Pecoraro]

Created attachment 361533 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

r=me

Can we test this functionality?  I'd imagine it would go something like this:
1. take a heap snapshot
2. export it
3. test that the exported string from #2 matches the expected format/structure (to some degree of accuracy)
4. import the exported string from #2 (this may require a bit of refactoring so that you wouldn't have to call `WI.FileUtilities.importText`)
5. check that the imported heap snapshot from #4 and the original heap snapshot from #1 match (to some degree of accuracy)

> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotProxy.js:86
> +    set snapshotStringData(x)

NIT: is `x` really the best name? 😂

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:62
> +        this._importButtonNavigationItem = new WI.ButtonNavigationItem("import", WI.UIString("Import"), "Images/Import.svg", 15, 15);

What about drag/drop?  I think we'd want to limit it to only working when the "Heap Allocations" view is shown, but it could also work anywhere in the "Timelines" tab and forcibly show the "Heap Allocations" view when a heap snapshot is imported.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:63
> +        this._importButtonNavigationItem.toolTip = WI.UIString("Import snapshot");

Our usual style is to have the tooltip match the text shown.  As such, this should also be `WI.UIString("Import")`.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:65
> +        this._importButtonNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, this._importButtonNavigationItemClicked, this);

NIT: I prefer to prefix all event listeners with `handle`, so this would be `this._handleImportButtonNavigationItemClicked`.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:408
> +        WI.FileUtilities.importText(function(result) {

NIT: arrow function?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:414
> +                const timestamp = NaN;

Style: add a newline before this.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotContentView.js:115
> +        let filename = WI.UIString("Heap Snapshot %s-%s-%s at %s.%s.%s").format(...values);

If only <https://webkit.org/b/194279> has landed, so that you'd have this functionality on `WI.FileUtilities` :(

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:-150
> -    // Protected

Why?

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:82
> +        this._imported = imported;

Should we add assertions for any functions that don't make sense when imported (e.g. `updateDeadNodesAndGatherCollectionData`)?

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:89
> +        console.assert(!type || (type === "Inspector" || type === "GCDebugging"), "Expect an Inspector / GCDebugging Heap Snapshot");

NIT: are the parenthesis necessary, or more for clarity when reading?

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:91
> +        this.nodeFieldCount = nodeFieldCount;

This value isn't used outside this class, so I'd make it "private" (e.g. `_nodeFieldCount`).  The static functions are also "part" of this class, so I think it's fine for them to access this value.

NIT: please put this in an `else`, so we aren't assigning twice in the "GCDebugging" case.

> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshotWorker.js:37
> +        this._importedSnapshots = [];

Is there actually any benefit to keeping a list of imported snapshots?

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotProxy.js:86
>> +    set snapshotStringData(x)
> 
> NIT: is `x` really the best name? 😂

I'll move this to data. But I've always found it unnecessary to use a name for a setter like this because we all know what is going to happen.

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:-150
>> -    // Protected
> 
> Why?

It is already there a few lines above.

>> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:82
>> +        this._imported = imported;
> 
> Should we add assertions for any functions that don't make sense when imported (e.g. `updateDeadNodesAndGatherCollectionData`)?

Sure!

>> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshot.js:89
>> +        console.assert(!type || (type === "Inspector" || type === "GCDebugging"), "Expect an Inspector / GCDebugging Heap Snapshot");
> 
> NIT: are the parenthesis necessary, or more for clarity when reading?

Only clarity.

>> Source/WebInspectorUI/UserInterface/Workers/HeapSnapshot/HeapSnapshotWorker.js:37
>> +        this._importedSnapshots = [];
> 
> Is there actually any benefit to keeping a list of imported snapshots?

I suppose there isn't. The `this._objects` map has them and that is good enough.

[Joseph Pecoraro]

Created attachment 361540 [details]
[PATCH] For Landing

[Joseph Pecoraro]

> Can we test this functionality?

Added a test for the full import path. Export is not really tested, but export is just the save dialog of the raw `snapshotStringData`.

[Devin Rousso]

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

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

> LayoutTests/inspector/heap/imported-snapshot.html:30
> +                        resolve();

Do we want to also check that the original and imported snapshots are the same?

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:281
> +                return WI.UIString("Imported \u2014 %s").format(timelineRecord.heapSnapshot.title);

Do we want to keep this as is or reverse the order of the "Imported" and title?

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #10)
> Comment on attachment 361540 [details]
> [PATCH] For Landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361540&action=review
> 
> > LayoutTests/inspector/heap/imported-snapshot.html:30
> > +                        resolve();
> 
> Do we want to also check that the original and imported snapshots are the
> same?

Seems unnecessary. They aren't going to be backed by the same instance, but their data should be identical. This is not any different than calling createSnapshot() twice with the same data, the createImportedSnapshot just provides a title and gets the imported flag set.

> > Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:281
> > +                return WI.UIString("Imported \u2014 %s").format(timelineRecord.heapSnapshot.title);
> 
> Do we want to keep this as is or reverse the order of the "Imported" and
> title?

I'm fine with this as is.

[WebKit Commit Bot]

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

Clearing flags on attachment: 361540

Committed r241219: <https://trac.webkit.org/changeset/241219>

[Yusuke Suzuki]

Cool!