[WIP] New GC related APIs.

New GC related APIs.

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

BUG=NONE

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

[Sven Panne, 2013-04-09 11:12:45]

LGTM with a nit

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

https://codereview.chromium.org/13786002/diff/1/src/global-handles.h#newcode48
src/global-handles.h:48: public:
No need for "public" here...

[Michael Starzinger, 2013-04-09 14:18:40]

This is heading into the right direction. I have a few comments though. I held
off of nits and just added real comments for now. But from my end this needs
another round for nits.

https://codereview.chromium.org/13786002/diff/4001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13786002/diff/4001/include/v8.h#newcode3455
include/v8.h:3455: void* id);
Is void* the right type for this "color", would it be possible to use either
intptr_t or uintptr_t instead?

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

https://codereview.chromium.org/13786002/diff/4001/src/global-handles.cc#newc...
src/global-handles.cc:616: new_object_groups->Add(object_groups_->at(j));
It would be good if we could get by with an in-place version of this function as
before which doesn't require to copy into a new-list.

https://codereview.chromium.org/13786002/diff/4001/src/global-handles.cc#newc...
src/global-handles.cc:874: List<ObjectGroup*>* GlobalHandles::object_groups() {
This transferal of ownership of the structure looks mighty dangerous. Also I see
only one use-site of this method (i.e. heap snapshot creation). Could we just
use a second iterator that allows you to iterate over
<Object**,RetainedObjectInfo*> tuples instead of materializing this list here
just to delete it right after iteration again?

[marja, 2013-04-09 16:46:51]

Thanks for comments, uploaded a new version.

https://codereview.chromium.org/13786002/diff/4001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13786002/diff/4001/include/v8.h#newcode3455
include/v8.h:3455: void* id);
On 2013/04/09 14:18:40, Michael Starzinger wrote:
> Is void* the right type for this "color", would it be possible to use either
> intptr_t or uintptr_t instead?

Didn't change this yet... svenpanne@, what's your take (void* vs. intptr_t)?
Would it help if I rename "id" to color?

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

https://codereview.chromium.org/13786002/diff/4001/src/global-handles.cc#newc...
src/global-handles.cc:616: new_object_groups->Add(object_groups_->at(j));
On 2013/04/09 14:18:40, Michael Starzinger wrote:
> It would be good if we could get by with an in-place version of this function
as
> before which doesn't require to copy into a new-list.

Done.

https://codereview.chromium.org/13786002/diff/4001/src/global-handles.cc#newc...
src/global-handles.cc:874: List<ObjectGroup*>* GlobalHandles::object_groups() {
On 2013/04/09 14:18:40, Michael Starzinger wrote:
> This transferal of ownership of the structure looks mighty dangerous. Also I
see
> only one use-site of this method (i.e. heap snapshot creation). Could we just
> use a second iterator that allows you to iterate over
> <Object**,RetainedObjectInfo*> tuples instead of materializing this list here
> just to delete it right after iteration again?

I couldn't wrap my head around how to transfer the ownership of the Info in this
iterator-based approach, so I wrote a version which is closer to the existing
code (HeapSnapshotGenerator modifies directly the data structures of
GlobalHandles, and gets the Info, nullifies the pointer to it and so on). Wdyt?

[Sven Panne, 2013-04-10 08:31:46]

https://codereview.chromium.org/13786002/diff/4001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13786002/diff/4001/include/v8.h#newcode3455
include/v8.h:3455: void* id);
On 2013/04/09 16:46:51, marja wrote:
> On 2013/04/09 14:18:40, Michael Starzinger wrote:
> > Is void* the right type for this "color", would it be possible to use either
> > intptr_t or uintptr_t instead?
> 
> Didn't change this yet... svenpanne@, what's your take (void* vs. intptr_t)?
> Would it help if I rename "id" to color?

Any "foo*" has an implicit connotation of "I can be dereferenced somehow", OTOH
void* is the traditional way of passing "something I don't know" around. I'm a
bit torn here... We talked about another API change where we will need some kind
of unique identifier. Perhaps the cleanest way might be a separate class
"UniqueId", wrapping an intptr_t, with only some operators like == and < on it.
(Perhaps something like HashCode might be good, too, I don't know all use
cases.) Using such a class at least removes the need to comment the signature
here, just a "void* id" is a bit opaque.

[Sven Panne, 2013-04-10 08:37:49]

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

https://codereview.chromium.org/13786002/diff/11001/src/global-handles.h#newc...
src/global-handles.h:69: ~GroupRetainedObjectInfo();
Kill me! :-)

[Sven Panne, 2013-04-10 08:45:22]

https://codereview.chromium.org/13786002/diff/11001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/13786002/diff/11001/src/api.cc#newcode5789
src/api.cc:5789: if (IsDeadCheck(isolate, "v8::V8::SetObjectGroupId()")) return;
Remove the ASSERT and the IsDeadCheck, these are on death row for several
reasons already elsewhere, so there is no need to introduce them here,
especially not in performance-sensitive parts.

https://codereview.chromium.org/13786002/diff/11001/src/api.cc#newcode5800
src/api.cc:5800: if (IsDeadCheck(isolate, "v8::V8::SetRetainedObjectInfo()"))
return;
Remove ASSERT/IsDeadCheck here, too.

[marja, 2013-04-10 09:31:07]

Thanks for having a look.

All done + added the UniqueId class.

https://codereview.chromium.org/13786002/diff/11001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/13786002/diff/11001/src/api.cc#newcode5789
src/api.cc:5789: if (IsDeadCheck(isolate, "v8::V8::SetObjectGroupId()")) return;
On 2013/04/10 08:45:22, Sven Panne wrote:
> Remove the ASSERT and the IsDeadCheck, these are on death row for several
> reasons already elsewhere, so there is no need to introduce them here,
> especially not in performance-sensitive parts.

Done.

https://codereview.chromium.org/13786002/diff/11001/src/api.cc#newcode5800
src/api.cc:5800: if (IsDeadCheck(isolate, "v8::V8::SetRetainedObjectInfo()"))
return;
On 2013/04/10 08:45:22, Sven Panne wrote:
> Remove ASSERT/IsDeadCheck here, too.

Done.

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

https://codereview.chromium.org/13786002/diff/11001/src/global-handles.h#newc...
src/global-handles.h:69: ~GroupRetainedObjectInfo();
On 2013/04/10 08:37:49, Sven Panne wrote:
> Kill me! :-)

Done.

[Michael Starzinger, 2013-04-10 10:54:16]

LGTM with a final round of nits.

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

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:579: object_groups_.Sort();
Isn't there a sort call to the retained_object_infos list missing here?

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:585: int surviving_element_ix = 0;
nit: Can we spell out the "index" in these variables?

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:609: if (!group_should_be_visited) {
nit: Can we add an empty newline in front of this if, IMHO that makes all of
this easier to read.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:623: if (info_ix < retained_object_infos_.length() &&
Likewise for this if.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:853: ObjectGroupConnection(UniqueId((intptr_t)handles[0]),
handles[i]));
nit: Use "static_cast<intptr_t>" instead of the C-style cast.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:856:
GroupRetainedObjectInfo(UniqueId((intptr_t)handles[0]), info));
Likewise.

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

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.h#newc...
src/global-handles.h:66: struct GroupRetainedObjectInfo {
suggestion: We could call this struct either "ObjectGroupRetainerInfo" or just
"ObjectGroupInfo", thereby having a common prefix for everything related to
object groups (note that I am only talking about the internal naming, not the
API).

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.h#newc...
src/global-handles.h:224: void SetObjectGroupId(Object** handle,
nit: Fits into one line.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.h#newc...
src/global-handles.h:227: void SetRetainedObjectInfo(UniqueId id,
nit: Fits into one line.

[Sven Panne, 2013-04-10 10:58:52]

Almost there from my side... :-)

https://codereview.chromium.org/13786002/diff/19001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13786002/diff/19001/include/v8.h#newcode148
include/v8.h:148: UniqueId()
Do we need this? I can't imagine a use case, UniqueId is immutable, which is a
very good thing BTW.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:2500: V8::SetObjectGroupId(iso, g1s1, v8::UniqueId(1));
Style nit: Use "using".

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:2508:
HEAP->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
Use iso->heap() instead of HEAP here and below.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
File test/cctest/test-mark-compact.cc (right):

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
test/cctest/test-mark-compact.cc:432: GlobalHandles* global_handles =
Isolate::Current()->global_handles();
Use CcTest::isolate().

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
test/cctest/test-mark-compact.cc:435: v8::HandleScope
handle_scope(env->GetIsolate());
Same here.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
test/cctest/test-mark-compact.cc:438:
global_handles->Create(HEAP->AllocateFixedArray(1)->ToObjectChecked());
Use CcTest::isolate()->heap(), perhaps store this in a local variable. Same for
the other HEAPs below.

[marja, 2013-04-10 13:14:50]

Thanks for comments again!

https://codereview.chromium.org/13786002/diff/19001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/13786002/diff/19001/include/v8.h#newcode148
include/v8.h:148: UniqueId()
On 2013/04/10 10:58:52, Sven Panne wrote:
> Do we need this? I can't imagine a use case, UniqueId is immutable, which is a
> very good thing BTW.

Removed.

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

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:579: object_groups_.Sort();
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> Isn't there a sort call to the retained_object_infos list missing here?

Yes! Thanks for pointing that out.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:585: int surviving_element_ix = 0;
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> nit: Can we spell out the "index" in these variables?

Done.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:609: if (!group_should_be_visited) {
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> nit: Can we add an empty newline in front of this if, IMHO that makes all of
> this easier to read.

Done.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:623: if (info_ix < retained_object_infos_.length() &&
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> Likewise for this if.

Done.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:853: ObjectGroupConnection(UniqueId((intptr_t)handles[0]),
handles[i]));
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> nit: Use "static_cast<intptr_t>" instead of the C-style cast.

Done.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.cc#new...
src/global-handles.cc:856:
GroupRetainedObjectInfo(UniqueId((intptr_t)handles[0]), info));
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> Likewise.

Done.

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

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.h#newc...
src/global-handles.h:66: struct GroupRetainedObjectInfo {
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> suggestion: We could call this struct either "ObjectGroupRetainerInfo" or just
> "ObjectGroupInfo", thereby having a common prefix for everything related to
> object groups (note that I am only talking about the internal naming, not the
> API).

Renamed to ObjectGroupRetainerInfo.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.h#newc...
src/global-handles.h:224: void SetObjectGroupId(Object** handle,
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> nit: Fits into one line.

Done.

https://codereview.chromium.org/13786002/diff/19001/src/global-handles.h#newc...
src/global-handles.h:227: void SetRetainedObjectInfo(UniqueId id,
On 2013/04/10 10:54:16, Michael Starzinger wrote:
> nit: Fits into one line.

Done.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:2500: V8::SetObjectGroupId(iso, g1s1, v8::UniqueId(1));
On 2013/04/10 10:58:52, Sven Panne wrote:
> Style nit: Use "using".

Done.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-api.cc#n...
test/cctest/test-api.cc:2508:
HEAP->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
On 2013/04/10 10:58:52, Sven Panne wrote:
> Use iso->heap() instead of HEAP here and below.

Done.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
File test/cctest/test-mark-compact.cc (right):

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
test/cctest/test-mark-compact.cc:432: GlobalHandles* global_handles =
Isolate::Current()->global_handles();
On 2013/04/10 10:58:52, Sven Panne wrote:
> Use CcTest::isolate().

Done.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
test/cctest/test-mark-compact.cc:435: v8::HandleScope
handle_scope(env->GetIsolate());
On 2013/04/10 10:58:52, Sven Panne wrote:
> Same here.

Done.

https://codereview.chromium.org/13786002/diff/19001/test/cctest/test-mark-com...
test/cctest/test-mark-compact.cc:438:
global_handles->Create(HEAP->AllocateFixedArray(1)->ToObjectChecked());
On 2013/04/10 10:58:52, Sven Panne wrote:
> Use CcTest::isolate()->heap(), perhaps store this in a local variable. Same
for
> the other HEAPs below.

Done.

[Michael Starzinger, 2013-04-10 14:53:18]

Committed patchset #11 manually as r14215 (presubmit successful).