Allow collection of DOM objects in minor GC cycles.

Allow collection of DOM objects in minor GC cycles.

A design document: https://docs.google.com/a/google.com/document/d/16DeHrzkm3cO9XCPT1aK3Y5qgUxXB3RFmueqQWYmN2rI/edit

Performance & memory results: https://docs.google.com/a/google.com/document/d/1h0-EsHu7T0sSMuZm5eE0r1e8sCAzY3weLvsDUpOSngE/edit

The WebKit side patch: https://bugs.webkit.org/show_bug.cgi?id=98725

At present no one is using the V8 APIs this patch is going to add. After this patch is landed, the WebKit side patch will use it.

BUG=

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

[haraken, 2012-10-24 17:12:53]

Would you review the patch?

[abarth-chromium, 2012-10-24 17:35:00]

https://chromiumcodereview.appspot.com/11085015/diff/6001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11085015/diff/6001/include/v8.h#newcod...
include/v8.h:3510: >>>>>>> temp
Looks like you have some conflict markers in your patch.

[Michael Starzinger, 2012-10-26 14:16:47]

I just looked at the API side of this change so far, not at the GC
implementation. But so far it is looking good. A first round of minor comments.

As discussed offline, we will need to hold off from landing this a few days,
because bleeding edge is currently in a freeze until Chrome made the branch.

https://codereview.chromium.org/11085015/diff/9001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11085015/diff/9001/include/v8.h#newcode412
include/v8.h:412: */
Nit of the day: can we add asterisks to the beginning of each comment line?

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

https://codereview.chromium.org/11085015/diff/9001/src/global-handles.cc#newc...
src/global-handles.cc:71: independent_ = false;
We should be defensive an reset the externally_unreachble_ flag here.

https://codereview.chromium.org/11085015/diff/9001/src/global-handles.cc#newc...
src/global-handles.cc:91: independent_ = false;
The externally_unreachable_ flag needs to be initialized here.

https://codereview.chromium.org/11085015/diff/9001/src/global-handles.cc#newc...
src/global-handles.cc:566: if (!node->is_independent() &&
!node->is_externally_unreachable()) continue;
More than 80 characters, move the continue into the next line and add curly
brackets around it.

[haraken, 2012-10-30 08:51:17]

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

https://chromiumcodereview.appspot.com/11085015/diff/9001/include/v8.h#newcod...
include/v8.h:412: */
On 2012/10/26 14:16:48, Michael Starzinger wrote:
> Nit of the day: can we add asterisks to the beginning of each comment line?

Done.

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

https://chromiumcodereview.appspot.com/11085015/diff/9001/src/global-handles....
src/global-handles.cc:71: independent_ = false;
On 2012/10/26 14:16:48, Michael Starzinger wrote:
> We should be defensive an reset the externally_unreachble_ flag here.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/9001/src/global-handles....
src/global-handles.cc:91: independent_ = false;
On 2012/10/26 14:16:48, Michael Starzinger wrote:
> The externally_unreachable_ flag needs to be initialized here.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/9001/src/global-handles....
src/global-handles.cc:91: independent_ = false;
On 2012/10/26 14:16:48, Michael Starzinger wrote:
> The externally_unreachable_ flag needs to be initialized here.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/9001/src/global-handles....
src/global-handles.cc:566: if (!node->is_independent() &&
!node->is_externally_unreachable()) continue;
On 2012/10/26 14:16:48, Michael Starzinger wrote:
> More than 80 characters, move the continue into the next line and add curly
> brackets around it.

Done.

[haraken, 2012-11-05 06:55:22]

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1382: bool Heap::IterateObjectGroups(ObjectVisitor*
scavenge_visitor) {
Note for review: The logic of this method is copied from
MarkCompactCollector::MarkObjectGroups().

[Michael Starzinger, 2012-11-05 10:47:27]

This should be the final round of reviews, after that we should be able to land
it.

I am still a little bit worried about the quadratic worst case complexity due to
the fix-point iteration in the scavenger. But I currently don't know how we
could avoid that.

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h#newco...
include/v8.h:406: * Marks the reference to this object externally unreachable.
The comment should not describe the underlying implementation but the intended
usage of this flag. How about the following comment?

Marks the reference to this object partially dependent. Partially dependent
handles only depend on other partially dependent handles and these dependencies
are provided through object groups. It provides a way to build smaller object
groups for young objects that represent only a subset of all external
dependencies. This mark is automatically cleared after each garbage collection.

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h#newco...
include/v8.h:413: inline void MarkExternallyUnreachable();
As discussed offline: The name of this flag is not really accurate, because the
object is externally reachable and dependent on other objects. How about naming
it "partially dependent"?

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h#newco...
include/v8.h:3267: * wrapper objects.
Maybe we should add this comment here ...

Object groups for all dependent handles need to be provided for
kGCTypeMarkSweepCompact collections, for all other garbage collection types it
is sufficient to provide object groups for partially dependent handles only.

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

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.cc:161: if
(global_handles->isolate()->heap()->InNewSpace(object_))
Put curly brackets around the body of this if-statement.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles.h
File src/global-handles.h (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.h:201: // Finds weak independent handles satisfying the
callback predicate
s/independent/independent or partially dependent/

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.h:205: // Iterates over weak independent handles. See the
note above.
Likewise.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.h:210: // All groups are destroyed after a mark-compact
collection.
s/mark-compact collection/garbage collection/

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1335: while (1) {
Can we rewrite this to not be an infinite loop? I think this looks saner.

while (IterateObjectGroups(&scavenge_visitor)) {
  new_space_front = DoScavenge(&scavenge_visitor, new_space_front);
}

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1382: bool Heap::IterateObjectGroups(ObjectVisitor*
scavenge_visitor) {
On 2012/11/05 06:55:23, haraken1 wrote:
> Note for review: The logic of this method is copied from
> MarkCompactCollector::MarkObjectGroups().

Yes, I realized that. It would be cool to unify the logic to avoid duplication.
I already have an idea how to do that and we can do that in a follow-up CL. Can
you add a "// TODO(mstarzinger): ..." comment about that?

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1409: // An object in the group is marked, so mark as grey all white
heap
Can we drop the comment from this copy of the method, because marking and colors
do not apply to the scavenger.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1420: // to NULL so it won't be processed again.
Likewise.

[haraken, 2012-11-05 12:42:07]

All comments addressed.

> I am still a little bit worried about the quadratic worst case complexity due
to
> the fix-point iteration in the scavenger. But I currently don't know how we
> could avoid that.

One way to avoid it would be to simply give up the scavenger GC for DOM Nodes
after a predefined number of iterations. In that case, we can copy all DOM Nodes
to the to-space. I'm happy to implement the logic in a follow-up CL.

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h#newco...
include/v8.h:406: * Marks the reference to this object externally unreachable.
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> The comment should not describe the underlying implementation but the intended
> usage of this flag. How about the following comment?
> 
> Marks the reference to this object partially dependent. Partially dependent
> handles only depend on other partially dependent handles and these
dependencies
> are provided through object groups. It provides a way to build smaller object
> groups for young objects that represent only a subset of all external
> dependencies. This mark is automatically cleared after each garbage
collection.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h#newco...
include/v8.h:413: inline void MarkExternallyUnreachable();
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> As discussed offline: The name of this flag is not really accurate, because
the
> object is externally reachable and dependent on other objects. How about
naming
> it "partially dependent"?

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/include/v8.h#newco...
include/v8.h:3267: * wrapper objects.
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> Maybe we should add this comment here ...
> 
> Object groups for all dependent handles need to be provided for
> kGCTypeMarkSweepCompact collections, for all other garbage collection types it
> is sufficient to provide object groups for partially dependent handles only.

Done.

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

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.cc:161: if
(global_handles->isolate()->heap()->InNewSpace(object_))
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> Put curly brackets around the body of this if-statement.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles.h
File src/global-handles.h (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.h:201: // Finds weak independent handles satisfying the
callback predicate
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> s/independent/independent or partially dependent/

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.h:205: // Iterates over weak independent handles. See the
note above.
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/global-handles...
src/global-handles.h:210: // All groups are destroyed after a mark-compact
collection.
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> s/mark-compact collection/garbage collection/

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1335: while (1) {
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> Can we rewrite this to not be an infinite loop? I think this looks saner.
> 
> while (IterateObjectGroups(&scavenge_visitor)) {
>   new_space_front = DoScavenge(&scavenge_visitor, new_space_front);
> }

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1382: bool Heap::IterateObjectGroups(ObjectVisitor*
scavenge_visitor) {
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> On 2012/11/05 06:55:23, haraken1 wrote:
> > Note for review: The logic of this method is copied from
> > MarkCompactCollector::MarkObjectGroups().
> 
> Yes, I realized that. It would be cool to unify the logic to avoid
duplication.
> I already have an idea how to do that and we can do that in a follow-up CL.
Can
> you add a "// TODO(mstarzinger): ..." comment about that?

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1409: // An object in the group is marked, so mark as grey all white
heap
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> Can we drop the comment from this copy of the method, because marking and
colors
> do not apply to the scavenger.

Done.

https://chromiumcodereview.appspot.com/11085015/diff/13001/src/heap.cc#newcod...
src/heap.cc:1420: // to NULL so it won't be processed again.
On 2012/11/05 10:47:27, Michael Starzinger wrote:
> Likewise.

Done.

[Michael Starzinger, 2012-11-05 13:24:27]

LGTM. I'll land this soon.

[Michael Starzinger, 2012-11-06 17:33:16]

Slightly changed the title of this CL. Fixed a minor compilation failure.
Landed.

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

https://codereview.chromium.org/11085015/diff/20001/src/global-handles.cc#new...
src/global-handles.cc:166: bool clear_partially_dependent() {
partially_dependent_ = false; }
Had to change the return type of this method to void.