Bug 155287

Summary: Web Inspector: JavaScript Heap Allocations Timeline
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, bburg, commit-queue, ggaren, graouts, joepeck, keith_miller, kling, mark.lam, mattbaker, msaboff, nvasilyev, sbarati, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 155264    
Bug Blocks: 155282    
Attachments:
Description Flags
[PATCH] Proposed Fix
timothy: review+
[IMAGE] List and Overview Graph
none
[IMAGE] Summary View
none
[IMAGE] Instances View (strings) - Snapshot Diff
none
[IMAGE] Instances View (objects) - Batched when >100
none
[IMAGE] Log Value - Context Menu - if object is still alive
none
[PATCH] Part 2 - Click on Timeline Graph -> Directly to ContentView timothy: review+

Description Joseph Pecoraro 2016-03-09 19:47:49 PST
* SUMMARY
JavaScript Heap Allocations Timeline.

First draft. This gets us:
  - timeline graph (snapshots in the timeline, start/stop/periodic and manually triggered)
  - summary view (breakdown top objects by size / count, most are internal)
  - instances view (list of each object and its own size, not yet retained size)
  - diff view (all the added objects between the two snapshots)

Next on the agenda:
  - a way to see the "Path to GC Root" per instance
    - to see the entire path to the object, like "window.foo[1].bar" or ""point.inner in closure scope of window.methods[0]"
  - attribute internal objects to the non-internal (retained size / dominated size)
    - for instance associate CodeBlocks/Executables with the Function that "owns" it
  - update snapshot data if objects get collected while still recording
    - more useful for developers to see only the live objects and not count objects that may have eventually gotten collected
Comment 1 Joseph Pecoraro 2016-03-09 20:00:08 PST
Created attachment 273533 [details]
[PATCH] Proposed Fix

Relies on a not-yet-landed patch, so no cq.
Comment 2 Joseph Pecoraro 2016-03-09 20:00:31 PST
Created attachment 273534 [details]
[IMAGE] List and Overview Graph
Comment 3 Joseph Pecoraro 2016-03-09 20:01:05 PST
Created attachment 273535 [details]
[IMAGE] Summary View
Comment 4 Joseph Pecoraro 2016-03-09 20:01:31 PST
Created attachment 273536 [details]
[IMAGE] Instances View (strings) - Snapshot Diff
Comment 5 Radar WebKit Bug Importer 2016-03-09 20:01:44 PST
<rdar://problem/25078088>
Comment 6 Joseph Pecoraro 2016-03-09 20:02:01 PST
Created attachment 273537 [details]
[IMAGE] Instances View (objects) - Batched when >100
Comment 7 Joseph Pecoraro 2016-03-09 20:02:39 PST
Created attachment 273538 [details]
[IMAGE] Log Value - Context Menu - if object is still alive
Comment 8 Joseph Pecoraro 2016-03-09 20:07:26 PST
Created attachment 273540 [details]
[PATCH] Part 2 - Click on Timeline Graph -> Directly to ContentView
Comment 9 Matt Baker 2016-03-09 20:27:45 PST
(In reply to comment #3)
> Created attachment 273535 [details]
> [IMAGE] Summary View

Nice! Some graph coloring suggestions:

1. Class Size/Class Count graphs: I think we should be consistent with the colors used to denote different types: "string" should have the same color in both graphs.

2. Allocation Size graph: what about using a different pool of colors, to help make this graph stand out from the others? Maybe ramp two complementary colors, a cool and warm color, to show Small to Very Large.
Comment 10 Timothy Hatcher 2016-03-09 21:50:02 PST
Comment on attachment 273533 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/HeapSnapshotDiff.js:75
> +        // FIXME: This only includes the newly added instances. Should we do anything with the removed instances?

I do think it would be useful to track what goes away, that could help you solve a bug with weak map references or some other collected objects you didn't expect to go away.

> Source/WebInspectorUI/UserInterface/Views/FormattedValue.js:160
> +    span.textContent = description.substring(0, description.indexOf("("));

Trim spaces after the substring too?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineDataGridNode.js:36
> +            name: WebInspector.UIString("Snapshot #%d").format(this._record.heapSnapshot.identifier),

Drop the #.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:172
> +            let diffComponent = new WebInspector.HierarchicalPathComponent(WebInspector.UIString("Snapshot Diff (#%d to #%d)").format(firstSnapshotIdentifier, secondSnapshotIdentifier), "snapshot-diff-icon", "snapshot-diff");

Is it really %d "to" %d? 1 to 3 would not fully take into account additions or removals in snapshot 2. Should it be "%d and %d"? I am not fond of the #s either.

Al other labels say "Comparison". This is the first use of "Diff". Should be consistent.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:194
> +    treeElementPathComponentSelected(event)
> +    {
> +        // FIXME: Nothing. The sidebar will soon be removed.
> +    }

I think with Matt's change today this isn't needed.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotClassDataGridNode.js:55
> +            percentElement.textContent = percent.toFixed(1) + "%";

Use your new helper function.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:66
> +            percentElement.textContent = percent.toFixed(1) + "%";

Helper.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:80
> +            idElement.textContent = "@" + id + " ";

I'm still meh about showing these. I don't think they provide any value to the user. Maybe only show it if we have not other data to show?

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstancesContentView.css:120
> +    text-indent: 0; /* Do not inherit the DataGrid's indent. */

Hmm, annoying.

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotSummaryContentView.css:111
> +    background: var(--memory-javascript-fill-color);

Great!

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:259
> +            return WebInspector.UIString("Snapshot #%d").format(timelineRecord.heapSnapshot.identifier);

Drop the #.
Comment 11 Joseph Pecoraro 2016-03-10 13:37:01 PST
(In reply to comment #9)
> (In reply to comment #3)
> > Created attachment 273535 [details]
> > [IMAGE] Summary View
> 
> Nice! Some graph coloring suggestions:
> 
> 1. Class Size/Class Count graphs: I think we should be consistent with the
> colors used to denote different types: "string" should have the same color
> in both graphs.
> 
> 2. Allocation Size graph: what about using a different pool of colors, to
> help make this graph stand out from the others? Maybe ramp two complementary
> colors, a cool and warm color, to show Small to Very Large.

(In reply to comment #10)
> Comment on attachment 273533 [details]
> > Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:80
> > +            idElement.textContent = "@" + id + " ";
> 
> I'm still meh about showing these. I don't think they provide any value to
> the user. Maybe only show it if we have not other data to show?

There is still opportunity to change the UI in many ways. I want to continue to add functionality and I'll talk with both of you about refining the UI over time.

As it stands, there still isn't a way to turn on the memory instruments without manual intervention, so the UI can be considered hidden at this point.
Comment 12 Joseph Pecoraro 2016-03-10 13:38:05 PST
<http://trac.webkit.org/changeset/197954>