Bug 194448 - Web Inspector: Import / Export Heap Snapshots
Summary: Web Inspector: Import / Export Heap Snapshots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-08 13:46 PST by Joseph Pecoraro
Modified: 2019-02-08 16:43 PST (History)
7 users (show)

See Also:


Attachments
[IMAGE] Import Button and Imported Title (517.30 KB, image/png)
2019-02-08 13:48 PST, Joseph Pecoraro
no flags Details
[IMAGE] Export Button (not there for Diffs) (650.39 KB, image/png)
2019-02-08 13:48 PST, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (35.48 KB, patch)
2019-02-08 13:57 PST, Joseph Pecoraro
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Landing (39.78 KB, patch)
2019-02-08 15:22 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-02-08 13:46:15 PST
Import / Export Heap Snapshots

• Should work with GCDebugging snapshots as well.
Comment 1 Radar WebKit Bug Importer 2019-02-08 13:46:36 PST
<rdar://problem/47928093>
Comment 2 Joseph Pecoraro 2019-02-08 13:48:02 PST
Created attachment 361530 [details]
[IMAGE] Import Button and Imported Title
Comment 3 Joseph Pecoraro 2019-02-08 13:48:37 PST
Created attachment 361531 [details]
[IMAGE] Export Button (not there for Diffs)
Comment 4 Devin Rousso 2019-02-08 13:53:05 PST
(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
Comment 5 Joseph Pecoraro 2019-02-08 13:57:53 PST
Created attachment 361533 [details]
[PATCH] Proposed Fix
Comment 6 Devin Rousso 2019-02-08 14:35:10 PST
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?
Comment 7 Joseph Pecoraro 2019-02-08 14:38:36 PST
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.
Comment 8 Joseph Pecoraro 2019-02-08 15:22:06 PST
Created attachment 361540 [details]
[PATCH] For Landing
Comment 9 Joseph Pecoraro 2019-02-08 15:23:03 PST
> 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`.
Comment 10 Devin Rousso 2019-02-08 15:41:35 PST
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?
Comment 11 Joseph Pecoraro 2019-02-08 16:00:40 PST
(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.
Comment 12 WebKit Commit Bot 2019-02-08 16:25:52 PST
Comment on attachment 361540 [details]
[PATCH] For Landing

Clearing flags on attachment: 361540

Committed r241219: <https://trac.webkit.org/changeset/241219>
Comment 13 Yusuke Suzuki 2019-02-08 16:27:30 PST
Cool!