New GC APIs, try 2. (issue 14007008)

[ma...@chromium.org]

Reviewers: Sven Panne, Michael Starzinger,

Message:
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).

Description:
New GC APIs, try 2.

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

BUG=

Please review this at https://codereview.chromium.org/14007008/

SVN Base: git://github.com/v8/v8.git@master

Affected files:
M include/v8.h
M src/api.cc
M src/global-handles.h
M src/global-handles.cc
M src/heap-snapshot-generator.cc
M src/mark-compact.cc
M test/cctest/test-api.cc
M test/cctest/test-global-handles.cc
M test/cctest/test-heap-profiler.cc
M test/cctest/test-mark-compact.cc

[mstar...@chromium.org]

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#newcode37
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.

https://codereview.chromium.org/14007008/

[yu...@chromium.org]

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

https://codereview.chromium.org/14007008/

[yu...@chromium.org]

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?

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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#newcode37
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.

https://codereview.chromium.org/14007008/

[yu...@chromium.org]

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#newcode88
src/global-handles.h:88: // For internal bookeeping.
typo: bookkeeping

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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#newcode88
src/global-handles.h:88: // For internal bookeeping.

On 2013/04/22 14:39:54, Yury Semikhatsky wrote:
> typo: bookkeeping

Done.

https://codereview.chromium.org/14007008/

[yu...@chromium.org]

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?

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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.)

https://codereview.chromium.org/14007008/

[yu...@chromium.org]

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)?



https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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.

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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!

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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).

https://codereview.chromium.org/14007008/

[mstar...@chromium.org]

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#newcode442
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#newcode452
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#newcode914
src/global-handles.cc:914:
nit: Two empty new-lines between functions.

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

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#newcode442
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#newcode452
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#newcode914
src/global-handles.cc:914:

On 2013/04/24 13:58:56, Michael Starzinger wrote:
> nit: Two empty new-lines between functions.

Done.

https://codereview.chromium.org/14007008/

[yu...@chromium.org]

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-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/14007008/diff/51001/test/cctest/test-heap-profiler.cc#newcode1212
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?

https://codereview.chromium.org/14007008/

[ma...@chromium.org]

Thanks for review!

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

https://codereview.chromium.org/14007008/diff/51001/test/cctest/test-heap-profiler.cc#newcode1212
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.

https://codereview.chromium.org/14007008/

[mstar...@chromium.org]

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

https://codereview.chromium.org/14007008/