Remove internal debugging objects from the classes view in heap profiler.
Created attachment 143049 [details] Patch
Comment on attachment 143049 [details] Patch Attachment 143049 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12734674 New failing tests: inspector/profiler/heap-snapshot-dominators-shown-node-count-preserved-when-sorting.html inspector/profiler/heap-snapshot-dominators-show-all.html inspector/profiler/heap-snapshot-dominators-expansion-preserved-when-sorting.html inspector/profiler/heap-snapshot-reveal-in-dominators-view.html inspector/profiler/heap-snapshot-dominators-sorting.html inspector/profiler/heap-snapshot-dominators-show-next.html
Created attachment 143055 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 143049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143049&action=review r- for the failing tests. > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please describe here what the change is about. > Source/WebCore/inspector/front-end/HeapSnapshot.js:866 > + var masterFilter = function(node) { style: var masterFilter = function(node) { -> function masterFilter(node) > LayoutTests/inspector/profiler/heap-snapshot-comparison-dom-groups-change.html:57 > + edge_types: [["shortcut"], "", ""] Why did this change? > LayoutTests/inspector/profiler/heap-snapshot-test.js:402 > + nodes.push(0, 0, 1, 0, (sizeOfA + sizeOfB) * instanceCount, 0, addEdge(1, 1, null)); You can use the new mock snapshot builder to construct the raw snapshot. Ilya added it yesterday.
Comment on attachment 143049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143049&action=review >> Source/WebCore/ChangeLog:6 >> + Reviewed by NOBODY (OOPS!). > > Please describe here what the change is about. ok >> Source/WebCore/inspector/front-end/HeapSnapshot.js:866 >> + var masterFilter = function(node) { > > style: var masterFilter = function(node) { -> function masterFilter(node) done >> LayoutTests/inspector/profiler/heap-snapshot-comparison-dom-groups-change.html:57 >> + edge_types: [["shortcut"], "", ""] > > Why did this change? There're no element and property edges in the snapshot, but there are shortcuts. We user shortcut edges coming out of the root to mark user global objects. If there's no shortcuts, there's no user globals and all the objects would be filtered out. >> LayoutTests/inspector/profiler/heap-snapshot-test.js:402 >> + nodes.push(0, 0, 1, 0, (sizeOfA + sizeOfB) * instanceCount, 0, addEdge(1, 1, null)); > > You can use the new mock snapshot builder to construct the raw snapshot. Ilya added it yesterday. done
Created attachment 143327 [details] Patch
Comment on attachment 143327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143327&action=review > Source/WebCore/inspector/front-end/HeapSnapshot.js:873 > + var aggregates = this._buildAggregates(masterFilter); > + this._calculateClassesRetainedSize(aggregates.aggregatesByClassIndex, masterFilter); could you please measure the performance gain/loss for _buildAggregates > LayoutTests/inspector/profiler/heap-snapshot-test.js:39 > + 2, 6, 3, 1, 7, 6, please put one edge per line > LayoutTests/inspector/profiler/heap-snapshot-test.js:597 > + var nodeB = new InspectorTest.HeapNode("B", sizeOfB, undefined, firstId++); > + windowNode.linkNode(nodeB, InspectorTest.HeapEdge.Type.element); > + var nodeA = new InspectorTest.HeapNode("A", sizeOfA, undefined, firstId++); > + nodeB.linkNode(nodeA, InspectorTest.HeapEdge.Type.property, "a"); it is not clear why do you need explicit id here? also even ids break the contract. They can be used only for Native nodes.
Comment on attachment 143327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143327&action=review > Source/WebCore/inspector/front-end/HeapSnapshot.js:427 > + get pageObject() Let's use functions instead of getters for the new code.
Created attachment 143513 [details] Patch
Created attachment 143549 [details] Patch
Comment on attachment 143549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143549&action=review > Source/WebCore/inspector/front-end/HeapSnapshot.js:427 > + pageObject: function() pageObject -> isPageObject > Source/WebCore/inspector/front-end/HeapSnapshot.js:1656 > createNodesProviderForClass: function(className, aggregatesKey) What about other views? Should the nodes there be filtered as well? > LayoutTests/inspector/profiler/heap-snapshot-test.js:597 > + var nodeB = new InspectorTest.HeapNode("B", sizeOfB, undefined, firstId++); What about the Ilya's concern about id oddity?
Before: RESULT heap-snapshot: take-snapshot= 2656 ms RESULT heap-snapshot: transfer-snapshot= 1937 ms RESULT heap-snapshot: _buildRetainers= 403 ms RESULT heap-snapshot: _markDetachedDOMTreeNodes= 11 ms RESULT heap-snapshot: _markQueriableHeapObjects= 33 ms RESULT heap-snapshot: _markPageOwnedNodes= 43 ms RESULT heap-snapshot: _calculateFlags= 90 ms RESULT heap-snapshot: _calculateObjectToWindowDistance= 107 ms RESULT heap-snapshot: _buildPostOrderIndex= 74 ms RESULT heap-snapshot: _buildDominatorTree= 362 ms RESULT heap-snapshot: _calculateRetainedSizes= 82 ms RESULT heap-snapshot: _buildDominatedNodes= 13 ms RESULT heap-snapshot: show-snapshot= 2366 ms RESULT heap-snapshot: _buildAggregates= 590 ms RESULT heap-snapshot: _calculateClassesRetainedSize= 707 ms RESULT heap-snapshot: switch-to-containment-view= 1420 ms RESULT heap-snapshot: full-summary-snapshot-time= 8381 ms RESULT heap-snapshot: clear-snapshot= 43 ms RESULT heap-delta: heap-snapshot= 159 kB After: RESULT heap-snapshot: take-snapshot= 2634 ms RESULT heap-snapshot: transfer-snapshot= 1934 ms RESULT heap-snapshot: _buildRetainers= 415 ms RESULT heap-snapshot: _markDetachedDOMTreeNodes= 3 ms RESULT heap-snapshot: _markQueriableHeapObjects= 33 ms RESULT heap-snapshot: _markPageOwnedNodes= 44 ms RESULT heap-snapshot: _calculateFlags= 84 ms RESULT heap-snapshot: _calculateObjectToWindowDistance= 106 ms RESULT heap-snapshot: _buildPostOrderIndex= 75 ms RESULT heap-snapshot: _buildDominatorTree= 375 ms RESULT heap-snapshot: _calculateRetainedSizes= 84 ms RESULT heap-snapshot: _buildDominatedNodes= 18 ms RESULT heap-snapshot: show-snapshot= 2398 ms RESULT heap-snapshot: _buildAggregates= 604 ms RESULT heap-snapshot: _calculateClassesRetainedSize= 722 ms RESULT heap-snapshot: switch-to-containment-view= 1443 ms RESULT heap-snapshot: full-summary-snapshot-time= 8410 ms RESULT heap-snapshot: clear-snapshot= 42 ms RESULT heap-delta: heap-snapshot= 162 kB
Comment on attachment 143549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143549&action=review >> Source/WebCore/inspector/front-end/HeapSnapshot.js:427 >> + pageObject: function() > > pageObject -> isPageObject ok > Source/WebCore/inspector/front-end/HeapSnapshot.js:1656 > createNodesProviderForClass: function(className, aggregatesKey) I'm not sure. May be. >> LayoutTests/inspector/profiler/heap-snapshot-test.js:597 >> + var nodeB = new InspectorTest.HeapNode("B", sizeOfB, undefined, firstId++); > > What about the Ilya's concern about id oddity? Sorry, forgot to publish my comments. AFAICT the oddity contract is internal V8 snapshot generator feature. It is needed to avoid collisions between heap and native object ids. The frontend shouldn't know about it.
Created attachment 143561 [details] Patch
Comment on attachment 143561 [details] Patch lgtm
Comment on attachment 143561 [details] Patch Clearing flags on attachment: 143561 Committed r118182: <http://trac.webkit.org/changeset/118182>
All reviewed patches have been landed. Closing bug.