Implement VisitHandlesInNewSpaceWithClassIds()

Implement VisitHandlesInNewSpaceWithClassIds()

BUG=
TEST=test-api.cc::PersistentHandleInNewSpaceVisitor

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

[haraken, 2012-11-07 15:24:38]

Currently V8 binding keeps "a list of wrappers probably existing in the new
space", which is iterated in minorGCPrologue(). To remove the list from V8
binding, this patch implements VisitHandlesInNewSpaceWithClassIds().

Could you take a look?

[Michael Starzinger, 2013-01-17 09:57:42]

First round of comments. My issues are mainly with the (long-lasting) promise
the API provides with that function, not so much with the actual (current)
implements.

I also CCed Sven, he might have further comments.

https://chromiumcodereview.appspot.com/11365131/diff/1/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11365131/diff/1/include/v8.h#newcode3497
include/v8.h:3497: * Iterates through all the persistent handles in the new
space of the current
The API doesn't have a notion of generational garbage collection (except in
GCType) and I am hesitant toward adding it.

How about moving the promise of this function towards providing "candidates for
independence/partially dependence". This would make it clear that this list is
to be used in sync with MarkIndependent() and MarkPartiallyDependent(). To get
more concrete, here is my suggestion for a method name and comment:

/**
 * Iterates through all the persistent handles in the current isolate's heap
 * that have class_ids and are candidates to be marked as partially dependent
 * handles. This will visit handles to young objects created since the last
 * garbage collection but is free to visit an arbitrary superset of these
 * objects.
 */
static void VisitHandlesForPartialDependence(PersistentHandleVisitor* visitor);

[haraken, 2013-01-17 11:59:21]

Thanks for reviewing! I updated a patch with michael's comment.

Sven: Could you take a look?

[Sven Panne, 2013-01-18 07:50:27]

https://chromiumcodereview.appspot.com/11365131/diff/2002/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11365131/diff/2002/include/v8.h#newcod...
include/v8.h:3495: static void VisitHandlesForPartialDependence(
Let's add a mandatory Isolate parameter as the first argument right away, this
will save us one round of deprecations.

[haraken, 2013-01-18 07:57:18]

Thanks for reviewing! Updated the patch.

https://chromiumcodereview.appspot.com/11365131/diff/2002/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11365131/diff/2002/include/v8.h#newcod...
include/v8.h:3495: static void VisitHandlesForPartialDependence(
On 2013/01/18 07:50:27, Sven Panne wrote:
> Let's add a mandatory Isolate parameter as the first argument right away, this
> will save us one round of deprecations.

Done.

[Sven Panne, 2013-01-18 08:14:30]

https://chromiumcodereview.appspot.com/11365131/diff/9001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11365131/diff/9001/include/v8.h#newcod...
include/v8.h:3495: static void VisitHandlesForPartialDependence(Isolate*
isolate,
Formatting nit: This should be on the next line.

https://chromiumcodereview.appspot.com/11365131/diff/9001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/11365131/diff/9001/src/api.cc#newcode4527
src/api.cc:4527: void v8::V8::VisitHandlesForPartialDependence(Isolate*
exported_isolate,
Again: Formatting nit, move this to the next line.

https://chromiumcodereview.appspot.com/11365131/diff/9001/src/api.cc#newcode4530
src/api.cc:4530: ASSERT(isolate == i::Isolate::Current());
I am not totally sure if this assertion is needed. Michael?

[haraken, 2013-01-18 08:18:46]

Updated the patch.

https://chromiumcodereview.appspot.com/11365131/diff/9001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11365131/diff/9001/include/v8.h#newcod...
include/v8.h:3495: static void VisitHandlesForPartialDependence(Isolate*
isolate,
On 2013/01/18 08:14:30, Sven Panne wrote:
> Formatting nit: This should be on the next line.

Done.

https://chromiumcodereview.appspot.com/11365131/diff/9001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/11365131/diff/9001/src/api.cc#newcode4527
src/api.cc:4527: void v8::V8::VisitHandlesForPartialDependence(Isolate*
exported_isolate,
On 2013/01/18 08:14:30, Sven Panne wrote:
> Again: Formatting nit, move this to the next line.

Done.

https://chromiumcodereview.appspot.com/11365131/diff/9001/src/api.cc#newcode4530
src/api.cc:4530: ASSERT(isolate == i::Isolate::Current());
FYI, we've been adding this check for Isolate-version APIs.

[Michael Starzinger, 2013-01-24 15:06:56]

LGTM. Just one more comment that I'll address while landing. I'll land this.

https://chromiumcodereview.appspot.com/11365131/diff/18001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/11365131/diff/18001/src/api.cc#newcode...
src/api.cc:4503: : visitor_(visitor) {}
Indentation is off.

https://chromiumcodereview.appspot.com/11365131/diff/18001/src/global-handles.cc
File src/global-handles.cc (right):

https://chromiumcodereview.appspot.com/11365131/diff/18001/src/global-handles...
src/global-handles.cc:727: if (node->has_wrapper_class_id() &&
node->IsRetainer()) {
Flip the two operands of the conjunction, otherwise ASAN will complain that we
access an uninitialized class-id. We had this "bug" before.