Bug 88324 – Web Inspector: serialize edge counts instead of indexes in heap snapshot

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

[Alexei Filippov]

That reduces the snapshot size and thus the transfer time is reduced by ~100ms or 8%.

Before:
RESULT heap-snapshot: transfer-snapshot= 1494 ms
RESULT heap-snapshot: full-summary-snapshot-time= 5827 ms

After:
RESULT heap-snapshot: transfer-snapshot= 1378 ms
RESULT heap-snapshot: _buildEdgeIndexes= 5 ms
RESULT heap-snapshot: full-summary-snapshot-time= 5587 ms

[Alexei Filippov]

Created attachment 145762 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 145762 [details]
Patch

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

other

> Source/WebCore/ChangeLog:4
> +        Web Inspector: serialize edge counts instead of indexes in heap snapshot
> +        https://bugs.webkit.org/show_bug.cgi?id=88324

please write more detailed description for the patch

> Source/WebCore/inspector/front-end/HeapSnapshot.js:720
> +            for (var nodeOrdinal = 0; nodeOrdinal < nodeCount; ++nodeOrdinal) {
> +                firstEdgeIndexes[nodeOrdinal] = nodes[nodeOrdinal * nodeFieldCount + nodeEdgesIndexOffset];

it'd be simpler to use nodeFirstEdgeIndex instead of nodeOrdinal here.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:734
> +            edgeIndex += nodes[nodeOrdinal * nodeFieldCount + nodeEdgeCountOffset] * edgeFieldCount;

ditto

[Alexei Filippov]

Comment on attachment 145762 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=88324
> 
> please write more detailed description for the patch

ok

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:720
>> +                firstEdgeIndexes[nodeOrdinal] = nodes[nodeOrdinal * nodeFieldCount + nodeEdgesIndexOffset];
> 
> it'd be simpler to use nodeFirstEdgeIndex instead of nodeOrdinal here.

I need to iterate with nodeOrdinal anyway (it is used in lhs).
So I can't get rid of it and I don't see a point of introducing second index for iteration.

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:734
>> +            edgeIndex += nodes[nodeOrdinal * nodeFieldCount + nodeEdgeCountOffset] * edgeFieldCount;
> 
> ditto

ditto

[Alexei Filippov]

Created attachment 145799 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 145799 [details]
Patch

lgtm

[Ilya Tikhonovsky]

Comment on attachment 145799 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshot.js:216
> -        this.edge.edgeIndex += this.edge._snapshot._edgeFieldsCount;
> +        this.edge.edgeIndex += this.edge._snapshot._edgeFieldCount;

could you please extract this part of the change as a separate patch

[Alexei Filippov]

Created attachment 145819 [details]
Patch

[Alexei Filippov]

Comment on attachment 145799 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:216
>> +        this.edge.edgeIndex += this.edge._snapshot._edgeFieldCount;
> 
> could you please extract this part of the change as a separate patch

done

[Ilya Tikhonovsky]

Comment on attachment 145819 [details]
Patch

Clearing flags on attachment: 145819

Committed r119498: <http://trac.webkit.org/changeset/119498>

[Ilya Tikhonovsky]

All reviewed patches have been landed.  Closing bug.