Bug 82453 – Web Inspector: switch heap profiler front-end to separate storage of nodes and edges

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

[Yury Semikhatsky]

We are going to split snapshot into separate arrays of nodes and edges on the front-end. Then switch heap profiler front-end to the two new arrays. And after that the snapshot format is going to be changed so that the nodes and edges arrive in two separate arrays.

[Yury Semikhatsky]

Created attachment 134274 [details]
Working draft of the patch

[Yury Semikhatsky]

Created attachment 134316 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 134316 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        we may just delete node offset by number of node fields to get node index. After

divide

> Source/WebCore/inspector/front-end/HeapSnapshot.js:398
> +    var retainerIndexPosition = retainedNodeIndex / snapshot._nodeFieldCount;

retainedNodePosition?

> Source/WebCore/inspector/front-end/HeapSnapshot.js:667
> +        return new WebInspector.HeapSnapshotArraySlice(this._snapshot._containmentEdges, this._firstEdgeIndex(), this._afterLastEdgeIndex());

I would call the functions _beginEdgeIndex and _endEdgeIndex.

[Yury Semikhatsky]

---------- Forwarded message ----------
From: Alexei Filippov <[e-mail]>
Date: 2012/3/28
Subject: Re: [Bug 82453] Web Inspector: switch heap profiler front-end to separate storage of nodes and edges
To: [e-mail]
Cc: [e-mail]


It says that I'm not authorized to edit that attachment. Anyway here are my comments:

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

> Source/WebCore/ChangeLog:9
> +        we may just delete node offset by number of node fields to get node index. After

typo: *divide* node offset

> Source/WebCore/inspector/front-end/HeapSnapshot.js:398
> +    var retainerIndexPosition = retainedNodeIndex / snapshot._nodeFieldCount;

May be name it retainerOrdinal?

> Source/WebCore/inspector/front-end/HeapSnapshot.js:401
> +    if (retainerIndexPosition + 1 < snapshot._firstRetainerIndex.length)

We used to have an extra element in these arrays to avoid such conditionals. Why not to have one extra item in _firstRetainerIndex and set it to _retainingEdges.length. wdyt?

> Source/WebCore/inspector/front-end/HeapSnapshot.js:702
> +        return this._snapshot._onlyNodes[this.nodeIndex + this._snapshot._edgesCountOffset];

Why _edgesCountOffset? Do you use it to store first edge index. Can we rename it or at least make an alias.

[Yury Semikhatsky]

Created attachment 134500 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 134316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134316&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        we may just delete node offset by number of node fields to get node index. After
> 
> divide
> 
Fixed.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:398
> > +    var retainerIndexPosition = retainedNodeIndex / snapshot._nodeFieldCount;
> 
> retainedNodePosition?
> 
Renamed to retainedNodeOrdinal

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:667
> > +        return new WebInspector.HeapSnapshotArraySlice(this._snapshot._containmentEdges, this._firstEdgeIndex(), this._afterLastEdgeIndex());
> 
> I would call the functions _beginEdgeIndex and _endEdgeIndex.
Renamed to _edgeIndexesStart/End


(In reply to comment #4)
> ---------- Forwarded message ----------
> From: Alexei Filippov <[e-mail]>
> View in context: https://bugs.webkit.org/attachment.cgi?id=134316&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        we may just delete node offset by number of node fields to get node index. After
> 
> typo: *divide* node offset
> 
Fixed.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:398
> > +    var retainerIndexPosition = retainedNodeIndex / snapshot._nodeFieldCount;
> 
> May be name it retainerOrdinal?
>
Done. retainedNodeOrdinal
 
> > Source/WebCore/inspector/front-end/HeapSnapshot.js:401
> > +    if (retainerIndexPosition + 1 < snapshot._firstRetainerIndex.length)
> 
> We used to have an extra element in these arrays to avoid such conditionals. Why not to have one extra item in _firstRetainerIndex and set it to _retainingEdges.length. wdyt?
> 
Good point. Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:702
> > +        return this._snapshot._onlyNodes[this.nodeIndex + this._snapshot._edgesCountOffset];
> 
> Why _edgesCountOffset? Do you use it to store first edge index. Can we rename it or at least make an alias.

Yes, it is now also used to store first containment edge index. Will add an alias.

[Yury Semikhatsky]

Committed r112523: <http://trac.webkit.org/changeset/112523>