Introduce one way dependencies into object grouping.

Introduce one way dependencies into object grouping.

Those are necessary to properly manage relationship between objects in cases
when they don't form proper tree structure.

Committed: http://code.google.com/p/v8/source/detail?r=7202

[antonm, 2011-03-14 18:01:39]

Guys,

may you have a look?

I am esp. uneasy about all those testing utils in liveobjects and profiler.

[mnaganov (inactive), 2011-03-14 18:33:07]

On 2011/03/14 18:01:39, antonm wrote:
> Guys,
> 
> may you have a look?
> 
> I am esp. uneasy about all those testing utils in liveobjects and profiler.

As we have discussed offline, tagging all objects (object + dependents) with a
single RetainedObjectInfo (and thus, representing them as a single group to Heap
profiler) is a wrong approach.
More correct is to define 2 separate groups, and then specify an unidirectional
dependency between them. This way, they can be accurately rendered in Heap
profiler.

[antonm, 2011-03-14 20:17:21]

Sure, may you have another look?

I am not introducing structures for handles' collection (length, array and your
info object) yet, let's reserve it for a separate CL.

On 2011/03/14 18:33:07, Mikhail Naganov (Chromium) wrote:
> On 2011/03/14 18:01:39, antonm wrote:
> > Guys,
> > 
> > may you have a look?
> > 
> > I am esp. uneasy about all those testing utils in liveobjects and profiler.
> 
> As we have discussed offline, tagging all objects (object + dependents) with a
> single RetainedObjectInfo (and thus, representing them as a single group to
Heap
> profiler) is a wrong approach.
> More correct is to define 2 separate groups, and then specify an
unidirectional
> dependency between them. This way, they can be accurately rendered in Heap
> profiler.

[mnaganov (inactive), 2011-03-14 21:45:12]

Yes, this makes more sense.

I think, you can remove some duplication in code, if you store dependents as an
object group on its own, with a flag. In this case, the code that need to
process both ordinary groups and dependent groups will remain the same, and the
code that needs only ordinary groups can filter them by checking the flag.

Is this what you've meant by "I am not introducing structures for handles'
collection"?

On 2011/03/14 20:17:21, antonm wrote:
> Sure, may you have another look?
> 
> I am not introducing structures for handles' collection (length, array and
your
> info object) yet, let's reserve it for a separate CL.
> 
> On 2011/03/14 18:33:07, Mikhail Naganov (Chromium) wrote:
> > On 2011/03/14 18:01:39, antonm wrote:
> > > Guys,
> > > 
> > > may you have a look?
> > > 
> > > I am esp. uneasy about all those testing utils in liveobjects and
profiler.
> > 
> > As we have discussed offline, tagging all objects (object + dependents) with
a
> > single RetainedObjectInfo (and thus, representing them as a single group to
> Heap
> > profiler) is a wrong approach.
> > More correct is to define 2 separate groups, and then specify an
> unidirectional
> > dependency between them. This way, they can be accurately rendered in Heap
> > profiler.

[Mads Ager (chromium), 2011-03-15 08:32:28]

I haven't checked the details, but I'm OK with the overall approach of adding
one way dependencies in object grouping.

[antonm, 2011-03-15 18:25:46]

In offline discussion Vitaly and Misha suggested to switch to the different API.
 Thanks a lot, guys.

May you have a look?

[Vitaly Repeshko, 2011-03-15 18:45:35]

LGTM with the comments addressed.


-- Vitaly

http://codereview.chromium.org/6686053/diff/12003/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6686053/diff/12003/include/v8.h#newcode2727
include/v8.h:2727: static void AddImplicitRefGroup(Persistent<Object> parent,
I don't like the name for two reasons: "Ref" is an abbreviation and there's no
group here. As we discussed we should avoid any implications on the relations
between children. "AddImplicitReferences", "AddImplicitChildren"?

http://codereview.chromium.org/6686053/diff/12003/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6686053/diff/12003/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:1847: V8::AddImplicitRefGroup(g2s2, g2_children, 1);
It would be interesting to test longer chains of implicit references.

[antonm, 2011-03-15 19:33:12]

Thanks a lot for review, Vitaly.

I've added a test with a cycle.

Regarding the name.  I am not insisting on my name (although I think it's indeed
good enough).  I would only ask for something with a proper plural (I started
with AddImplicitReferences, but it's hard to make
GlobalHandles::ImplictReferenceses :).  Yes, set and all the stuff, but if
possible, I'd rather keep it parallel to object groups.

Mads, Misha, any ideas?

If not, ok, I'll switch back to AddImplicitReferences.