Oilpan: Allocation should be allowed in pre-finalizers

Oilpan: Allocation should be allowed in pre-finalizers

There is no reason we need to forbid allocations during pre-finalizers.
Pages to be swept are already linked in the list of m_firstUnsweptPage,
and thus pages allocated during pre-finalizers will just go to a list of
m_firstPage. No issue here.

The only substantial change in this CL is that NoAllocationScope is removed
from invokePrefinalizer's scope.

BUG=420515
TEST=HeapTest.AllocationDuringPrefinalizer

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197399

[haraken, 2015-06-17 23:12:36]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2015-06-17 23:12:37]

PTAL

[sof, 2015-06-18 06:41:00]

sigbjornf@opera.com changed reviewers:
+ tkent@chromium.org

[sof, 2015-06-18 06:41:00]

+tkent who introduced prefinalizers - they're explicitly exempted from
allocation (see ThreadState.h comment), so getting clarity on the tradeoffs
between allowing or not, seems key here.

[haraken, 2015-06-18 06:51:49]

On 2015/06/18 06:41:00, sof wrote:
> +tkent who introduced prefinalizers - they're explicitly exempted from
> allocation (see ThreadState.h comment), so getting clarity on the tradeoffs
> between allowing or not, seems key here.

I don't remember the reason why we had to forbid allocations during
pre-finalizers, but one possibility is because we didn't have m_firstUnsweptPage
before we landed lazy sweeping (but even in that world, I guess allocations
during pre-finalizers would have been ok because pages allocated during
pre-finalizers should have been inserted at the beginning of the list of
m_firstPage (which won't be swept in that sweeping phase))...

As far as I consider, allocations and mutations on an object graph are ok in
pre-finalizers. The only restriction is that it must not resurrect dead objects
that are about to be swept in the following sweeping phase.

[tkent, 2015-06-18 06:55:43]

I also don't remember the exact reason.  Probably it was same as thread-local
weak processing.

[haraken, 2015-06-18 06:59:03]

On 2015/06/18 06:55:43, tkent wrote:
> I also don't remember the exact reason.  Probably it was same as thread-local
> weak processing.

Maybe we thought so.

I think the restriction of pre-finalizers should be the same as the restriction
of destructors (i.e., must not resurrect dead objects). The restriction of
thread-local weak processing is stronger (i.e., must not mutate an object
graph).

[sof, 2015-06-18 12:18:03]

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:4351: EXPECT_EQ(42,
IntWrapper::s_destructorCalls);
Could you add a comment where the 42 IntWrappers come from; saves a bit of time
for anyone trying to make sense of the pass condition later.

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:980: GCState previousGCState = gcState();
This runs the risk of triggering an unused variable warning, non-Oilpan.

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:981: setGCState(Sweeping);
Moving this up before running the prefinalizers makes me a bit worried, but I'm
fine with giving it a go -- I cannot see it allowing problematic operations.

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:77: // garbage-collected objects allocated in
the thread.  However we must not
Could you update the documentation for prefinalizers?

[haraken, 2015-06-18 16:30:13]

Thanks for review!

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:4351: EXPECT_EQ(42,
IntWrapper::s_destructorCalls);
On 2015/06/18 12:18:03, sof wrote:
> Could you add a comment where the 42 IntWrappers come from; saves a bit of
time
> for anyone trying to make sense of the pass condition later.

Done.

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:980: GCState previousGCState = gcState();
On 2015/06/18 12:18:03, sof wrote:
> This runs the risk of triggering an unused variable warning, non-Oilpan.

Done.

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:981: setGCState(Sweeping);
On 2015/06/18 12:18:03, sof wrote:
> Moving this up before running the prefinalizers makes me a bit worried, but
I'm
> fine with giving it a go -- I cannot see it allowing problematic operations.

Moving setGCState(Sweeping) up here is necessary to avoid a GC from getting
scheduled during the pre-finalizers. scheduleGCIfNeeded() returns without
nothing if isSweepingProgress returns true.

I added a comment about this.

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1190863003/diff/60001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:77: // garbage-collected objects allocated in
the thread.  However we must not
On 2015/06/18 12:18:03, sof wrote:
> Could you update the documentation for prefinalizers?

Done.

[sof, 2015-06-18 18:14:04]

lgtm

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:4352: // The 42 IntWrappers were the ones
allocated in the pre-finalizer
Yes, but a bunch of them are coming from LargeHeapObject, which is what's
non-obvious.

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:75: // Declare that a class has a
pre-finalizer function. The function is called in
Thanks; it doesn't have to be 100% complete, but eager finalization adds a twist
wrt allowance to touch heap objects from within a destructor.

[haraken, 2015-06-18 18:54:38]

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (right):

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:4352: // The 42 IntWrappers were the ones
allocated in the pre-finalizer
On 2015/06/18 18:14:03, sof wrote:
> Yes, but a bunch of them are coming from LargeHeapObject, which is what's
> non-obvious.

Thanks, updated the comment.

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1190863003/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:75: // Declare that a class has a
pre-finalizer function. The function is called in
On 2015/06/18 18:14:03, sof wrote:
> Thanks; it doesn't have to be 100% complete, but eager finalization adds a
twist
> wrt allowance to touch heap objects from within a destructor.

Updated the comment.

[haraken, 2015-06-18 18:54:42]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-06-18 18:54:43]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1190863003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-06-18 18:55:33]

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

[commit-bot: I haz the power, 2015-06-18 21:53:54]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197399