Retain maps for several garbage collections

Retain maps for several garbage collections

This keeps dying maps alive for FLAG_retain_maps_for_n_gc garbage collections to increase chances of them being reused for new objects in future.

BUG=v8:3664
LOG=N
TEST=cctest/test-heap/MapRetaining

Committed: https://crrev.com/2bc756e4b36160595c811c5433d0238ae5192525
Cr-Commit-Position: refs/heads/master@{#25887}

[ulan, 2014-12-15 15:46:43]

Patchset #1 (id:1) has been deleted

[ulan, 2014-12-15 15:46:48]

Patchset #1 (id:20001) has been deleted

[ulan, 2014-12-15 16:35:14]

ulan@chromium.org changed reviewers:
+ hpayer@chromium.org

[ulan, 2014-12-15 16:37:02]

PTAL

[Erik Corry, 2014-12-16 16:16:32]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2014-12-16 16:16:33]

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2134: // down to Map::kRetainingCounterStart. This
range can be narrowed
Comment looks wrong.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2269: RetainMaps(&root_visitor);
This should be after the ephemeral stuff.  Maps that are kept alive by
ephemerally-alive objects should not age.

But even better would be to put it inside ClearNonLiveReferences. That already
iterates over the map space.  It is better for the cache not to iterate multiple
times.

[ulan, 2014-12-16 16:35:23]

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2134: // down to Map::kRetainingCounterStart. This
range can be narrowed
On 2014/12/16 16:16:32, Erik Corry wrote:
> Comment looks wrong.

Thanks, that should be "down to Map::kRetainingCounterEnd", will fix.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2269: RetainMaps(&root_visitor);
On 2014/12/16 16:16:33, Erik Corry wrote:
> This should be after the ephemeral stuff.  Maps that are kept alive by
> ephemerally-alive objects should not age.
> 
> But even better would be to put it inside ClearNonLiveReferences. That already
> iterates over the map space.  It is better for the cache not to iterate
multiple
> times.

It should not be after ephemeral marking, because a resurrected map could mark
the key of an ephemeron, which would require marking the value of the ephemeron.

The most precise would be to put RetainMaps inside the loop in
ProcessEphemeralMarking, but that would be prohibitively expensive. So I had to
use imprecise (some live maps will age) but faster approach.

[Erik Corry, 2014-12-16 16:52:48]

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2159: ProcessMarkingDeque();
Can't we just process the marking deque at the end, instead of inside the loop?
By doing it here you may mark maps live that you haven't got to yet, and they
won't age, even though they are only reachable through maps that are ageing.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2269: RetainMaps(&root_visitor);
On 2014/12/16 16:35:23, ulan wrote:
> On 2014/12/16 16:16:33, Erik Corry wrote:
> > This should be after the ephemeral stuff.  Maps that are kept alive by
> > ephemerally-alive objects should not age.
> > 
> > But even better would be to put it inside ClearNonLiveReferences. That
already
> > iterates over the map space.  It is better for the cache not to iterate
> multiple
> > times.
> 
> It should not be after ephemeral marking, because a resurrected map could mark
> the key of an ephemeron, which would require marking the value of the
ephemeron.
> 
> The most precise would be to put RetainMaps inside the loop in
> ProcessEphemeralMarking, but that would be prohibitively expensive. So I had
to
> use imprecise (some live maps will age) but faster approach.

OK that makes sense in a sort of sad way.

[ulan, 2014-12-16 16:54:36]

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2159: ProcessMarkingDeque();
On 2014/12/16 16:52:48, Erik Corry wrote:
> Can't we just process the marking deque at the end, instead of inside the
loop?
> By doing it here you may mark maps live that you haven't got to yet, and they
> won't age, even though they are only reachable through maps that are ageing.

Good point, I will move it to the end.

[Hannes Payer (out of office), 2014-12-16 17:15:35]

About the tests: Instead of changing the old tests to FLAG_retain_maps_for_n_gc
= 0; wouldn't it make more sense to change the test semantics?

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2127: if (reduce_memory_footprint_ ||
abort_incremental_marking_ ||
Why do you bail out for abort_incremental_marking_?

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2161: if (counter < Map::kRetainingCounterStart) {
else if

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2269: RetainMaps(&root_visitor);
On 2014/12/16 16:35:23, ulan wrote:
> On 2014/12/16 16:16:33, Erik Corry wrote:
> > This should be after the ephemeral stuff.  Maps that are kept alive by
> > ephemerally-alive objects should not age.
> > 
> > But even better would be to put it inside ClearNonLiveReferences. That
already
> > iterates over the map space.  It is better for the cache not to iterate
> multiple
> > times.
> 
> It should not be after ephemeral marking, because a resurrected map could mark
> the key of an ephemeron, which would require marking the value of the
ephemeron.
> 
> The most precise would be to put RetainMaps inside the loop in
> ProcessEphemeralMarking, but that would be prohibitively expensive. So I had
to
> use imprecise (some live maps will age) but faster approach.

Please write a comment about that somewhere.

https://codereview.chromium.org/794583003/diff/60001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/794583003/diff/60001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:4955: TEST(MapRetaining) {
Please also test the flag for n == 0, n in between start and end, and n outside
start and end.

[ulan, 2014-12-17 11:13:34]

> About the tests: Instead of changing the old tests to
FLAG_retain_maps_for_n_gc = 0; wouldn't it make more sense to change the test
semantics?
Most of them test for leaks, so we would need to do |FLAG_retain_maps_for_n_gc|
number of GCs instead of one GC. TEST(MapRetaining) checks that there are no
leaks introduced by this CL, and I thought that justifies usage of
FLAG_retain_maps_for_n_gc = 0.

I can change the tests to run |FLAG_retain_maps_for_n_gc| GC if you think that
would be better (I don't have preference).

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2127: if (reduce_memory_footprint_ ||
abort_incremental_marking_ ||
On 2014/12/16 17:15:35, Hannes Payer wrote:
> Why do you bail out for abort_incremental_marking_?

The incremental marking is aborted for GC requested explicitly by tests and
dev-tools. In that cases we want to be precise. I added a comment in the code.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2134: // down to Map::kRetainingCounterStart. This
range can be narrowed
On 2014/12/16 16:16:32, Erik Corry wrote:
> Comment looks wrong.

Done.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2159: ProcessMarkingDeque();
On 2014/12/16 16:52:48, Erik Corry wrote:
> Can't we just process the marking deque at the end, instead of inside the
loop?
> By doing it here you may mark maps live that you haven't got to yet, and they
> won't age, even though they are only reachable through maps that are ageing.

Done.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2161: if (counter < Map::kRetainingCounterStart) {
On 2014/12/16 17:15:35, Hannes Payer wrote:
> else if

Done.

https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc...
src/heap/mark-compact.cc:2269: RetainMaps(&root_visitor);
On 2014/12/16 17:15:35, Hannes Payer wrote:
> On 2014/12/16 16:35:23, ulan wrote:
> > On 2014/12/16 16:16:33, Erik Corry wrote:
> > > This should be after the ephemeral stuff.  Maps that are kept alive by
> > > ephemerally-alive objects should not age.
> > > 
> > > But even better would be to put it inside ClearNonLiveReferences. That
> already
> > > iterates over the map space.  It is better for the cache not to iterate
> > multiple
> > > times.
> > 
> > It should not be after ephemeral marking, because a resurrected map could
mark
> > the key of an ephemeron, which would require marking the value of the
> ephemeron.
> > 
> > The most precise would be to put RetainMaps inside the loop in
> > ProcessEphemeralMarking, but that would be prohibitively expensive. So I had
> to
> > use imprecise (some live maps will age) but faster approach.
> 
> Please write a comment about that somewhere.

Done.

https://codereview.chromium.org/794583003/diff/60001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/794583003/diff/60001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:4955: TEST(MapRetaining) {
On 2014/12/16 17:15:35, Hannes Payer wrote:
> Please also test the flag for n == 0, n in between start and end, and n
outside
> start and end.

Done.

[Hannes Payer (out of office), 2014-12-17 18:45:49]

OK, don't change the test. It should be fine.

LGTM from my side, just add the comment about the impreciseness.

[Erik Corry, 2014-12-18 11:25:19]

LGTM

However, I think this change would be even better if the aging was done as an
incremental task, instead of being put in the atomic GC pause, which we are
trying to reduce.

[Hannes Payer (out of office), 2014-12-18 12:15:23]

good point erik.

[ulan, 2014-12-18 15:16:54]

> just add the comment about the impreciseness.
Done.


> However, I think this change would be even better if the aging was done as an
> incremental task, instead of being put in the atomic GC pause, which we are
> trying to reduce.
I agree. I am thinking about introducing additional marking state in incremental
marking and retaining maps during transition between MARKING1 -> MARKING2
states.

[ulan, 2014-12-18 15:17:17]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2014-12-18 15:18:00]

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

[commit-bot: I haz the power, 2014-12-18 15:44:25]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2014-12-18 18:05:37]

Patchset 7 (id:??) landed as
https://crrev.com/2bc756e4b36160595c811c5433d0238ae5192525
Cr-Commit-Position: refs/heads/master@{#25887}