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
<rdar://problem/26375866>
Created attachment 279428 [details] [PATCH] Proposed Fix
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(){};
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) {
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.
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
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
Comment on attachment 279428 [details] [PATCH] Proposed Fix Clearing flags on attachment: 279428 Committed r201183: <http://trac.webkit.org/changeset/201183>
All reviewed patches have been landed. Closing bug.