Oilpan: complete current sweep before preparing for new GC.

Oilpan: complete current sweep before preparing for new GC.

If collectGarbage() is called while sweeping is in progress, complete
the sweeping before recording what GCState to revert back to should
we fail to get the other threads to stop at a safe point.

Otherwise we risk reverting back to a sweeping state that has been
completed & advanced past.

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

[sof, 2015-05-21 20:39:28]

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

[sof, 2015-05-21 20:39:29]

please take a look.

[haraken, 2015-05-21 23:01:34]

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

[haraken, 2015-05-21 23:01:35]

LGTM

https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:740: VERIFY_STATE_TRANSITION(m_gcState ==
NoGCScheduled || m_gcState == IdleGCScheduled || m_gcState == PreciseGCScheduled
|| m_gcState == FullGCScheduled || m_gcState == Sweeping || m_gcState ==
SweepingAndIdleGCScheduled || m_gcState == SweepingAndPreciseGCScheduled);

I think you can now remove a couple of states from here, since it's guaranteed
that sweeping is no progress at this point.

[sof, 2015-05-22 05:07:05]

https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:740: VERIFY_STATE_TRANSITION(m_gcState ==
NoGCScheduled || m_gcState == IdleGCScheduled || m_gcState == PreciseGCScheduled
|| m_gcState == FullGCScheduled || m_gcState == Sweeping || m_gcState ==
SweepingAndIdleGCScheduled || m_gcState == SweepingAndPreciseGCScheduled);
On 2015/05/21 23:01:35, haraken wrote:
> 
> I think you can now remove a couple of states from here, since it's guaranteed
> that sweeping is no progress at this point.

Maybe, not so sure -- sweeping might be forbidden when collectGarbage() is
called.

[sof, 2015-05-22 05:07:28]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-05-22 05:07:41]

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

[commit-bot: I haz the power, 2015-05-22 05:11:38]

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

[haraken, 2015-05-22 05:13:05]

https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:740: VERIFY_STATE_TRANSITION(m_gcState ==
NoGCScheduled || m_gcState == IdleGCScheduled || m_gcState == PreciseGCScheduled
|| m_gcState == FullGCScheduled || m_gcState == Sweeping || m_gcState ==
SweepingAndIdleGCScheduled || m_gcState == SweepingAndPreciseGCScheduled);
On 2015/05/22 05:07:05, sof wrote:
> On 2015/05/21 23:01:35, haraken wrote:
> > 
> > I think you can now remove a couple of states from here, since it's
guaranteed
> > that sweeping is no progress at this point.
> 
> Maybe, not so sure -- sweeping might be forbidden when collectGarbage() is
> called.

Since you're calling completeSweep() before setting the StoppingOtherThreads
state, I think we can remove Sweeping, SweepingAndIdleGCScheduled and
SweepingAndPreciseGCScheduled.

Instead you can add ASSERT(!isSweepInProgress()).

[sof, 2015-05-22 05:15:19]

On 2015/05/22 05:13:05, haraken wrote:
>
https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
> Source/platform/heap/ThreadState.cpp:740: VERIFY_STATE_TRANSITION(m_gcState ==
> NoGCScheduled || m_gcState == IdleGCScheduled || m_gcState ==
PreciseGCScheduled
> || m_gcState == FullGCScheduled || m_gcState == Sweeping || m_gcState ==
> SweepingAndIdleGCScheduled || m_gcState == SweepingAndPreciseGCScheduled);
> On 2015/05/22 05:07:05, sof wrote:
> > On 2015/05/21 23:01:35, haraken wrote:
> > > 
> > > I think you can now remove a couple of states from here, since it's
> guaranteed
> > > that sweeping is no progress at this point.
> > 
> > Maybe, not so sure -- sweeping might be forbidden when collectGarbage() is
> > called.
> 
> Since you're calling completeSweep() before setting the StoppingOtherThreads
> state, I think we can remove Sweeping, SweepingAndIdleGCScheduled and
> SweepingAndPreciseGCScheduled.
> 
> Instead you can add ASSERT(!isSweepInProgress()).

Doesn't completeSweep() bail out on sweepForbidden() ?

[haraken, 2015-05-22 05:18:49]

On 2015/05/22 05:15:19, sof wrote:
> On 2015/05/22 05:13:05, haraken wrote:
> >
>
https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
> > File Source/platform/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1153533002/diff/1/Source/platform/heap/Thread...
> > Source/platform/heap/ThreadState.cpp:740: VERIFY_STATE_TRANSITION(m_gcState
==
> > NoGCScheduled || m_gcState == IdleGCScheduled || m_gcState ==
> PreciseGCScheduled
> > || m_gcState == FullGCScheduled || m_gcState == Sweeping || m_gcState ==
> > SweepingAndIdleGCScheduled || m_gcState == SweepingAndPreciseGCScheduled);
> > On 2015/05/22 05:07:05, sof wrote:
> > > On 2015/05/21 23:01:35, haraken wrote:
> > > > 
> > > > I think you can now remove a couple of states from here, since it's
> > guaranteed
> > > > that sweeping is no progress at this point.
> > > 
> > > Maybe, not so sure -- sweeping might be forbidden when collectGarbage() is
> > > called.
> > 
> > Since you're calling completeSweep() before setting the StoppingOtherThreads
> > state, I think we can remove Sweeping, SweepingAndIdleGCScheduled and
> > SweepingAndPreciseGCScheduled.
> > 
> > Instead you can add ASSERT(!isSweepInProgress()).
> 
> Doesn't completeSweep() bail out on sweepForbidden() ?

That can happen only if completeSweep is called recursively, as commented on the
sweepForbidden check. After finishing the out-most completeSweep,
ASSERT(isSweepInProgress()) must hold.

If you'd argue that the out-most completeSweep can bail out, the completeSweep
added to this CL won't do the work you expect, and that would be a problem
anyway, I guess.