Bug 132088 - The GC should only resume compiler threads that it suspended in the same GC pass
Summary: The GC should only resume compiler threads that it suspended in the same GC pass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-23 16:26 PDT by Mark Lam
Modified: 2014-04-23 17:44 PDT (History)
7 users (show)

See Also:


Attachments
the patch (3.42 KB, patch)
2014-04-23 16:34 PDT, Mark Lam
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.