Bug 157920 – Web Inspector: HeapSnapshot Instances view should remove dead objects

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

[Joseph Pecoraro]

Created attachment 279427 [details]
[TEST] Test Page

* SUMMARY
HeapSnapshot Instances view should remove dead objects.

This would make Heap Snapshot Comparisons much more useful. Previously if a lot of objects were created and went away, then the diff would be cluttered with dead objects. By only focusing on live objects, you see exactly what has "leaked", or just what new objects were created and persist.

* TEST
<button id="x">Action</button>
<script>
window.x.onclick = function() {
    console.takeHeapSnapshot("before");    
    window.list = [];
    for (let i = 0; i < 200; ++i)
        window.list.push({a:"string" + i});
    setInterval(function() { window.list.pop(); }, 1000);
    console.takeHeapSnapshot("after");
}
</script>

* STEPS TO REPRODUCE
1. Inspect test page
2. Show Timeline Tab
3. Enable JavaScript Allocations Timeline
4. Start Timeline recording
5. Click the button on the page
6. Keep timeline recording...
7. Show Heap Snapshot comparison between "before" and "after" snapshots
8. Expand "Object" section
9. Wait and watch
  => Every 10 seconds when the periodic snapshot happens the count of "Objects" and "strings" should drop
  => When no more new objects exist the Instances category should go away
  => The "Fetch More" buttons should be accurate

[Radar WebKit Bug Importer]

<rdar://problem/26375866>

[Joseph Pecoraro]

Created attachment 279428 [details]
[PATCH] Proposed Fix

[Matt Baker]

Comment on attachment 279428 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/unit-tests/heap-snapshot-collection-event.html:23
> +        name: "HeapSnapshot",

Are we skipping test case descriptions, since they aren't shown anywhere?

> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotProxy.js:98
> +};

Is there a reason we aren't using Symbol for events?

> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotWorkerProxy.js:54
> +        this.performAction("clearSnapshots", callback);

Making callback optional would be cleaner than requiring callers to supply a no-op:

callback = callback | function(){};

[Timothy Hatcher]

Comment on attachment 279428 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotClassDataGridNode.js:170
> +        if (count) {
> +            for (let i = 0; i <= count; ++i) {

Could combine.

for (let i = 0; count && i <= count; ++i) {

[Joseph Pecoraro]

Comment on attachment 279428 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/unit-tests/heap-snapshot-collection-event.html:23
>> +        name: "HeapSnapshot",
> 
> Are we skipping test case descriptions, since they aren't shown anywhere?

This is shown as "-- Running test case: HeapSnapshot"

>> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotProxy.js:98
>> +};
> 
> Is there a reason we aren't using Symbol for events?

Hmm, I suppose we could but I'm not sure it would be much of a win. We have so many events.

>> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotWorkerProxy.js:54
>> +        this.performAction("clearSnapshots", callback);
> 
> Making callback optional would be cleaner than requiring callers to supply a no-op:
> 
> callback = callback | function(){};

Though normally I would agree, I wanted to enforce callbacks at this level to make the WorkerProxy interface as clean and easy to reason about as possible. The method signature has it all.

[Build Bot]

Comment on attachment 279428 [details]
[PATCH] Proposed Fix

Attachment 279428 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1349818

New failing tests:
inspector/debugger/scriptParsed.html

[Build Bot]

Created attachment 279431 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[WebKit Commit Bot]

Comment on attachment 279428 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 279428

Committed r201183: <http://trac.webkit.org/changeset/201183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.