New GC APIs, try 2.

New GC APIs, try 2.

With these APIs, the embedder doesn't need to copy Persistent handles around.

BUG=

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

[marja, 2013-04-19 16:47:16]

mstarzinger, ptal.

Here's a rewritten version of the new GC APIs on the V8 side.

As a bonus, this is less obscure! Now the object group & implicit ref walking
(looping over different lists) is in one place and the users of GlobalHandles
don't need to care, they just get a nice intuitive representation.

The impl refs and the object groups need to be read at the same time, because we
normally want to ignore 1-object object groups (if we don't, the perf will be
bad), but they might have some impl refs connected in them.

Some perf results here: https://codereview.chromium.org/13975005/

Blink guys are willing to take the Blink side in (provided running dromaeo in
the chrome browser as opposed to DRT doesn't regress, but I believe we're fine
there).

[Michael Starzinger, 2013-04-22 11:34:51]

LGTM with two nits. I will land this for you as soon as the nits are addressed.
I reviewed the changes to the previous CL and rubber-stamped the rest as it was
already reviewed in https://codereview.chromium.org/14175005/ before.

https://codereview.chromium.org/14007008/diff/22001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/22001/include/v8.h#newcode147
include/v8.h:147: // Generic-purpose unique identifier.
nit: Can we turn that into a Doxygen comment (i.e. double-asterisk and
multi-line). Also two empty new-lines in front of the comment.

https://codereview.chromium.org/14007008/diff/22001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/14007008/diff/22001/src/global-handles.cc#new...
src/global-handles.cc:37: #define OBJECT_GROUP_CONNECTIONS_CAPACITY 20
Make this a "static const int kObjectGroupConntectionsCapacity" in the private
section within the GlobalHandles class.

[yurys, 2013-04-22 12:05:46]

https://codereview.chromium.org/14007008/diff/22001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/22001/include/v8.h#newcode3553
include/v8.h:3553: static void SetRetainedObjectInfo(Isolate* isolate,
As discussed offline, this method should be on v8::HeapProfiler

[yurys, 2013-04-22 12:10:43]

https://codereview.chromium.org/14007008/diff/22001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/22001/include/v8.h#newcode3549
include/v8.h:3549: static void SetObjectGroupId(Isolate* isolate,
Since these methods are new why not make them instance methods on Isolate?

[marja, 2013-04-22 13:49:06]

Thanks for comments!

I moved SetObjectGroupId and AddImplicitReference to Isolate, and
SetRetainedObjectInfo to HeapProfiler.

https://codereview.chromium.org/14007008/diff/22001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/22001/include/v8.h#newcode147
include/v8.h:147: // Generic-purpose unique identifier.
On 2013/04/22 11:34:51, Michael Starzinger wrote:
> nit: Can we turn that into a Doxygen comment (i.e. double-asterisk and
> multi-line). Also two empty new-lines in front of the comment.

Done.

https://codereview.chromium.org/14007008/diff/22001/include/v8.h#newcode3549
include/v8.h:3549: static void SetObjectGroupId(Isolate* isolate,
On 2013/04/22 12:10:43, Yury Semikhatsky wrote:
> Since these methods are new why not make them instance methods on Isolate?

Done.

https://codereview.chromium.org/14007008/diff/22001/include/v8.h#newcode3553
include/v8.h:3553: static void SetRetainedObjectInfo(Isolate* isolate,
On 2013/04/22 12:05:46, Yury Semikhatsky wrote:
> As discussed offline, this method should be on v8::HeapProfiler

Done.

https://codereview.chromium.org/14007008/diff/22001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/14007008/diff/22001/src/global-handles.cc#new...
src/global-handles.cc:37: #define OBJECT_GROUP_CONNECTIONS_CAPACITY 20
On 2013/04/22 11:34:51, Michael Starzinger wrote:
> Make this a "static const int kObjectGroupConntectionsCapacity" in the private
> section within the GlobalHandles class.

Done.

[yurys, 2013-04-22 14:39:54]

https://codereview.chromium.org/14007008/diff/29001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/29001/include/v8.h#newcode3158
include/v8.h:3158: void AddImplicitReference(UniqueId id,
Method name is a bit confusing. May be SetReferenceFromGroup? SetObjectGroupId
actually adds bidirectional reference while AddImplicitReference adds
unidirectional one. Method names should probably reflect this.

https://codereview.chromium.org/14007008/diff/29001/src/global-handles.h
File src/global-handles.h (right):

https://codereview.chromium.org/14007008/diff/29001/src/global-handles.h#newc...
src/global-handles.h:88: // For internal bookeeping.
typo: bookkeeping

[marja, 2013-04-23 12:00:04]

Name suggestions sg.

https://codereview.chromium.org/14007008/diff/29001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/29001/include/v8.h#newcode3158
include/v8.h:3158: void AddImplicitReference(UniqueId id,
On 2013/04/22 14:39:54, Yury Semikhatsky wrote:
> Method name is a bit confusing. May be SetReferenceFromGroup? SetObjectGroupId
> actually adds bidirectional reference while AddImplicitReference adds
> unidirectional one. Method names should probably reflect this.

Done.

https://codereview.chromium.org/14007008/diff/29001/src/global-handles.h
File src/global-handles.h (right):

https://codereview.chromium.org/14007008/diff/29001/src/global-handles.h#newc...
src/global-handles.h:88: // For internal bookeeping.
On 2013/04/22 14:39:54, Yury Semikhatsky wrote:
> typo: bookkeeping

Done.

[yurys, 2013-04-23 15:25:15]

https://codereview.chromium.org/14007008/diff/32001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/32001/include/v8.h#newcode3159
include/v8.h:3159: void SetReferenceFromGroup(UniqueId id,
I've just realized that this change will affect heap snapshots. Implicit
references are used to report links from DOM Node wrappers to event listeners
and with the current API when you take a heap snapshot you see which specific
node retains the listener. But with the new API you will see a link from a
random representative from the group to the event listener which may be
confusing. Can we change this method to accept "const Persistent<Value>&" as
source of the reference to supply exact element that retains the target?

[marja, 2013-04-23 16:08:52]

https://codereview.chromium.org/14007008/diff/32001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/14007008/diff/32001/include/v8.h#newcode3159
include/v8.h:3159: void SetReferenceFromGroup(UniqueId id,
On 2013/04/23 15:25:15, Yury Semikhatsky wrote:
> I've just realized that this change will affect heap snapshots. Implicit
> references are used to report links from DOM Node wrappers to event listeners
> and with the current API when you take a heap snapshot you see which specific
> node retains the listener. But with the new API you will see a link from a
> random representative from the group to the event listener which may be
> confusing. Can we change this method to accept "const Persistent<Value>&" as
> source of the reference to supply exact element that retains the target?

For implicit references which are not about event listeners: V8GCController (the
current impl) selects a "representative object" (which is basically arbitrary,
since it selects the first object, and the sorting doesn't specify order for 2
objects in the same color group) for each object group, and attaches the
implicit references to it (see the usage of m_rootGroupMap).

However, for "object and event listeners", we create a separate object group for
the object and attach implicit references to that group. So it should just work;
there's only 1 object in the object group. 

The object can then be part of another object group, nothing forbids that, but
for the group that references the event listeners, there is only a one-object
group. This group is then ignored by GlobalHandles, and only the impl references
are extracted, so the users of GlobalHandles don't even see it. From their point
of view, nothing has changed.

(This all is in the Blink side of the patch.)

[yurys, 2013-04-23 16:22:06]

On 2013/04/23 16:08:52, marja wrote:
> https://codereview.chromium.org/14007008/diff/32001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/14007008/diff/32001/include/v8.h#newcode3159
> include/v8.h:3159: void SetReferenceFromGroup(UniqueId id,
> On 2013/04/23 15:25:15, Yury Semikhatsky wrote:
> > I've just realized that this change will affect heap snapshots. Implicit
> > references are used to report links from DOM Node wrappers to event
listeners
> > and with the current API when you take a heap snapshot you see which
specific
> > node retains the listener. But with the new API you will see a link from a
> > random representative from the group to the event listener which may be
> > confusing. Can we change this method to accept "const Persistent<Value>&" as
> > source of the reference to supply exact element that retains the target?
> 
> For implicit references which are not about event listeners: V8GCController
(the
> current impl) selects a "representative object" (which is basically arbitrary,
> since it selects the first object, and the sorting doesn't specify order for 2
> objects in the same color group) for each object group, and attaches the
> implicit references to it (see the usage of m_rootGroupMap).
> 
> However, for "object and event listeners", we create a separate object group
for
> the object and attach implicit references to that group. So it should just
work;
> there's only 1 object in the object group. 
> 
> The object can then be part of another object group, nothing forbids that, but
> for the group that references the event listeners, there is only a one-object
> group. This group is then ignored by GlobalHandles, and only the impl
references
> are extracted, so the users of GlobalHandles don't even see it. From their
point
> of view, nothing has changed.
> 
> (This all is in the Blink side of the patch.)

But what will happen if the one-object group that references the event listeners
will have the same id as another group with more than 1 element (in case when
the object with event listeners is a root of some detached DOM tree for
example)?

[marja, 2013-04-23 16:35:30]

You're right, that case is broken.

So, I'd add an API for declaring an implicit reference from a specific object
(in addition to this API which declares an implicit reference from an object
group). But how to make it so that we don't need to copy Persistent...

I can't change the API they way you suggest, because that would require keeping
track of these "representative objects" for each group and that requires copying
Persistent.

[marja, 2013-04-23 16:37:37]

Hmmh, I'm overcomplicating things probably. It should be doable to add that API
and then just pass the Persistent as reference: when we're dealing with the
event listeners, V8 has just passed us the Persistent for the object, and we can
pass it back by reference, no problem.

yurys, thanks for finding the bug!

[marja, 2013-04-24 09:22:02]

I added an API for declaring references from an object too (not only from an
object group).

mstarzinger, svenpanne: ptal (see the delta in files v8.h, api.cc,
global-handles.*, test-heap-profiler.cc; other deltas are because of a rebase).

[Michael Starzinger, 2013-04-24 13:58:56]

LGTM with nits. I'll land this as soon as Yury gave his OK.

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc#new...
src/global-handles.cc:442: const int
GlobalHandles::kObjectGroupConnectionsCapacity = 20;
Can we move the value into the header? I don't think it has to belong to
particular unit of compilation for "static const" fields.

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc#new...
src/global-handles.cc:452:
object_group_connections_(kObjectGroupConnectionsCapacity) {}
The retainer_infos_ and the implicit_ref_connections_ list should be initialized
here explicitly as well.

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc#new...
src/global-handles.cc:914: 
nit: Two empty new-lines between functions.

[marja, 2013-04-24 14:05:42]

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc
File src/global-handles.cc (right):

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc#new...
src/global-handles.cc:442: const int
GlobalHandles::kObjectGroupConnectionsCapacity = 20;
On 2013/04/24 13:58:56, Michael Starzinger wrote:
> Can we move the value into the header? I don't think it has to belong to
> particular unit of compilation for "static const" fields.

Done.

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc#new...
src/global-handles.cc:452:
object_group_connections_(kObjectGroupConnectionsCapacity) {}
On 2013/04/24 13:58:56, Michael Starzinger wrote:
> The retainer_infos_ and the implicit_ref_connections_ list should be
initialized
> here explicitly as well.

Discussed offline -> no.

https://codereview.chromium.org/14007008/diff/39001/src/global-handles.cc#new...
src/global-handles.cc:914: 
On 2013/04/24 13:58:56, Michael Starzinger wrote:
> nit: Two empty new-lines between functions.

Done.

[yurys, 2013-04-24 14:21:24]

LGTM, I'd appreciate a new test for heap profiler that would cover references
reported using SetRerenceFromGroup.

https://codereview.chromium.org/14007008/diff/51001/test/cctest/test-heap-pro...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/14007008/diff/51001/test/cctest/test-heap-pro...
test/cctest/test-heap-profiler.cc:1212:
isolate_->SetReference(v8::Persistent<v8::Object>::Cast(objects_[0]),
Can you also add a test for the references provided via SetReferenceFromGroup
call from a one-object group to make sure heap profiler will handle it properly?

[marja, 2013-04-24 14:33:53]

Thanks for review!

https://codereview.chromium.org/14007008/diff/51001/test/cctest/test-heap-pro...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/14007008/diff/51001/test/cctest/test-heap-pro...
test/cctest/test-heap-profiler.cc:1212:
isolate_->SetReference(v8::Persistent<v8::Object>::Cast(objects_[0]),
On 2013/04/24 14:21:24, Yury Semikhatsky wrote:
> Can you also add a test for the references provided via SetReferenceFromGroup
> call from a one-object group to make sure heap profiler will handle it
properly?

Offline discussion: references from an one-object object group are converted
into the same format than references from an object.
test-global-handles/ImplicitReferences tests this. So from the heap profiler
point of view, it shouldn't make a difference. However, I made one of these
references to be from an one-object object group.

[Michael Starzinger, 2013-04-24 15:59:35]

Committed patchset #14 manually as r14423 (presubmit successful).