Bug 82453 - Web Inspector: switch heap profiler front-end to separate storage of nodes and edges
Summary: Web Inspector: switch heap profiler front-end to separate storage of nodes an...
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: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks: 81927
  Show dependency treegraph
 
Reported: 2012-03-28 05:42 PDT by Yury Semikhatsky
Modified: 2012-03-29 05:39 PDT (History)
11 users (show)

See Also:


Attachments
Working draft of the patch (25.01 KB, patch)
2012-03-28 05:44 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (38.92 KB, patch)
2012-03-28 09:35 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (38.96 KB, patch)
2012-03-28 22:38 PDT, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-03-28 05:42:12 PDT
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.
Comment 1 Yury Semikhatsky 2012-03-28 05:44:00 PDT
Created attachment 134274 [details]
Working draft of the patch
Comment 2 Yury Semikhatsky 2012-03-28 09:35:41 PDT
Created attachment 134316 [details]
Patch
Comment 3 Ilya Tikhonovsky 2012-03-28 10:49:37 PDT
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.
Comment 4 Yury Semikhatsky 2012-03-28 21:59:03 PDT
---------- Forwarded message ----------
From: Alexei Filippov <alexeif@google.com>
Date: 2012/3/28
Subject: Re: [Bug 82453] Web Inspector: switch heap profiler front-end to separate storage of nodes and edges
To: bugzilla-daemon@webkit.org
Cc: alexeif@chromium.org


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.
Comment 5 Yury Semikhatsky 2012-03-28 22:38:30 PDT
Created attachment 134500 [details]
Patch
Comment 6 Yury Semikhatsky 2012-03-28 22:41:43 PDT
(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 <alexeif@google.com>
> 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.
Comment 7 Yury Semikhatsky 2012-03-29 05:39:03 PDT
Committed r112523: <http://trac.webkit.org/changeset/112523>