Split nodes and edges into separate arrays in heap snapshot serialization.

Split nodes and edges into separate arrays in heap snapshot serialization.

Committed: https://code.google.com/p/v8/source/detail?r=11315

[alexeif, 2012-04-10 16:08:09]

Please take a look at the backend part.

[mnaganov (inactive), 2012-04-10 22:28:09]

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3621: writer_->AddString(JSON_O(
Is this object still the first item of 'nodes' array? This is asymmetric to the
absence of such meta-information in 'edges' array. Perhaps, we should reserve
the first item of 'edges' for this data? This will also eliminate the special
case of the item 0. It is not necessary to split the meta-data in this
changelist.

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3624: JSON_S("type") ","
Why are you moving commas to the suffix position? I supposed, having them as a
prefix makes you only to change a single line when adding / removing a field.

https://chromiumcodereview.appspot.com/10037004/diff/2001/test/cctest/test-he...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/2001/test/cctest/test-he...
test/cctest/test-heap-profiler.cc:632: "parsed.nodes = parsed.nodes.slice(1);\n"
Why is this needed?

[yurys, 2012-04-11 05:20:05]

http://codereview.chromium.org/10037004/diff/2001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/10037004/diff/2001/src/profile-generator.cc#ne...
src/profile-generator.cc:3477: List<HeapEntry*>& nodes =
*(snapshot_->entries());
Consider extracting this loop in a separate method.

http://codereview.chromium.org/10037004/diff/2001/src/profile-generator.cc#ne...
src/profile-generator.cc:3621: writer_->AddString(JSON_O(
On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> Is this object still the first item of 'nodes' array? This is asymmetric to
the
> absence of such meta-information in 'edges' array. Perhaps, we should reserve
> the first item of 'edges' for this data? This will also eliminate the special
> case of the item 0. It is not necessary to split the meta-data in this
> changelist.

I'd rather put the array length at the first item in both cases. This way we can
preallocate array of required size from the very beginning.

[loislo, 2012-04-11 07:40:17]

On 2012/04/11 05:20:05, Yury Semikhatsky wrote:
> http://codereview.chromium.org/10037004/diff/2001/src/profile-generator.cc
> File src/profile-generator.cc (right):
> 
>
http://codereview.chromium.org/10037004/diff/2001/src/profile-generator.cc#ne...
> src/profile-generator.cc:3477: List<HeapEntry*>& nodes =
> *(snapshot_->entries());
> Consider extracting this loop in a separate method.
> 
>
http://codereview.chromium.org/10037004/diff/2001/src/profile-generator.cc#ne...
> src/profile-generator.cc:3621: writer_->AddString(JSON_O(
> On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> > Is this object still the first item of 'nodes' array? This is asymmetric to
> the
> > absence of such meta-information in 'edges' array. Perhaps, we should
reserve
> > the first item of 'edges' for this data? This will also eliminate the
special
> > case of the item 0. It is not necessary to split the meta-data in this
> > changelist.
> 
> I'd rather put the array length at the first item in both cases. This way we
can
> preallocate array of required size from the very beginning.

I agree with Yury's opinion about dropping meta info from the list of nodes.
But I'm against the idea of using zero element for length.
I think it'd be better to put this into TakeSnapshot response as I did for
maxJSObjectId field.

[alexeif, 2012-04-11 12:49:57]

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3477: List<HeapEntry*>& nodes =
*(snapshot_->entries());
On 2012/04/11 05:20:05, Yury Semikhatsky wrote:
> Consider extracting this loop in a separate method.

Done.

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3621: writer_->AddString(JSON_O(
On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> Is this object still the first item of 'nodes' array? This is asymmetric to
the
> absence of such meta-information in 'edges' array. Perhaps, we should reserve
> the first item of 'edges' for this data? This will also eliminate the special
> case of the item 0. It is not necessary to split the meta-data in this
> changelist.
moved meta info to the snapshot object.

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3621: writer_->AddString(JSON_O(
On 2012/04/11 05:20:05, Yury Semikhatsky wrote:
> On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> > Is this object still the first item of 'nodes' array? This is asymmetric to
> the
> > absence of such meta-information in 'edges' array. Perhaps, we should
reserve
> > the first item of 'edges' for this data? This will also eliminate the
special
> > case of the item 0. It is not necessary to split the meta-data in this
> > changelist.
> 
> I'd rather put the array length at the first item in both cases. This way we
can
> preallocate array of required size from the very beginning.
Added node and edges count to the snapshot info.

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3624: JSON_S("type") ","
On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> Why are you moving commas to the suffix position? I supposed, having them as a
> prefix makes you only to change a single line when adding / removing a field.
Unless this it the first field. :-)
So in this sense none of prefix and postfix commas wins. But IMO the postfix
commas are more readable because fields of the same object have the same
padding.

https://chromiumcodereview.appspot.com/10037004/diff/2001/test/cctest/test-he...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/2001/test/cctest/test-he...
test/cctest/test-heap-profiler.cc:632: "parsed.nodes = parsed.nodes.slice(1);\n"
On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> Why is this needed?
it's not needed anymore. I removed the meta node from nodes array.

[yurys, 2012-04-11 13:07:17]

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3621: writer_->AddString(JSON_O(
On 2012/04/11 12:49:57, alexeif wrote:
> On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> > Is this object still the first item of 'nodes' array? This is asymmetric to
> the
> > absence of such meta-information in 'edges' array. Perhaps, we should
reserve
> > the first item of 'edges' for this data? This will also eliminate the
special
> > case of the item 0. It is not necessary to split the meta-data in this
> > changelist.
> moved meta info to the snapshot object.

What about the front-end part?

[alexeif, 2012-04-11 13:10:23]

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
File src/profile-generator.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/2001/src/profile-generat...
src/profile-generator.cc:3621: writer_->AddString(JSON_O(
On 2012/04/11 13:07:17, Yury Semikhatsky wrote:
> On 2012/04/11 12:49:57, alexeif wrote:
> > On 2012/04/10 22:28:09, Mikhail Naganov (Chromium) wrote:
> > > Is this object still the first item of 'nodes' array? This is asymmetric
to
> > the
> > > absence of such meta-information in 'edges' array. Perhaps, we should
> reserve
> > > the first item of 'edges' for this data? This will also eliminate the
> special
> > > case of the item 0. It is not necessary to split the meta-data in this
> > > changelist.
> > moved meta info to the snapshot object.
> 
> What about the front-end part?
It will follow as soon as we agree on the BE part.

[mnaganov (inactive), 2012-04-11 14:50:51]

Overall looks good, but I have a question.

https://chromiumcodereview.appspot.com/10037004/diff/9001/test/cctest/test-he...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/9001/test/cctest/test-he...
test/cctest/test-heap-profiler.cc:632: "parsed.nodes.concat(0, 0, 0, 0, 0, 0,
parsed.edges.length);");
And why do we need this line?

[alexeif, 2012-04-11 15:04:54]

https://chromiumcodereview.appspot.com/10037004/diff/9001/test/cctest/test-he...
File test/cctest/test-heap-profiler.cc (right):

https://chromiumcodereview.appspot.com/10037004/diff/9001/test/cctest/test-he...
test/cctest/test-heap-profiler.cc:632: "parsed.nodes.concat(0, 0, 0, 0, 0, 0,
parsed.edges.length);");
On 2012/04/11 14:50:51, Mikhail Naganov (Chromium) wrote:
> And why do we need this line?
We use it to iterate over edges. look at the line 642. we take edges_index for
the node and for its immediate successor to get the edges interval. So we need
one extra node that points to the end of array, so we can process edges for the
very last node correctly. I believe you used this technique in
dominators/retainers building in FE.

[mnaganov (inactive), 2012-04-11 17:57:44]

LGTM

Please, update the comment in include/v8-profiler.h regarding the JSON
structure.

[yurys, 2012-04-12 11:55:17]

lgtm