Bug 87286 - Web Inspector: Speed up edges iteration in heap profiler
: Web Inspector: Speed up edges iteration in heap profiler
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-23 11:00 PST by
Modified: 2012-05-25 10:40 PST (History)


Attachments
Patch (10.47 KB, patch)
2012-05-23 11:43 PST, Alexei Filippov
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2012-05-24 11:14 PST, Alexei Filippov
yurys: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-23 11:00:03 PST
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.
------- Comment #1 From 2012-05-23 11:43:31 PST -------
Created an attachment (id=143604) [details]
Patch
------- Comment #2 From 2012-05-24 10:47:15 PST -------
(From update of attachment 143604 [details])
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
------- Comment #3 From 2012-05-24 11:08:01 PST -------
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
------- Comment #4 From 2012-05-24 11:14:57 PST -------
Created an attachment (id=143855) [details]
Patch
------- Comment #5 From 2012-05-25 00:56:42 PST -------
(From update of attachment 143855 [details])
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.
------- Comment #6 From 2012-05-25 01:34:58 PST -------
(From update of attachment 143855 [details])
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?
------- Comment #7 From 2012-05-25 04:07:43 PST -------
(From update of attachment 143855 [details])
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.
------- Comment #8 From 2012-05-25 07:57:08 PST -------
(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?
------- Comment #9 From 2012-05-25 08:33:06 PST -------
Committed r118528: <http://trac.webkit.org/changeset/118528>
------- Comment #10 From 2012-05-25 10:40:28 PST -------
(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