Summary: | Web Inspector: JavaScript Heap Allocations Timeline | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||||||
Component: | Web Inspector | Assignee: | 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
Joseph Pecoraro
2016-03-09 19:47:49 PST
Created attachment 273533 [details]
[PATCH] Proposed Fix
Relies on a not-yet-landed patch, so no cq.
Created attachment 273534 [details]
[IMAGE] List and Overview Graph
Created attachment 273535 [details]
[IMAGE] Summary View
Created attachment 273536 [details]
[IMAGE] Instances View (strings) - Snapshot Diff
Created attachment 273537 [details]
[IMAGE] Instances View (objects) - Batched when >100
Created attachment 273538 [details]
[IMAGE] Log Value - Context Menu - if object is still alive
Created attachment 273540 [details]
[PATCH] Part 2 - Click on Timeline Graph -> Directly to ContentView
(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 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 #. (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. |