Changed scavenge GC to collect unmodified references

Changed scavenge GC to collect unmodified references

Added a scavenge GC pass that collects unmodified references instead of
processing object groups.  This mode can be controlled by setting
FLAG_scavenge_remove_unmodified_objects. By default this is turned off.
Also, modified a test case to suit the handle the new GC pass.

BUG=v8:4421
LOG=N

Committed: https://crrev.com/6254019238a853c9f3c09d615ba153043f6957c7
Cr-Commit-Position: refs/heads/master@{#31102}

[mythria, 2015-09-21 09:54:23]

mythria@chromium.org changed reviewers:
+ jochen@chromium.org, rmcilroy@chromium.org

[mythria, 2015-09-21 09:54:23]

Could you please look at global-handles.cc and review my changes.

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1358703003/diff/1/src/global-handles.cc#newco...
src/global-handles.cc:702: 
Here, I think node->location() is correct. One of the layout tests fails when I
use node->location(). When I pass &object the test is successful. I am not sure
if passing &object is correct.

[mythria, 2015-09-21 11:25:40]

On 2015/09/21 09:54:23, mythria wrote:
> Could you please look at global-handles.cc and review my changes.
> 
> Thanks and Regards,
> Mythri
> 
> https://codereview.chromium.org/1358703003/diff/1/src/global-handles.cc
> File src/global-handles.cc (right):
> 
>
https://codereview.chromium.org/1358703003/diff/1/src/global-handles.cc#newco...
> src/global-handles.cc:702: 
> Here, I think node->location() is correct. One of the layout tests fails when
I
> use node->location(). When I pass &object the test is successful. I am not
sure
> if passing &object is correct.

Just to clarify, I was not quite clear earlier. I uploaded this cl to get your
comments regarding the object pointer in global-handles.cc that I asked you on
Friday. Once, I fix it I will upload another cl for actual review. The current
one is not quite ready for the full review yet.

[jochen (gone - plz use gerrit), 2015-09-21 11:33:25]

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

https://codereview.chromium.org/1358703003/diff/1/src/global-handles.cc#newco...
src/global-handles.cc:702: 
On 2015/09/21 at 09:54:23, mythria wrote:
> Here, I think node->location() is correct. One of the layout tests fails when
I use node->location(). When I pass &object the test is successful. I am not
sure if passing &object is correct.

that's odd - object and *node->location() should be the same.

what's the failure output of the test?

maybe add a CHECK() here that object* and *node->location() are the same. At
least in theory, object is nothing the GC knows about, so if anything moves, it
wouldn't be updated.

https://codereview.chromium.org/1358703003/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1358703003/diff/1/src/heap/heap.cc#newcode1458
src/heap/heap.cc:1458: if
(!heap_object->GetIsolate()->heap()->InFromSpace(heap_object))
if (!IsUnscavengedHeapObject(..)) return true;

[rmcilroy, 2015-09-21 11:44:16]

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

https://codereview.chromium.org/1358703003/diff/1/src/global-handles.cc#newco...
src/global-handles.cc:702: 
One difference I can possibly think of is that VisitPointer would update the
pointer which is at the address passed to it when it moves the object from
From->To space. If we use &object then this is going to update the location on
the stack, and the handle will still point to the old location in the From space
won't it (if we use node->location() then it would update the location in the
handle to the To space)? I don't know much about the Scavenge visitor though so
I might be incorrect, also I'm not sure why this would cause the test to fail (I
would have thought the error would be the other way around where
node->location() would work but &object wouldn't).

[jochen (gone - plz use gerrit), 2015-09-21 11:54:19]

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

https://codereview.chromium.org/1358703003/diff/1/src/global-handles.cc#newco...
src/global-handles.cc:702: 
On 2015/09/21 at 11:44:15, rmcilroy wrote:
> One difference I can possibly think of is that VisitPointer would update the
pointer which is at the address passed to it when it moves the object from
From->To space. If we use &object then this is going to update the location on
the stack, and the handle will still point to the old location in the From space
won't it (if we use node->location() then it would update the location in the
handle to the To space)? I don't know much about the Scavenge visitor though so
I might be incorrect, also I'm not sure why this would cause the test to fail (I
would have thought the error would be the other way around where
node->location() would work but &object wouldn't).

right, the visitor indeed updates the passed in location, node->location() is
definitely correct.

https://codereview.chromium.org/1358703003/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1358703003/diff/1/src/heap/heap.cc#newcode1460
src/heap/heap.cc:1460: if (heap_object->map()->GetBackPointer()->IsUndefined())
return true;
maybe this is the wrong check? Try
heap_object->map()->GetConstructor()->initial_map() == heap_object->map()?

[mythria, 2015-09-22 10:45:08]

Hi Jochen,

I have the same behavior with the new check as well. I modified the file
heap/heap.cc for updating the check. Even now if I pass &object (i.e. address on
the stack) for the scavenge visitor in global-handles.cc the test passes. If I
pass node->location() the test fails. 

The test basically sets a property called foo to Hello and then calls minor GC
and checks to see if the property is maintained. Then it does the same test (it
again sets the property) by running full GC. The failed message is as follows:

FAIL getNodeInImport().foo should be Hello (of type String). Was undefined (of
type undefined).
PASS getNodeInImport().foo is "Hello"
PASS successfullyParsed is true

Can you give me any pointers on what could have gone wrong?

Thanks and Regards,
Mythri

https://codereview.chromium.org/1358703003/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1358703003/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1467: if (constructor->initial_map() == heap_object->map())
return true;
I modified the check and I still have the same problem. I am not quite sure,
what I should do with objects of type Name. I tried both visiting them and not
visiting them (returning true or false) but it does not change the behavior.

[mythria, 2015-09-22 10:57:00]

Hi,

I am not sure if this helps, but I just tried changing the way object group
visits the objects, by passing objects[j] which actually contains the pointer to
the object rather than sending the address on the stack (&object) and the test
fails too. This change is in global-handles.cc. I am not sure if this is related
to the problem with new scavenge but I just thought I will point it out. 

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1358703003/diff/40001/src/global-handles.cc#n...
src/global-handles.cc:735: v->VisitPointer(objects[j]);
If I pass objects[j] instead of &object for the visitor the test fails when I
use object groups for minor gc. I am not sure which one of these correct, but I
just tried to get more insight to debug the earlier problem

[mythria, 2015-09-25 17:23:51]

Hi Jochen,

Could you look at the changes in heap.cc and global-handles.cc and see if they
are Ok. This patch iterates over all weak nodes and marks the un-modified
objects as partially dependent. This lets other passes discard these nodes. I
did this because in the call IterateAllRoots, which is the first thing we do in
scavenge, it copies all strong and weak nodes that are not marked independent or
partially dependent to to_space. That means all the un-modified objects are
already in the to_space. It gets complicated to discard them later. 

I have also disabled object grouping in the minorGC call back in the webkits. I
will upload another cl for the same. 
With this all LayoutTests pass except for three which are image diffs. 

Just to note, both these cls are not ready for review yet, but just to get your
feedback if I am on the right path.

Thanks and Regards,
Mythri

https://codereview.chromium.org/1358703003/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1358703003/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1589:
isolate()->global_handles()->IdentifyWeakUnmodifiedObjects(
Before, we process anything we park the unmodified objects as partially
dependent. This allows the later passes to discard these objects. In the earlier
patches, we used to do this when we processed object groups. However, by that
time all weak nodes have been moved to to_space and that causes problems if we
want to discard them

[jochen (gone - plz use gerrit), 2015-09-28 11:26:04]

I'd prefer if we didn't use partial dependent information at all when your flag
is enabled.

What about introducing a new flag, and mark the objects that can be freed with
this new flag (say "unmodified_unreachable" or similar).

The when iterating all roots, just not copy the objects with that mark.

The code that looks at partial dependent nodes should be disabled when your flag
is set.

Also, instead of disabling the object grouping in blink, why not just not
invoking the gc epilogue/prologue callbacks for minor GCs if your flag is set?

[mythria, 2015-09-28 15:20:08]

Hi Jochen,

Thank you for your comments. I addressed both your comments. Now, I use a new
unmodified flag. I just added a new field to Node. flags_ field is already full
and I was not sure if it is better to extend it or add a new field. I also
disabled the GCPrologue and GCEpologue callbacks when the new scavenge
optimization is enabled. All Layout tests (except for two image diffs) pass with
this patch too. Can you have a look at these changes and tell me if they are Ok.
If you think it is ok, I will run performance tests. 

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:365: // Merge this with flags_ later on. May be we need to
extend flags_
I added a new field for the unmodified check.flags_ does not have any free bits
left. I am not sure if I should extend flags_ to 16 bits or just use a new
field.

[jochen (gone - plz use gerrit), 2015-09-28 15:56:19]

lgtm % nits

looking forward to the perf results

Did you get the image diffs locally? that might be just because of small
differences in your setup vs. the baseline...

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

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:365: // Merge this with flags_ later on. May be we need to
extend flags_
On 2015/09/28 at 15:20:08, mythria wrote:
> I added a new field for the unmodified check.flags_ does not have any free
bits left. I am not sure if I should extend flags_ to 16 bits or just use a new
field.

if the performance tests are looking good, we'll just rip the partially
dependent flag out...

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:700: void
GlobalHandles::IdentifyWeakUnmodifiedObjects(WeakSlotCallback f) {
nit. f -> is_unmodified()

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:711: WeakSlotCallbackWithHeap f) {
f -> is_unscavenged

[rmcilroy, 2015-09-28 17:32:47]

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

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:704: node->set_partially_dependent(true);
fix to set_unmodified(true);

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

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.h#ne...
src/global-handles.h:232: // independent
update comment please.

[mythria, 2015-10-02 15:29:16]

Hi,

I merged unmodified flag into flags_. Overloaded the partially_dependent flag to
mean unmodified depending on the FLAG_scavenge_reclaim_unmodified_objects.
Please review my changes.

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:700: void
GlobalHandles::IdentifyWeakUnmodifiedObjects(WeakSlotCallback f) {
On 2015/09/28 15:56:19, jochen wrote:
> nit. f -> is_unmodified()

Done.

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:704: node->set_partially_dependent(true);
On 2015/09/28 17:32:46, rmcilroy wrote:
> fix to set_unmodified(true);

Done.

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.cc#n...
src/global-handles.cc:711: WeakSlotCallbackWithHeap f) {
On 2015/09/28 15:56:19, jochen wrote:
> f -> is_unscavenged

Done.

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

https://codereview.chromium.org/1358703003/diff/80001/src/global-handles.h#ne...
src/global-handles.h:232: // independent
On 2015/09/28 17:32:46, rmcilroy wrote:
> update comment please.

Done.

[rmcilroy, 2015-10-02 18:09:47]

Lgtm once nits are addressed and assuming Jochen is still happy.

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc
File src/global-handles.cc (left):

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:657: 
Add back missing newline (2 newlines between functions in V8).

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

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:730: if (is_unmodified(node->location())) {
Nit - do both checks in the if above, I.e. - if (node->IsWeak() &&
is_unmodified(node->location())

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:738: void
GlobalHandles::MarkNewSpaceWeakUnModifiedObjectsPending(
Nit - replace UnModified with Unmodified (throughout cl)

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:840: // Skip dependent handles. Their weak callbacks might
expect to be
Nit, update comment (dependent or unmodified handles).

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1244: FLAG_scavenge_reclaim_unmodified_objects))
Nit - swap flag and GC_type check order.

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1331: FLAG_scavenge_reclaim_unmodified_objects))
Ditto

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1492: static bool IsUnModifiedNewSpaceObject(Object** p) {
This function doesn't seem to check if the object is in the new space. Just call
it IsUnmodifiedHeapObject() ?

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1504: return false;
Nit - just:

return constructor != nullptr && constructor->initial_map() ==
heap_object->map()

(Also replace all NULL with nullptr throughout CL).

[mythria, 2015-10-05 10:55:34]

Hi Jochen,

Fixed nits from Ross.If you have no further comments and if you think it is ok I
will land it :)

Thanks and Regards,
Mythri

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

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:730: if (is_unmodified(node->location())) {
On 2015/10/02 18:09:46, rmcilroy wrote:
> Nit - do both checks in the if above, I.e. - if (node->IsWeak() &&
> is_unmodified(node->location())

Done.

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:738: void
GlobalHandles::MarkNewSpaceWeakUnModifiedObjectsPending(
On 2015/10/02 18:09:46, rmcilroy wrote:
> Nit - replace UnModified with Unmodified (throughout cl)

Done.

https://codereview.chromium.org/1358703003/diff/120001/src/global-handles.cc#...
src/global-handles.cc:840: // Skip dependent handles. Their weak callbacks might
expect to be
On 2015/10/02 18:09:46, rmcilroy wrote:
> Nit, update comment (dependent or unmodified handles).

Done.

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1244: FLAG_scavenge_reclaim_unmodified_objects))
On 2015/10/02 18:09:46, rmcilroy wrote:
> Nit - swap flag and GC_type check order.

Done.

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1331: FLAG_scavenge_reclaim_unmodified_objects))
On 2015/10/02 18:09:46, rmcilroy wrote:
> Ditto

Done.

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1492: static bool IsUnModifiedNewSpaceObject(Object** p) {
On 2015/10/02 18:09:46, rmcilroy wrote:
> This function doesn't seem to check if the object is in the new space. Just
call
> it IsUnmodifiedHeapObject() ?

Done.

https://codereview.chromium.org/1358703003/diff/120001/src/heap/heap.cc#newco...
src/heap/heap.cc:1504: return false;
On 2015/10/02 18:09:46, rmcilroy wrote:
> Nit - just:
> 
> return constructor != nullptr && constructor->initial_map() ==
> heap_object->map()
> 
> (Also replace all NULL with nullptr throughout CL).

Done.

[jochen (gone - plz use gerrit), 2015-10-05 10:57:09]

lgtm

[mythria, 2015-10-05 15:59:07]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-10-05 15:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1358703003/#ps160001
(title: "Fixed another nit which I missed in last patch")

[commit-bot: I haz the power, 2015-10-05 15:59:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358703003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358703003/160001

[commit-bot: I haz the power, 2015-10-05 16:01:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-05 16:01:52]

Patchset 9 (id:??) landed as
https://crrev.com/6254019238a853c9f3c09d615ba153043f6957c7
Cr-Commit-Position: refs/heads/master@{#31102}

[Michael Achenbach, 2015-10-06 16:54:28]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1388133002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Speculative revert due to
crbug.com/539814.

[mythria, 2015-10-19 14:28:05]

Hi,

I fixed the problem with browser test. This patch checks if the object is active
or not. I also modified V8GCController on the blink side, to obtain this
information. I will upload the patch separately. I am not completely sure if the
checks are sufficient or if the checks are too strict. Please review the changes
and let me know if you think the changes are Ok. I started running dromaeo and
other performance benchmarks and I will update with the results. 

Thanks and Regards,
Mythri

[rmcilroy, 2015-10-19 15:52:03]

Description was changed from

==========
Changed scavenge GC to collect unmodified references

Added a scavenge GC pass that collects unmodified references instead of
processing object groups.  This mode can be controlled by setting
FLAG_scavenge_remove_unmodified_objects. By default this is turned off.
Also, modified a test case to suit the handle the new GC pass.

BUG=v8:4421
LOG=N

Committed: https://crrev.com/6254019238a853c9f3c09d615ba153043f6957c7
Cr-Commit-Position: refs/heads/master@{#31102}
==========

to

==========
Changed scavenge GC to collect unmodified references

Added a scavenge GC pass that collects unmodified references instead of
processing object groups.  This mode can be controlled by setting
FLAG_scavenge_remove_unmodified_objects. By default this is turned off.
Also, modified a test case to suit the handle the new GC pass.

BUG=v8:4421
LOG=N

Committed: https://crrev.com/6254019238a853c9f3c09d615ba153043f6957c7
Cr-Commit-Position: refs/heads/master@{#31102}
==========

[jochen (gone - plz use gerrit), 2015-10-20 09:34:32]

could you create a new CL please?