Bug 81930 – Web Inspector: split nodes and containment edges into two different arrays

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

[Yury Semikhatsky]

We are planning to send nodes and edges in separate arrays from the backend. This is the first step on the way to the new format: we convert current representation to the new one so that we can incrementally switch heap profiler to the new format.

[Yury Semikhatsky]

Created attachment 133290 [details]
Patch

[Yury Semikhatsky]

Created attachment 133291 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 133291 [details]
Patch

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

> Source/WebCore/inspector/front-end/HeapSnapshot.js:811
> +        while (true) {
> +            if (index >= this._nodes.length)
> +                break;

while (index < this._nodes.length)

> Source/WebCore/inspector/front-end/HeapSnapshot.js:813
> +            var edgesCount = this._nodes[index + this._edgesCountOffset];

please extract this._edgesCountOffset as a local const.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:815
> +            if (edgesCount > 1)
> +                totalEdgeCount += edgesCount;

I'm not understand the this magic if - statement.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:816
> +            index += this._firstEdgeOffset + edgesCount * edgeFieldCount;

please use nodeFieldCount instead of this._firstEdgeOffset

> Source/WebCore/inspector/front-end/HeapSnapshot.js:820
> +        this._containmentEdges = new Int32Array(totalEdgeCount * this._edgeFieldsCount);

this._edgeFieldsCount -> edgeFieldsCount

> Source/WebCore/inspector/front-end/HeapSnapshot.js:836
> +        while (true) {
> +            if (srcIndex >= this._nodes.length)
> +                break;

while (srcIndex < this._nodes.length)

> Source/WebCore/inspector/front-end/HeapSnapshot.js:837
> +            var srcNodeTypeIndex = srcIndex + this._nodeTypeOffset;

If this is necessary from performance point of view please introduce const for this._nodeTypeOffset value. Otherwise please remove such const from the other places.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:839
> +            var edgesCount = this._nodes[srcIndex + this._edgesCountOffset];

ditto for this._edgesCountOffset

> Source/WebCore/inspector/front-end/HeapSnapshot.js:841
> +            for (var i = 0; i < nodeFieldCount; i++)
> +                this._onlyNodes[dstIndex++] = this._nodes[srcIndex++];

I'm not sure but it could be faster to use set function here
this._onlyNodes.set(this._nodes.subarray(srcIndex, nodeFieldCount), dstIndex);

> Source/WebCore/inspector/front-end/HeapSnapshot.js:850
> +        while (true) {
> +            if (newNodeIndex >= this._onlyNodes.length)
> +                break;

it can be 'for' cycle

> Source/WebCore/inspector/front-end/HeapSnapshot.js:851
> +            var dominatorIndex = this._onlyNodes[newNodeIndex + this._dominatorOffset]

const for this._dominatorOffset

> Source/WebCore/inspector/front-end/HeapSnapshot.js:853
> +            var newDominatoIndex = this._nodes[dominatorIndex + this._nodeTypeOffset];
> +            this._onlyNodes[newNodeIndex + this._dominatorOffset] = newDominatoIndex;

this._onlyNodes[newNodeIndex + this._dominatorOffset] = this._nodes[dominatorIndex + this._nodeTypeOffset];
const for nodeTypeOffset and dominatorOffset
nit: missed r symbol in newDominatoIndex

> Source/WebCore/inspector/front-end/HeapSnapshot.js:863
> +        while (true) {
> +            if (srcIndex >= this._nodes.length)
> +                break;

ditto

> Source/WebCore/inspector/front-end/HeapSnapshot.js:864
> +            var srcNodeTypeIndex = srcIndex + this._nodeTypeOffset;

const nodeTypeOffset?

> Source/WebCore/inspector/front-end/HeapSnapshot.js:881
> +        while (true) {
> +            if (srcIndex >= this._nodes.length)
> +                break;

a variable for length, etc.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:892
> +                this._retainingNodes[firstUnusedRetainerSlot] = srcNodeNewIndex;
> +                this._retainingNodes[firstUnusedRetainerSlot + 1] = this._retainingNodes[destinationNewIndex];
> +                this._retainingNodes[destinationNewIndex] = firstUnusedRetainerSlot;
> +                firstUnusedRetainerSlot += 2;

I think this is an overhead. It could be enough to have an array of offsets and array of newEdgeIndexes.
These two arrays can be joined into one but I see no reasons for this.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:907
> +        while (true) {
> +            if (srcIndex >= this._nodes.length)
> +                break;

ditto

> Source/WebCore/inspector/front-end/HeapSnapshot.js:913
> +            var edgesCount = this._nodes[srcIndex + this._edgesCountOffset];

const

> Source/WebCore/inspector/front-end/HeapSnapshot.js:914
> +            srcIndex += this._firstEdgeOffset;

const

> Source/WebCore/inspector/front-end/HeapSnapshot.js:916
> +            for (var i = 0; i < edgesCount * this._edgeFieldsCount; i++) {

for (var i = 0, l = edgesCount * this._edgeFieldsCount; i < l; i++)

> Source/WebCore/inspector/front-end/HeapSnapshot.js:925
> +            for (var i = 0; i < edgesCount * this._edgeFieldsCount; i++) {
> +                this._containmentEdges[edgeArrayIndex++] = this._nodes[srcIndex++];
> +            }
> +
> +            // Translate destination node indexes for the copied edges.
> +            for (var destinationIndex = firstEdgeIndex + this._edgeToNodeOffset; destinationIndex < edgeArrayIndex; destinationIndex += this._edgeFieldsCount) {
> +                var oldDestinationNodeIndex = this._containmentEdges[destinationIndex];
> +                var newDestinationNodeIndex = this._nodes[oldDestinationNodeIndex + this._nodeTypeOffset];
> +                this._containmentEdges[destinationIndex] = newDestinationNodeIndex;
> +            }

looks weird. please join these cycles together

> Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:96
> +    this._worker = /*typeof InspectorTest === "undefined" ? new WebInspector.HeapSnapshotRealWorker() :*/ new WebInspector.HeapSnapshotFakeWorker();

wrong change

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 133291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133291&action=review
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:811
> > +        while (true) {
> > +            if (index >= this._nodes.length)
> > +                break;
> 
> while (index < this._nodes.length)
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:813
> > +            var edgesCount = this._nodes[index + this._edgesCountOffset];
> 
> please extract this._edgesCountOffset as a local const.
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:815
> > +            if (edgesCount > 1)
> > +                totalEdgeCount += edgesCount;
> 
> I'm not understand the this magic if - statement.
> 
Fixed.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:816
> > +            index += this._firstEdgeOffset + edgesCount * edgeFieldCount;
> 
> please use nodeFieldCount instead of this._firstEdgeOffset
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:820
> > +        this._containmentEdges = new Int32Array(totalEdgeCount * this._edgeFieldsCount);
> 
> this._edgeFieldsCount -> edgeFieldsCount
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:836
> > +        while (true) {
> > +            if (srcIndex >= this._nodes.length)
> > +                break;
> 
> while (srcIndex < this._nodes.length)
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:837
> > +            var srcNodeTypeIndex = srcIndex + this._nodeTypeOffset;
> 
> If this is necessary from performance point of view please introduce const for this._nodeTypeOffset value. Otherwise please remove such const from the other places.
> 
Done. It doesn't give any noticeable performance gain so I've removed the consts.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:839
> > +            var edgesCount = this._nodes[srcIndex + this._edgesCountOffset];
> 
> ditto for this._edgesCountOffset
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:841
> > +            for (var i = 0; i < nodeFieldCount; i++)
> > +                this._onlyNodes[dstIndex++] = this._nodes[srcIndex++];
> 
> I'm not sure but it could be faster to use set function here
> this._onlyNodes.set(this._nodes.subarray(srcIndex, nodeFieldCount), dstIndex);
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:850
> > +        while (true) {
> > +            if (newNodeIndex >= this._onlyNodes.length)
> > +                break;
> 
> it can be 'for' cycle
Done.

> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:851
> > +            var dominatorIndex = this._onlyNodes[newNodeIndex + this._dominatorOffset]
> 
> const for this._dominatorOffset
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:853
> > +            var newDominatoIndex = this._nodes[dominatorIndex + this._nodeTypeOffset];
> > +            this._onlyNodes[newNodeIndex + this._dominatorOffset] = newDominatoIndex;
> 
> this._onlyNodes[newNodeIndex + this._dominatorOffset] = this._nodes[dominatorIndex + this._nodeTypeOffset];
> const for nodeTypeOffset and dominatorOffset
> nit: missed r symbol in newDominatoIndex
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:863
> > +        while (true) {
> > +            if (srcIndex >= this._nodes.length)
> > +                break;
> 
> ditto
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:864
> > +            var srcNodeTypeIndex = srcIndex + this._nodeTypeOffset;
> 
> const nodeTypeOffset?
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:881
> > +        while (true) {
> > +            if (srcIndex >= this._nodes.length)
> > +                break;
> 
> a variable for length, etc.
> 
Why? There is no performance gain in this case.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:892
> > +                this._retainingNodes[firstUnusedRetainerSlot] = srcNodeNewIndex;
> > +                this._retainingNodes[firstUnusedRetainerSlot + 1] = this._retainingNodes[destinationNewIndex];
> > +                this._retainingNodes[destinationNewIndex] = firstUnusedRetainerSlot;
> > +                firstUnusedRetainerSlot += 2;
> 
> I think this is an overhead. It could be enough to have an array of offsets and array of newEdgeIndexes.
> These two arrays can be joined into one but I see no reasons for this.
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:907
> > +        while (true) {
> > +            if (srcIndex >= this._nodes.length)
> > +                break;
> 
> ditto
Done.

> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:913
> > +            var edgesCount = this._nodes[srcIndex + this._edgesCountOffset];
> 
> const
Const wouldn't add any performance gain, so I'd leave it this way.

> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:914
> > +            srcIndex += this._firstEdgeOffset;
> 
> const
> 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:916
> > +            for (var i = 0; i < edgesCount * this._edgeFieldsCount; i++) {
> 
> for (var i = 0, l = edgesCount * this._edgeFieldsCount; i < l; i++)
> 
It wouldn't make any difference in terms of performance but would add additional characters, I'd leave it as is.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:925
> > +            for (var i = 0; i < edgesCount * this._edgeFieldsCount; i++) {
> > +                this._containmentEdges[edgeArrayIndex++] = this._nodes[srcIndex++];
> > +            }
> > +
> > +            // Translate destination node indexes for the copied edges.
> > +            for (var destinationIndex = firstEdgeIndex + this._edgeToNodeOffset; destinationIndex < edgeArrayIndex; destinationIndex += this._edgeFieldsCount) {
> > +                var oldDestinationNodeIndex = this._containmentEdges[destinationIndex];
> > +                var newDestinationNodeIndex = this._nodes[oldDestinationNodeIndex + this._nodeTypeOffset];
> > +                this._containmentEdges[destinationIndex] = newDestinationNodeIndex;
> > +            }
> 
> looks weird. please join these cycles together
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshotProxy.js:96
> > +    this._worker = /*typeof InspectorTest === "undefined" ? new WebInspector.HeapSnapshotRealWorker() :*/ new WebInspector.HeapSnapshotFakeWorker();
> 
> wrong change
Reverted.

[Yury Semikhatsky]

Created attachment 133736 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 133736 [details]
Patch

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

please resort functions' bodies in order of invocations.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:812
> +        this._onlyNodes = new Uint32Array((nodeCount + 1) * this._nodeFieldCount);

Unnecessary +1

> Source/WebCore/inspector/front-end/HeapSnapshot.js:816
> +        this._retainingNodes = new Uint32Array(totalEdgeCount);
> +        // Index of the first retainer in the _retainingNodes array. Addressed by retained node index.
> +        this._firstRetainerIndex = new Uint32Array(nodeCount);
> +        this._containmentEdges = new Uint32Array(totalEdgeCount * this._edgeFieldsCount);

I'd move allocations to the corresponding builders.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:841
> +        for (var newNodeIndex = 0; newNodeIndex < this._onlyNodes.length; newNodeIndex += this._nodeFieldCount) {
> +            var dominatorIndex = this._onlyNodes[newNodeIndex + this._dominatorOffset]

for (var newNodeDominatorIndex = this._dominatorOffset; newNodeDominatorIndex < this._onlyNodes.length; newNodeDominatorIndex += this._nodeFieldCount) {
    var dominatorIndex = this._onlyNodes[newNodeDominatorIndex];

> Source/WebCore/inspector/front-end/HeapSnapshot.js:843
> +            this._onlyNodes[newNodeIndex + this._dominatorOffset] = newDominatorIndex;

this._onlyNodes[newNodeDominatorIndex] = newDominatorIndex;

> Source/WebCore/inspector/front-end/HeapSnapshot.js:862
> +        for (var edgeDestinationIndex = this._edgeToNodeOffset; edgeDestinationIndex < this._containmentEdges.length; edgeDestinationIndex += this._edgeFieldsCount)
> +            ++this._firstRetainerIndex[edgeDestinationIndex];

wrong code.

> Source/WebCore/inspector/front-end/HeapSnapshot.js:871
> +        var nextNodeFirstEdgeIndex =  srcNodeIndex + this._edgesCountOffset;

unnecessary srcNodeIndex
Style: unnecessary space symbol

> Source/WebCore/inspector/front-end/HeapSnapshot.js:877
> +            var nextNodeFirstEdgeIndex  = nextNodeIndex < this._onlyNodes.length
> +                                        ? this._onlyNodes[nextNodeIndex + this._edgesCountOffset]
> +                                        : this._containmentEdges.length;

simpler version? : var nextNodeFirstEdgeIndex = this._onlyNodes[nextNodeIndex + this._edgesCountOffset] || this._containmentEdges.length

[Yury Semikhatsky]

(In reply to comment #6)
> (From update of attachment 133736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133736&action=review
> 
> please resort functions' bodies in order of invocations.
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:812
> > +        this._onlyNodes = new Uint32Array((nodeCount + 1) * this._nodeFieldCount);
> 
> Unnecessary +1
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:816
> > +        this._retainingNodes = new Uint32Array(totalEdgeCount);
> > +        // Index of the first retainer in the _retainingNodes array. Addressed by retained node index.
> > +        this._firstRetainerIndex = new Uint32Array(nodeCount);
> > +        this._containmentEdges = new Uint32Array(totalEdgeCount * this._edgeFieldsCount);
> 
> I'd move allocations to the corresponding builders.
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:841
> > +        for (var newNodeIndex = 0; newNodeIndex < this._onlyNodes.length; newNodeIndex += this._nodeFieldCount) {
> > +            var dominatorIndex = this._onlyNodes[newNodeIndex + this._dominatorOffset]
> 
> for (var newNodeDominatorIndex = this._dominatorOffset; newNodeDominatorIndex < this._onlyNodes.length; newNodeDominatorIndex += this._nodeFieldCount) {
>     var dominatorIndex = this._onlyNodes[newNodeDominatorIndex];
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:843
> > +            this._onlyNodes[newNodeIndex + this._dominatorOffset] = newDominatorIndex;
> 
> this._onlyNodes[newNodeDominatorIndex] = newDominatorIndex;
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:862
> > +        for (var edgeDestinationIndex = this._edgeToNodeOffset; edgeDestinationIndex < this._containmentEdges.length; edgeDestinationIndex += this._edgeFieldsCount)
> > +            ++this._firstRetainerIndex[edgeDestinationIndex];
> 
> wrong code.
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:871
> > +        var nextNodeFirstEdgeIndex =  srcNodeIndex + this._edgesCountOffset;
> 
> unnecessary srcNodeIndex
> Style: unnecessary space symbol
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:877
> > +            var nextNodeFirstEdgeIndex  = nextNodeIndex < this._onlyNodes.length
> > +                                        ? this._onlyNodes[nextNodeIndex + this._edgesCountOffset]
> > +                                        : this._containmentEdges.length;
> 
> simpler version? : var nextNodeFirstEdgeIndex = this._onlyNodes[nextNodeIndex + this._edgesCountOffset] || this._containmentEdges.length
No. It would be wrong because this._onlyNodes[nextNodeIndex + this._edgesCountOffset] may contain 0.

[Yury Semikhatsky]

Created attachment 133748 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 133748 [details]
Patch

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

lgtm with nits

> Source/WebCore/inspector/front-end/HeapSnapshot.js:861
> +                    var oldDestinationNodeIndex = this._containmentEdges[edgeArrayIndex];
> +                    var newDestinationNodeIndex = this._nodes[oldDestinationNodeIndex + this._nodeTypeOffset];

nit: oldToNodeIndex, newToNodeIndex

> Source/WebCore/inspector/front-end/HeapSnapshot.js:876
> +        for (var edgeDestinationIndex = this._edgeToNodeOffset; edgeDestinationIndex < this._containmentEdges.length; edgeDestinationIndex += this._edgeFieldsCount)

ditto

[Yury Semikhatsky]

Created attachment 133759 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 133748 [details])
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:861
> > +                    var oldDestinationNodeIndex = this._containmentEdges[edgeArrayIndex];
> > +                    var newDestinationNodeIndex = this._nodes[oldDestinationNodeIndex + this._nodeTypeOffset];
> 
> nit: oldToNodeIndex, newToNodeIndex
> 
Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:876
> > +        for (var edgeDestinationIndex = this._edgeToNodeOffset; edgeDestinationIndex < this._containmentEdges.length; edgeDestinationIndex += this._edgeFieldsCount)
> 
> ditto
Done.

[Yury Semikhatsky]

Committed r112071: <http://trac.webkit.org/changeset/112071>