Fix nested parking of threads while setting up a garbage collection.

Fix nested parking of threads while setting up a garbage collection.

When a thread requests a GC, it first enters a safe point to indicate
that it is prepared to garbage collect. After that it tries to interrupt/
notify the other threads to also enter safe points and waits around for
them to enter that state. While waiting, it will block other threads from
attempting to similarly park threads.

But, should another thread trigger a GC while the parking thread is
waiting, it will first enter a safe point, but then proceed to try to
park the other threads. Which it will be blocked from making progress on
due to the above lock being taken by the initial thread requesting thread
parking. By initially entering a safe point though, the first thread may
indirectly reach the desired condition of all threads being parked. Which
means it can safely proceed with the GC _along_ with releasing the lock
for other threads to attempt to park thread. The second parking thread
will then do so, but be blocked waiting for all the threads to park. The
thread that has proceeded to initiate a GC will not do so, so after the
max parking time has passed, the second thread will signal that it failed
to park the other threads, and try to abort its attempted GC.

As part of that abort/cancellation step, it will attempt to reset its
GCState back to what it was when the GC attempt was initiated. That
GCState may have changed behind the thread's back -- the initial thread
has proceeded with a GC, and will as part of that change the GCState of
each attached thread. Hence, GC cancellation needs to check that the
GCState it is trying to reset from is the expected one. If it isn't,
that is a sign that another thread has initiated a GC and it should
just quietly return. Which means waiting for it to be allowed to leave
its safe point..and at that point perform the thread-specific parts of
the GC.

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

[haraken, 2015-05-28 01:31:47]

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

[haraken, 2015-05-28 01:31:48]

Thanks for the great detective work!!

This looks good, but another possible fix would be to remove
StoppingOtherThreads from GCState. Alternatively we can prepare
ThreadState::m_stoppingOtherThreads. StoppingOtherThreads is a state just to
prevent the thread from re-entering Heap::collectGarbage, so one flag would be
enough. It will remove the need to "restore" the GCState, which I hope will make
the transition model much easier.

[sof, 2015-05-28 05:20:52]

On 2015/05/28 01:31:48, haraken wrote:
> Thanks for the great detective work!!
> 
> This looks good, but another possible fix would be to remove
> StoppingOtherThreads from GCState. Alternatively we can prepare
> ThreadState::m_stoppingOtherThreads. StoppingOtherThreads is a state just to
> prevent the thread from re-entering Heap::collectGarbage, so one flag would be
> enough. It will remove the need to "restore" the GCState, which I hope will
make
> the transition model much easier.

Yes, this pseudo state has now proven itself not worth keeping around :) , so we
should attempt removal. I don't really want to take that on right here though,
but I can follow up.

(We may have to merge fixes like this back to M44, not sure. Keeping CLs
small&tidy has some merit wrt that.)

[haraken, 2015-05-28 05:44:19]

On 2015/05/28 05:20:52, sof wrote:
> On 2015/05/28 01:31:48, haraken wrote:
> > Thanks for the great detective work!!
> > 
> > This looks good, but another possible fix would be to remove
> > StoppingOtherThreads from GCState. Alternatively we can prepare
> > ThreadState::m_stoppingOtherThreads. StoppingOtherThreads is a state just to
> > prevent the thread from re-entering Heap::collectGarbage, so one flag would
be
> > enough. It will remove the need to "restore" the GCState, which I hope will
> make
> > the transition model much easier.
> 
> Yes, this pseudo state has now proven itself not worth keeping around :) , so
we
> should attempt removal. I don't really want to take that on right here though,
> but I can follow up.
> 
> (We may have to merge fixes like this back to M44, not sure. Keeping CLs
> small&tidy has some merit wrt that.)

Makes sense. Let's make that change in a follow-up. LGTM.

[sof, 2015-05-28 06:31:50]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-05-28 06:32:00]

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

[commit-bot: I haz the power, 2015-05-28 06:35:16]

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