Bug 155287 – Web Inspector: JavaScript Heap Allocations Timeline

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

[Joseph Pecoraro]

* 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

[Joseph Pecoraro]

Created attachment 273533 [details]
[PATCH] Proposed Fix

Relies on a not-yet-landed patch, so no cq.

[Joseph Pecoraro]

Created attachment 273534 [details]
[IMAGE] List and Overview Graph

[Joseph Pecoraro]

Created attachment 273535 [details]
[IMAGE] Summary View

[Joseph Pecoraro]

Created attachment 273536 [details]
[IMAGE] Instances View (strings) - Snapshot Diff

[Radar WebKit Bug Importer]

<rdar://problem/25078088>

[Joseph Pecoraro]

Created attachment 273537 [details]
[IMAGE] Instances View (objects) - Batched when >100

[Joseph Pecoraro]

Created attachment 273538 [details]
[IMAGE] Log Value - Context Menu - if object is still alive

[Joseph Pecoraro]

Created attachment 273540 [details]
[PATCH] Part 2 - Click on Timeline Graph -> Directly to ContentView

[Matt Baker]

(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.

[Timothy Hatcher]

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 #.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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