Bug 86723 – Web Inspector: filter out debugging stuff from the heap class view

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexei Filippov]

Remove internal debugging objects from the classes view in heap profiler.

[Alexei Filippov]

Created attachment 143049 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Yury Semikhatsky]

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.

[Alexei Filippov]

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

[Alexei Filippov]

Created attachment 143327 [details]
Patch

[Ilya Tikhonovsky]

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.

[Yury Semikhatsky]

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.

[Alexei Filippov]

Created attachment 143513 [details]
Patch

[Alexei Filippov]

Created attachment 143549 [details]
Patch

[Yury Semikhatsky]

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?

[Alexei Filippov]

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

[Alexei Filippov]

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.

[Alexei Filippov]

Created attachment 143561 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 143561 [details]
Patch

lgtm

[WebKit Review Bot]

Comment on attachment 143561 [details]
Patch

Clearing flags on attachment: 143561

Committed r118182: <http://trac.webkit.org/changeset/118182>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.