heap: allow allocation in gc prologue/epilogue

heap: allow allocation in gc prologue/epilogue

BUG=
R=hpayer@chromium.org, danno@chromium.org

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

[danno, 2014-02-25 15:53:39]

Hannes, can you please take a look at this for Fedor?

[Hannes Payer (out of office), 2014-02-26 09:01:37]

What is the motivation of that change?

[indutny, 2014-02-26 09:07:54]

On 2014/02/26 09:01:37, Hannes Payer wrote:
> What is the motivation of that change?

In the 3.22 version of v8 it was possible to perform allocations in GC epilogues
and prologues. The purpose of this patch is to preserve some API compatibility
in this place.

Here is an example of how we was using it in node.js:
https://github.com/joyent/node/blob/master/src/node_v8.cc#L116

[Hannes Payer (out of office), 2014-02-27 15:32:37]

This change is a bit scary. I am not a big fan of allocation in the GC prologue.
I guess it works if you make the callback not reentrant. If the callback would
allocate all the time, it would continuously trigger gc.

Can you provide a test case that demonstrates that allocation in the gc prologue
works?

[indutny, 2014-02-27 15:55:53]

On 2014/02/27 15:32:37, Hannes Payer wrote:
> This change is a bit scary. I am not a big fan of allocation in the GC
prologue.
> I guess it works if you make the callback not reentrant. If the callback would
> allocate all the time, it would continuously trigger gc.
> 
> Can you provide a test case that demonstrates that allocation in the gc
prologue
> works?

Perhaps, you are right. Do you think it'll be fine, if I'll remove
`AllowHeapAllocation` in prologue call and leave one in epilogue?

[Hannes Payer (out of office), 2014-03-03 15:20:12]

It would be nice to keep them in sync. Can you try to write a test case that
allocates in the prologue and in the epilogue to demonstrate that it works in a
non-reentrant implementation of the callback.

[indutny, 2014-03-05 10:48:49]

On 2014/03/03 15:20:12, Hannes Payer wrote:
> It would be nice to keep them in sync. Can you try to write a test case that
> allocates in the prologue and in the epilogue to demonstrate that it works in
a
> non-reentrant implementation of the callback.

Sure!

[indutny, 2014-03-05 11:23:10]

On 2014/03/05 10:48:49, indutny wrote:
> On 2014/03/03 15:20:12, Hannes Payer wrote:
> > It would be nice to keep them in sync. Can you try to write a test case that
> > allocates in the prologue and in the epilogue to demonstrate that it works
in
> a
> > non-reentrant implementation of the callback.
> 
> Sure!

Just pushed an update, not sure if it suffice, since the GC is triggered
manually, not because of allocation failure. What do you think?

[Hannes Payer (out of office), 2014-03-06 08:20:32]

I don't think this is testing the interesting case, i.e., triggering a gc in the
prologue/epilogue.
I think the test should do the following:
1. Create a full heap, e.g. new space
SimulateFullSpace(CcTest::heap()->new_space());
2. Allocate another object to trigger gc
3. Allocate an object in the prologue and epilogue which schould trigger another
gc. Make sure that the prologue and epilogue are not reentrant, otherwise you
would end up in an infinite series of gcs.

[indutny, 2014-03-06 12:12:39]

On 2014/03/06 08:20:32, Hannes Payer wrote:
> I don't think this is testing the interesting case, i.e., triggering a gc in
the
> prologue/epilogue.
> I think the test should do the following:
> 1. Create a full heap, e.g. new space
> SimulateFullSpace(CcTest::heap()->new_space());
> 2. Allocate another object to trigger gc
> 3. Allocate an object in the prologue and epilogue which schould trigger
another
> gc. Make sure that the prologue and epilogue are not reentrant, otherwise you
> would end up in an infinite series of gcs.

Hannes,

I see two solutions to this problem:

1) Do not invoke callbacks if GC is triggered from within them
2) Do not allow GC if it is triggered from callbacks

I chosen former one, because it seem to be safer. Is it fine? (Already uploaded
patch).

[indutny, 2014-03-10 20:37:14]

On 2014/03/06 12:12:39, indutny wrote:
> On 2014/03/06 08:20:32, Hannes Payer wrote:
> > I don't think this is testing the interesting case, i.e., triggering a gc in
> the
> > prologue/epilogue.
> > I think the test should do the following:
> > 1. Create a full heap, e.g. new space
> > SimulateFullSpace(CcTest::heap()->new_space());
> > 2. Allocate another object to trigger gc
> > 3. Allocate an object in the prologue and epilogue which schould trigger
> another
> > gc. Make sure that the prologue and epilogue are not reentrant, otherwise
you
> > would end up in an infinite series of gcs.
> 
> Hannes,
> 
> I see two solutions to this problem:
> 
> 1) Do not invoke callbacks if GC is triggered from within them
> 2) Do not allow GC if it is triggered from callbacks
> 
> I chosen former one, because it seem to be safer. Is it fine? (Already
uploaded
> patch).

Ping?

[Hannes Payer (out of office), 2014-03-11 10:57:19]

Looking good, I like it!
Additionally, could you please add a comment to the corresponding API calls
explaining that the callbacks are not re-entrant.

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

https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:18642: ASSERT(!obj.IsEmpty());
Can we call
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
at the end of this function, just to make sure that GC happens.

https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:18659: ASSERT(!obj.IsEmpty());
Can we call
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
at the end of this function, just to make sure that GC happens.

https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:18734:
CcTest::heap()->CollectAllGarbage(i::Heap::kNoGCFlags);
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
to make sure that GC gets triggerd if incremental marking is in progress. This
may happen on the stress gc builders.

[indutny, 2014-03-11 20:43:58]

On 2014/03/11 10:57:19, Hannes Payer wrote:
> Looking good, I like it!
> Additionally, could you please add a comment to the corresponding API calls
> explaining that the callbacks are not re-entrant.
> 
> https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
> test/cctest/test-api.cc:18642: ASSERT(!obj.IsEmpty());
> Can we call
>
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
> at the end of this function, just to make sure that GC happens.
> 
>
https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
> test/cctest/test-api.cc:18659: ASSERT(!obj.IsEmpty());
> Can we call
>
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
> at the end of this function, just to make sure that GC happens.
> 
>
https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
> test/cctest/test-api.cc:18734:
> CcTest::heap()->CollectAllGarbage(i::Heap::kNoGCFlags);
>
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
> to make sure that GC gets triggerd if incremental marking is in progress. This
> may happen on the stress gc builders.

Updated, thanks!

[indutny, 2014-03-13 16:16:42]

On 2014/03/11 20:43:58, indutny wrote:
> On 2014/03/11 10:57:19, Hannes Payer wrote:
> > Looking good, I like it!
> > Additionally, could you please add a comment to the corresponding API calls
> > explaining that the callbacks are not re-entrant.
> > 
> > https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc
> > File test/cctest/test-api.cc (right):
> > 
> >
>
https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
> > test/cctest/test-api.cc:18642: ASSERT(!obj.IsEmpty());
> > Can we call
> >
>
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
> > at the end of this function, just to make sure that GC happens.
> > 
> >
>
https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
> > test/cctest/test-api.cc:18659: ASSERT(!obj.IsEmpty());
> > Can we call
> >
>
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
> > at the end of this function, just to make sure that GC happens.
> > 
> >
>
https://codereview.chromium.org/177243012/diff/40001/test/cctest/test-api.cc#...
> > test/cctest/test-api.cc:18734:
> > CcTest::heap()->CollectAllGarbage(i::Heap::kNoGCFlags);
> >
>
CcTest::heap()->CollectAllGarbage(i::Heap::Heap::kAbortIncrementalMarkingMask);
> > to make sure that GC gets triggerd if incremental marking is in progress.
This
> > may happen on the stress gc builders.
> 
> Updated, thanks!

Ping?

[Hannes Payer (out of office), 2014-03-14 08:57:15]

LGTM, please fix the comments

https://codereview.chromium.org/177243012/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/177243012/diff/60001/include/v8.h#newcode4132
include/v8.h:4132: * trigger the Garbage Collection, the callback won't be
called again.
a garbage collection

https://codereview.chromium.org/177243012/diff/60001/include/v8.h#newcode4150
include/v8.h:4150: * trigger the Garbage Collection, the callback won't be
called again.
a garbage collection

[indutny, 2014-03-14 09:42:19]

On 2014/03/14 08:57:15, Hannes Payer wrote:
> LGTM, please fix the comments
> 
> https://codereview.chromium.org/177243012/diff/60001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/177243012/diff/60001/include/v8.h#newcode4132
> include/v8.h:4132: * trigger the Garbage Collection, the callback won't be
> called again.
> a garbage collection
> 
> https://codereview.chromium.org/177243012/diff/60001/include/v8.h#newcode4150
> include/v8.h:4150: * trigger the Garbage Collection, the callback won't be
> called again.
> a garbage collection

Thank you, done!

[Hannes Payer (out of office), 2014-03-17 10:38:58]

Committed patchset #5 manually as r19978 (presubmit successful).

[indutny, 2014-03-17 10:42:30]

On 2014/03/17 10:38:58, Hannes Payer wrote:
> Committed patchset #5 manually as r19978 (presubmit successful).

Yaaay! Thank you very much, any chance it could be backported to 3.24?