Bug 87286 – Web Inspector: Speed up edges iteration in heap profiler

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

[Alexei Filippov]

Add an extra node to nodes array that points to the end of edges array.
It allows to eliminate a check for the last node in iteration code.

[Alexei Filippov]

Created attachment 143604 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 143604 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Web Inspector: Speed up edges iteration in heap profiler

Does it have any noticeable impact on the performance? Please attach measurement results for the heap profiler perf test.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:724
> +        this._nodesLength = this._nodes.length;

_nodesLength name is confusing because _nodes.length is actually different, you can use this.nodeCount * this._nodeFieldCount for nodesLength where necessary or pick a better name that would reflect the difference with this._nodes.length

[Alexei Filippov]

Before:

RESULT heap-snapshot: take-snapshot= 2169 ms
RESULT heap-snapshot: transfer-snapshot= 1719 ms
RESULT heap-snapshot: _buildRetainers= 448 ms
RESULT heap-snapshot: _markDetachedDOMTreeNodes= 2 ms
RESULT heap-snapshot: _markQueriableHeapObjects= 32 ms
RESULT heap-snapshot: _markPageOwnedNodes= 43 ms
RESULT heap-snapshot: _calculateFlags= 80 ms
RESULT heap-snapshot: _calculateObjectToWindowDistance= 144 ms
RESULT heap-snapshot: _buildPostOrderIndex= 73 ms
RESULT heap-snapshot: _buildDominatorTree= 364 ms
RESULT heap-snapshot: _calculateRetainedSizes= 83 ms
RESULT heap-snapshot: _buildDominatedNodes= 17 ms
RESULT heap-snapshot: show-snapshot= 2259 ms
RESULT heap-snapshot: _buildAggregates= 631 ms
RESULT heap-snapshot: _calculateClassesRetainedSize= 719 ms
RESULT heap-snapshot: switch-to-containment-view= 1470 ms
RESULT heap-snapshot: full-summary-snapshot-time= 7620 ms
RESULT heap-snapshot: clear-snapshot= 58 ms
RESULT heap-delta: heap-snapshot= 139 kB

After:

RESULT heap-snapshot: take-snapshot= 2139 ms
RESULT heap-snapshot: transfer-snapshot= 1721 ms
RESULT heap-snapshot: _buildRetainers= 86 ms
RESULT heap-snapshot: _markDetachedDOMTreeNodes= 2 ms
RESULT heap-snapshot: _markQueriableHeapObjects= 33 ms
RESULT heap-snapshot: _markPageOwnedNodes= 44 ms
RESULT heap-snapshot: _calculateFlags= 83 ms
RESULT heap-snapshot: _calculateObjectToWindowDistance= 163 ms
RESULT heap-snapshot: _buildPostOrderIndex= 78 ms
RESULT heap-snapshot: _buildDominatorTree= 401 ms
RESULT heap-snapshot: _calculateRetainedSizes= 83 ms
RESULT heap-snapshot: _buildDominatedNodes= 17 ms
RESULT heap-snapshot: show-snapshot= 2077 ms
RESULT heap-snapshot: _buildAggregates= 615 ms
RESULT heap-snapshot: _calculateClassesRetainedSize= 719 ms
RESULT heap-snapshot: switch-to-containment-view= 1443 ms
RESULT heap-snapshot: full-summary-snapshot-time= 7382 ms
RESULT heap-snapshot: clear-snapshot= 69 ms
RESULT heap-delta: heap-snapshot= 142 kB

[Alexei Filippov]

Created attachment 143855 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 143855 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshot.js:724
> +        this._realNodesLength = this._nodes.length;

I'd call it just _nodesLength

> Source/WebCore/inspector/front-end/HeapSnapshot.js:730
> +        this._nodes = new Uint32Array(this._realNodesLength + this._nodeFieldCount);

Actually _nodes array has extra elements that were stripped in WebInspector.Uint32Array.array getter.
Thus we can avoid additional array reallocation here.

[Yury Semikhatsky]

Comment on attachment 143855 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:730
>> +        this._nodes = new Uint32Array(this._realNodesLength + this._nodeFieldCount);
> 
> Actually _nodes array has extra elements that were stripped in WebInspector.Uint32Array.array getter.
> Thus we can avoid additional array reallocation here.

Can we pass an array with nodeLength + nodeFieldCount elements so that we don't need to allocate new array?

[Alexei Filippov]

Comment on attachment 143855 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:724
>> +        this._realNodesLength = this._nodes.length;
> 
> I'd call it just _nodesLength

I'd too, but Yury asked to rename it.

>>> Source/WebCore/inspector/front-end/HeapSnapshot.js:730
>>> +        this._nodes = new Uint32Array(this._realNodesLength + this._nodeFieldCount);
>> 
>> Actually _nodes array has extra elements that were stripped in WebInspector.Uint32Array.array getter.
>> Thus we can avoid additional array reallocation here.
> 
> Can we pass an array with nodeLength + nodeFieldCount elements so that we don't need to allocate new array?

@Ilya actually it does not, because the exact number of nodes is allocated in HeapSnapshotLoader.
@Yury That was my initial implementation, but I didn't like it because in this case the logic has to go into the HeapSnapshotLoader.

[Yury Semikhatsky]

(In reply to comment #7)
> @Yury That was my initial implementation, but I didn't like it because in this case the logic has to go into the HeapSnapshotLoader.

Do you have numbers of what it costs us?

[Ilya Tikhonovsky]

Committed r118528: <http://trac.webkit.org/changeset/118528>

[Ilya Tikhonovsky]

(In reply to comment #8)
> (In reply to comment #7)
> > @Yury That was my initial implementation, but I didn't like it because in this case the logic has to go into the HeapSnapshotLoader.
> 
> Do you have numbers of what it costs us?

it cost us 11ms