Bug 82325 - Web Inspector: Speed up heap profiler snapshot parsing
Summary: Web Inspector: Speed up heap profiler snapshot parsing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 05:03 PDT by Alexei Filippov
Modified: 2012-03-27 08:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.61 KB, patch)
2012-03-27 05:05 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (10.12 KB, patch)
2012-03-27 07:27 PDT, Alexei Filippov
no flags 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-03-27 05:03:21 PDT
It can be made faster.
Comment 1 Alexei Filippov 2012-03-27 05:05:28 PDT
Created attachment 134031 [details]
Patch
Comment 2 Ilya Tikhonovsky 2012-03-27 06:38:11 PDT
Comment on attachment 134031 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1231
> +        for (var i = 1, l = nodes.length; i < l; ++count) {

rootNodeIndex?

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1238
> +        for (var i = 1, l = nodes.length; i < l; ++count) {

ditto

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1282
> +        var indexArray = this._dominatedIndex = new Int32Array(nodeCount + 1);

Uint32Array

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1291
> +        var dominatedNodes = this._dominatedNodes = new Int32Array(nodeCount + 1);

ditto
Comment 3 Yury Semikhatsky 2012-03-27 06:51:26 PDT
Comment on attachment 134031 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1227
> +        var edgesCountOffset = this._edgesCountOffset;

We can just inline these as there is no performance gain.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1272
> +        var nodeIndexes = this.nodeIndexes;

Consider inlining these as well.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1282
> +        var indexArray = this._dominatedIndex = new Int32Array(nodeCount + 1);

Int32Array -> Uint32Array

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1291
> +        var dominatedNodes = this._dominatedNodes = new Int32Array(nodeCount + 1);

Int32Array -> Uint32Array, length of the dominatedNodes array should be equal to sum of all indexArrat values at this point, not nodeCount+1

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1389
> +        var flags = this._flags;

Please inline these.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:1403
> +            edge._edges = node.rawEdges;

Why not take node.edges? Could you at least introduce a public setter on the edge?
Comment 4 Alexei Filippov 2012-03-27 07:25:25 PDT
Comment on attachment 134031 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1227
>> +        var edgesCountOffset = this._edgesCountOffset;
> 
> We can just inline these as there is no performance gain.

done

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1231
>> +        for (var i = 1, l = nodes.length; i < l; ++count) {
> 
> rootNodeIndex?

done

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1238
>> +        for (var i = 1, l = nodes.length; i < l; ++count) {
> 
> ditto

done

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1272
>> +        var nodeIndexes = this.nodeIndexes;
> 
> Consider inlining these as well.

done

>>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1291
>>> +        var dominatedNodes = this._dominatedNodes = new Int32Array(nodeCount + 1);
>> 
>> ditto
> 
> Int32Array -> Uint32Array, length of the dominatedNodes array should be equal to sum of all indexArrat values at this point, not nodeCount+1

it is nodeCount - 1, as each node except for root has one dominator.

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1389
>> +        var flags = this._flags;
> 
> Please inline these.

done

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:1403
>> +            edge._edges = node.rawEdges;
> 
> Why not take node.edges? Could you at least introduce a public setter on the edge?

node.edges is an iterator which I'd like to avoid. Don't wanna introduce a setter, we don't have them for nodeIndex and edgeIndex.
Comment 5 Alexei Filippov 2012-03-27 07:27:26 PDT
Created attachment 134056 [details]
Patch
Comment 6 Yury Semikhatsky 2012-03-27 08:15:30 PDT
Comment on attachment 134056 [details]
Patch

Clearing flags on attachment: 134056

Committed r112271: <http://trac.webkit.org/changeset/112271>
Comment 7 Yury Semikhatsky 2012-03-27 08:15:40 PDT
All reviewed patches have been landed.  Closing bug.