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, saam, 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+

Joseph Pecoraro
Reported 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
Attachments
[PATCH] Proposed Fix (150.16 KB, patch)
2016-03-09 20:00 PST, Joseph Pecoraro 		timothy: review+
DetailsFormatted DiffDiff
[IMAGE] List and Overview Graph (90.91 KB, image/png)
2016-03-09 20:00 PST, Joseph Pecoraro 		no flags
Details
[IMAGE] Summary View (158.30 KB, image/png)
2016-03-09 20:01 PST, Joseph Pecoraro 		no flags
Details
[IMAGE] Instances View (strings) - Snapshot Diff (194.77 KB, image/png)
2016-03-09 20:01 PST, Joseph Pecoraro 		no flags
Details
[IMAGE] Instances View (objects) - Batched when >100 (280.82 KB, image/png)
2016-03-09 20:02 PST, Joseph Pecoraro 		no flags
Details
[IMAGE] Log Value - Context Menu - if object is still alive (132.33 KB, image/png)
2016-03-09 20:02 PST, Joseph Pecoraro 		no flags
Details
[PATCH] Part 2 - Click on Timeline Graph -> Directly to ContentView (9.42 KB, patch)
2016-03-09 20:07 PST, Joseph Pecoraro 		timothy: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2016-03-09 20:00:08 PST
Created attachment 273533 [details] [PATCH] Proposed Fix Relies on a not-yet-landed patch, so no cq.
Joseph Pecoraro
Comment 2 2016-03-09 20:00:31 PST
Created attachment 273534 [details] [IMAGE] List and Overview Graph
Joseph Pecoraro
Comment 3 2016-03-09 20:01:05 PST
Created attachment 273535 [details] [IMAGE] Summary View
Joseph Pecoraro
Comment 4 2016-03-09 20:01:31 PST
Created attachment 273536 [details] [IMAGE] Instances View (strings) - Snapshot Diff
Radar WebKit Bug Importer
Comment 5 2016-03-09 20:01:44 PST
<rdar://problem/25078088>
Joseph Pecoraro
Comment 6 2016-03-09 20:02:01 PST
Created attachment 273537 [details] [IMAGE] Instances View (objects) - Batched when >100
Joseph Pecoraro
Comment 7 2016-03-09 20:02:39 PST
Created attachment 273538 [details] [IMAGE] Log Value - Context Menu - if object is still alive
Joseph Pecoraro
Comment 8 2016-03-09 20:07:26 PST
Created attachment 273540 [details] [PATCH] Part 2 - Click on Timeline Graph -> Directly to ContentView
Matt Baker
Comment 9 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.
Timothy Hatcher
Comment 10 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 #.
Joseph Pecoraro
Comment 11 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.
Joseph Pecoraro
Comment 12 2016-03-10 13:38:05 PST
<http://trac.webkit.org/changeset/197954>
Note You need to log in before you can comment on or make changes to this bug.