Oilpan: simplify away StoppingOtherThreads pseudo GCState.

Oilpan: simplify away StoppingOtherThreads pseudo GCState.

ThreadState now keeps track of whether or not GCs are forbidden; use that
to prevent nested GCs from kicking in rather than the temporary
StoppingOtherThreads GCState previously used.

R=haraken
BUG=482838
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196193

[sof, 2015-05-28 14:45:04]

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

[sof, 2015-05-28 14:45:05]

please take a look.

[haraken, 2015-05-28 15:00:55]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-05-28 15:00:56]

LGTM

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:168: SafePointScope m_safePointScope;

Add a comment and mention that the order between GCForbiddenScope and
SafePointScope matters.

I don't have a strong opinion, but it might be clearer to remove
GCForbiddenScope and write code like:

GCScope() {
  m_state->enterGCForbiddenScope();
  m_state->enterSafePointScope();
  ...
}

~GCScope() {
  ...
  m_state->leaveSafePointScope();
  m_state->leaveGCForbiddenScope();
}

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1894: RELEASE_ASSERT(!state->isGCForbidden());

I'd move this check to GCForbiddenScope's constructor. At least, after the
following completeSweep().

[sof, 2015-05-28 15:08:51]

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:168: SafePointScope m_safePointScope;
On 2015/05/28 15:00:56, haraken wrote:
> 
> Add a comment and mention that the order between GCForbiddenScope and
> SafePointScope matters.
> 
> I don't have a strong opinion, but it might be clearer to remove
> GCForbiddenScope and write code like:
> 
> GCScope() {
>   m_state->enterGCForbiddenScope();
>   m_state->enterSafePointScope();
>   ...
> }
> 
> ~GCScope() {
>   ...
>   m_state->leaveSafePointScope();
>   m_state->leaveGCForbiddenScope();
> }

Will add a comment re: ordering.

[sof, 2015-05-28 15:27:57]

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1894: RELEASE_ASSERT(!state->isGCForbidden());
On 2015/05/28 15:00:55, haraken wrote:
> 
> I'd move this check to GCForbiddenScope's constructor. At least, after the
> following completeSweep().

I don't understand the motivation behind the suggested change, could you
explain?

I'll explain mine :) Having it at the top of collectGarbage() catches the
problem right at the source/outset -- you shouldn't be entering collectGarbage()
at all while GCs are forbidden.

[haraken, 2015-05-28 15:28:46]

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1894: RELEASE_ASSERT(!state->isGCForbidden());
On 2015/05/28 15:27:57, sof wrote:
> On 2015/05/28 15:00:55, haraken wrote:
> > 
> > I'd move this check to GCForbiddenScope's constructor. At least, after the
> > following completeSweep().
> 
> I don't understand the motivation behind the suggested change, could you
> explain?
> 
> I'll explain mine :) Having it at the top of collectGarbage() catches the
> problem right at the source/outset -- you shouldn't be entering
collectGarbage()
> at all while GCs are forbidden.

ah, that makes sense!

[sof, 2015-05-29 18:52:19]

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:168: SafePointScope m_safePointScope;
On 2015/05/28 15:08:50, sof wrote:
> On 2015/05/28 15:00:56, haraken wrote:
> > 
> > Add a comment and mention that the order between GCForbiddenScope and
> > SafePointScope matters.
> > 
> > I don't have a strong opinion, but it might be clearer to remove
> > GCForbiddenScope and write code like:
> > 
> > GCScope() {
> >   m_state->enterGCForbiddenScope();
> >   m_state->enterSafePointScope();
> >   ...
> > }
> > 
> > ~GCScope() {
> >   ...
> >   m_state->leaveSafePointScope();
> >   m_state->leaveGCForbiddenScope();
> > }
> 
> Will add a comment re: ordering.

Done.

https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1894: RELEASE_ASSERT(!state->isGCForbidden());
On 2015/05/28 15:28:46, haraken wrote:
> On 2015/05/28 15:27:57, sof wrote:
> > On 2015/05/28 15:00:55, haraken wrote:
> > > 
> > > I'd move this check to GCForbiddenScope's constructor. At least, after the
> > > following completeSweep().
> > 
> > I don't understand the motivation behind the suggested change, could you
> > explain?
> > 
> > I'll explain mine :) Having it at the top of collectGarbage() catches the
> > problem right at the source/outset -- you shouldn't be entering
> collectGarbage()
> > at all while GCs are forbidden.
> 
> ah, that makes sense!

Added a comment next to it.

[haraken, 2015-05-30 00:24:36]

On 2015/05/29 18:52:19, sof wrote:
>
https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
> Source/platform/heap/Heap.cpp:168: SafePointScope m_safePointScope;
> On 2015/05/28 15:08:50, sof wrote:
> > On 2015/05/28 15:00:56, haraken wrote:
> > > 
> > > Add a comment and mention that the order between GCForbiddenScope and
> > > SafePointScope matters.
> > > 
> > > I don't have a strong opinion, but it might be clearer to remove
> > > GCForbiddenScope and write code like:
> > > 
> > > GCScope() {
> > >   m_state->enterGCForbiddenScope();
> > >   m_state->enterSafePointScope();
> > >   ...
> > > }
> > > 
> > > ~GCScope() {
> > >   ...
> > >   m_state->leaveSafePointScope();
> > >   m_state->leaveGCForbiddenScope();
> > > }
> > 
> > Will add a comment re: ordering.
> 
> Done.
> 
>
https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
> Source/platform/heap/Heap.cpp:1894: RELEASE_ASSERT(!state->isGCForbidden());
> On 2015/05/28 15:28:46, haraken wrote:
> > On 2015/05/28 15:27:57, sof wrote:
> > > On 2015/05/28 15:00:55, haraken wrote:
> > > > 
> > > > I'd move this check to GCForbiddenScope's constructor. At least, after
the
> > > > following completeSweep().
> > > 
> > > I don't understand the motivation behind the suggested change, could you
> > > explain?
> > > 
> > > I'll explain mine :) Having it at the top of collectGarbage() catches the
> > > problem right at the source/outset -- you shouldn't be entering
> > collectGarbage()
> > > at all while GCs are forbidden.
> > 
> > ah, that makes sense!
> 
> Added a comment next to it.

Would you rebase this? My GCSope change would have caused a conflict with this
CL...

[sof, 2015-05-30 09:31:18]

On 2015/05/30 00:24:36, haraken wrote:
> On 2015/05/29 18:52:19, sof wrote:
> >
>
https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
> > File Source/platform/heap/Heap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
> > Source/platform/heap/Heap.cpp:168: SafePointScope m_safePointScope;
> > On 2015/05/28 15:08:50, sof wrote:
> > > On 2015/05/28 15:00:56, haraken wrote:
> > > > 
> > > > Add a comment and mention that the order between GCForbiddenScope and
> > > > SafePointScope matters.
> > > > 
> > > > I don't have a strong opinion, but it might be clearer to remove
> > > > GCForbiddenScope and write code like:
> > > > 
> > > > GCScope() {
> > > >   m_state->enterGCForbiddenScope();
> > > >   m_state->enterSafePointScope();
> > > >   ...
> > > > }
> > > > 
> > > > ~GCScope() {
> > > >   ...
> > > >   m_state->leaveSafePointScope();
> > > >   m_state->leaveGCForbiddenScope();
> > > > }
> > > 
> > > Will add a comment re: ordering.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1145423009/diff/20001/Source/platform/heap/He...
> > Source/platform/heap/Heap.cpp:1894: RELEASE_ASSERT(!state->isGCForbidden());
> > On 2015/05/28 15:28:46, haraken wrote:
> > > On 2015/05/28 15:27:57, sof wrote:
> > > > On 2015/05/28 15:00:55, haraken wrote:
> > > > > 
> > > > > I'd move this check to GCForbiddenScope's constructor. At least, after
> the
> > > > > following completeSweep().
> > > > 
> > > > I don't understand the motivation behind the suggested change, could you
> > > > explain?
> > > > 
> > > > I'll explain mine :) Having it at the top of collectGarbage() catches
the
> > > > problem right at the source/outset -- you shouldn't be entering
> > > collectGarbage()
> > > > at all while GCs are forbidden.
> > > 
> > > ah, that makes sense!
> > 
> > Added a comment next to it.
> 
> Would you rebase this? My GCSope change would have caused a conflict with this
> CL...

Rebased and adjusted. In a manner that will allow you to use GCScope as planned,
hopefully.

[haraken, 2015-05-31 00:13:21]

LGTM

https://codereview.chromium.org/1145423009/diff/60001/Source/platform/heap/Sa...
File Source/platform/heap/SafePoint.h (right):

https://codereview.chromium.org/1145423009/diff/60001/Source/platform/heap/Sa...
Source/platform/heap/SafePoint.h:16: SafePointScope(ThreadState::StackState
stackState, ThreadState* state = ThreadState::current())

This looks a bit messy though -- sometimes take an optional parameter, sometimes
take nullptr which is actually a flag to change the behavior.

I'd prefer calling enter/leaveSafePoint() in GCScope's constructor/destructor to
avoid the mess.

[sof, 2015-05-31 10:28:09]

https://codereview.chromium.org/1145423009/diff/60001/Source/platform/heap/Sa...
File Source/platform/heap/SafePoint.h (right):

https://codereview.chromium.org/1145423009/diff/60001/Source/platform/heap/Sa...
Source/platform/heap/SafePoint.h:16: SafePointScope(ThreadState::StackState
stackState, ThreadState* state = ThreadState::current())
On 2015/05/31 00:13:20, haraken wrote:
> 
> This looks a bit messy though -- sometimes take an optional parameter,
sometimes
> take nullptr which is actually a flag to change the behavior.
> 
> I'd prefer calling enter/leaveSafePoint() in GCScope's constructor/destructor
to
> avoid the mess.

If you explicitly supply no ThreadState, it just behaves accordingly. Depending
on upstream changes, we may be able to avoid supporting a nullable ThreadState.

The constructor still needs an 'explicit' - added back.

[sof, 2015-05-31 10:29:11]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-31 10:29:11]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1145423009/#ps80001
(title: "revert 'explicit' removal")

[commit-bot: I haz the power, 2015-05-31 10:29:19]

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

[commit-bot: I haz the power, 2015-05-31 10:32:28]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196193