Bug 87286 - Web Inspector: Speed up edges iteration in heap profiler
: Web Inspector: Speed up edges iteration in heap profiler
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexei Filippov
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 11:00 PDT by Alexei Filippov
Modified: 2012-05-25 10:40 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Filippov 2012-05-23 11:00:03 PDT
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 Alexei Filippov 2012-05-23 11:43:31 PDT
Created attachment 143604 [details]
Patch
Comment 2 Yury Semikhatsky 2012-05-24 10:47:15 PDT
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
Comment 3 Alexei Filippov 2012-05-24 11:08:01 PDT
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 Alexei Filippov 2012-05-24 11:14:57 PDT
Created attachment 143855 [details]
Patch
Comment 5 Ilya Tikhonovsky 2012-05-25 00:56:42 PDT
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.
Comment 6 Yury Semikhatsky 2012-05-25 01:34:58 PDT
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?
Comment 7 Alexei Filippov 2012-05-25 04:07:43 PDT
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.
Comment 8 Yury Semikhatsky 2012-05-25 07:57:08 PDT
(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 Ilya Tikhonovsky 2012-05-25 08:33:06 PDT
Committed r118528: <http://trac.webkit.org/changeset/118528>
Comment 10 Ilya Tikhonovsky 2012-05-25 10:40:28 PDT
(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