Bug 132088

Summary: The GC should only resume compiler threads that it suspended in the same GC pass
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch mhahnenberg: review+

Description Mark Lam 2014-04-23 16:26:05 PDT
Currently this scenario can occur:
1. Thread 1 starts a GC and tries to suspend DFG worklist threads.  However, no worklists were created yet at the that time.
2. Thread 2 starts to compile some functions and creates a DFG worklist, and acquires the worklist thread's lock.
3. Thread 1's GC completes and tries to resume suspended DFG worklist thread.  This time, it sees the worklist created by Thread 2 and ends up unlocking the worklist thread's lock that is supposedly held by Thread 2.

Thereafter, chaos ensues.

The fix is to cache the worklists that were actually suspended by each GC pass, and only resume those when the GC is done.
Comment 1 Radar WebKit Bug Importer 2014-04-23 16:26:33 PDT
<rdar://problem/16706259>
Comment 2 Mark Lam 2014-04-23 16:34:40 PDT
Created attachment 230022 [details]
the patch
Comment 3 Mark Hahnenberg 2014-04-23 16:49:32 PDT
Comment on attachment 230022 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230022&action=review

r=me

> Source/JavaScriptCore/heap/Heap.cpp:1025
> +    ASSERT(!m_suspendedCompilerWorklists.size());

I find it's more readable to say "m_suspendedCompilerWorklists.isEmpty()" instead.

> Source/JavaScriptCore/heap/Heap.cpp:1235
> +    for (auto worklist : m_suspendedCompilerWorklists)
> +        worklist->resumeAllThreads();
> +    m_suspendedCompilerWorklists.clear();

You should change the other places in the GC to only iterate this list. If the GC itself didn't suspend a work list then it shouldn't be touching that work list at all.
Comment 4 Mark Lam 2014-04-23 17:44:23 PDT
Thanks.  Suggested changes applied.  Landed in r167733: <http://trac.webkit.org/r167733>.