Bug 125305 - Incorrect usage of ThreadingOnce class.
Summary: Incorrect usage of ThreadingOnce class.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-05 07:29 PST by peavo
Modified: 2014-01-06 11:52 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.05 KB, patch)
2013-12-05 07:37 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (4.56 KB, patch)
2013-12-05 12:15 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2013-12-06 12:22 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2013-12-06 14:04 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2013-12-06 14:31 PST, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2013-12-05 07:29:25 PST
I have used the ThreadingOnce class incorrectly.

One cannot rely on that statics are threadsafe, so it can't be used as a static within a function.
On Mac, the class cannot be used as a global static, as global constructors/destructors fails to compile with the current compiler settings.

Based on this, I think the best way is to use the pthread implementation on Mac/Unix/..., and use the ThreadingOnce class for platforms which doesn't use pthreads.
Comment 1 peavo 2013-12-05 07:37:54 PST
Created attachment 218513 [details]
Patch
Comment 2 Geoffrey Garen 2013-12-05 10:24:01 PST
Comment on attachment 218513 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        On Mac, the class cannot be used as a global static, as global constructors/destructors fails to compile with the current compiler settings.

Those compiler settings exist for a reason. We want other platforms to get closer to being able to enable those settings, not farther away.
Comment 3 peavo 2013-12-05 12:15:41 PST
Created attachment 218528 [details]
Patch
Comment 4 peavo 2013-12-05 12:19:04 PST
(In reply to comment #2)
> (From update of attachment 218513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218513&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        On Mac, the class cannot be used as a global static, as global constructors/destructors fails to compile with the current compiler settings.
> 
> Those compiler settings exist for a reason. We want other platforms to get closer to being able to enable those settings, not farther away.

Trying again :)

I removed the static global ThreadingOnce object, and used a local static flag instead.
It's not fully equivalent to the pthread version with regards to synchronization between threads, but there are no global constructors/destructors now.
Comment 5 Brent Fulgham 2013-12-05 12:34:59 PST
Comment on attachment 218528 [details]
Patch

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

I know you are basically reverting to the original code here, but I think the original code still allows multiple threads to initialize. (Geof, please correct me where I'm wrong).

We are very close to switching to VS2013, which will allow us to use "std::call_once", which might be a better cross-platform solution.

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:287
> +    if (!initializedGlobalWorklist) {

It seems like this should be done using the old "check->lock mutex->check again" construct.  It seems like this logic still gives us a chance for two threads to initialize the global wordlist.

Shouldn't it be something like (pseudocode below):

static mutex initializationMutex;

Worklist* initializeGlobalWOrklist()
{
#if USE(PTHREADS)
    pthread_once(&initializeGlobalWorklistKeyOnce, initializeGlobalWorklistOnce);
 #else
    static bool initializedGlobalWorklist = false;
    if (!initializedGlobalWorklist) {
        ScopedLock locker(intializationMutex);
        if (!initializedGlobalWorklist) {
             initializeGlobalWOrklistOnce();
             initializeGlobalWorklist = true;
        }
    }

> Source/WTF/wtf/CompilationThread.cpp:52
> +    if (!initializedCompilationThreads) {

Ditto above. I think we can double-initialize with this logic.
Comment 6 peavo 2013-12-05 12:42:29 PST
(In reply to comment #5)
> (From update of attachment 218528 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218528&action=review
> 
> I know you are basically reverting to the original code here, but I think the original code still allows multiple threads to initialize. (Geof, please correct me where I'm wrong).
> 
> We are very close to switching to VS2013, which will allow us to use "std::call_once", which might be a better cross-platform solution.
> 
> > Source/JavaScriptCore/dfg/DFGWorklist.cpp:287
> > +    if (!initializedGlobalWorklist) {
> 
> It seems like this should be done using the old "check->lock mutex->check again" construct.  It seems like this logic still gives us a chance for two threads to initialize the global wordlist.
> 
> Shouldn't it be something like (pseudocode below):
> 
> static mutex initializationMutex;
> 
> Worklist* initializeGlobalWOrklist()
> {
> #if USE(PTHREADS)
>     pthread_once(&initializeGlobalWorklistKeyOnce, initializeGlobalWorklistOnce);
>  #else
>     static bool initializedGlobalWorklist = false;
>     if (!initializedGlobalWorklist) {
>         ScopedLock locker(intializationMutex);
>         if (!initializedGlobalWorklist) {
>              initializeGlobalWOrklistOnce();
>              initializeGlobalWorklist = true;
>         }
>     }
> 
> > Source/WTF/wtf/CompilationThread.cpp:52
> > +    if (!initializedCompilationThreads) {
> 
> Ditto above. I think we can double-initialize with this logic.

I agree that there is a possibility for double-initialization with just using a static flag.
But isn't the pseudocode above equivalent to using a static global ThreadingOnce object?
Comment 7 Geoffrey Garen 2013-12-05 13:58:59 PST
Comment on attachment 218528 [details]
Patch

Also not thread-safe.
Comment 8 peavo 2013-12-06 12:22:40 PST
Created attachment 218612 [details]
Patch
Comment 9 peavo 2013-12-06 12:25:22 PST
(In reply to comment #5)
> 
> It seems like this should be done using the old "check->lock mutex->check again" construct.  It seems like this logic still gives us a chance for two threads to initialize the global wordlist.
> 

Modified the patch by adding a mutex as suggested, should be thread-safe now :)
Comment 10 Geoffrey Garen 2013-12-06 12:37:09 PST
Comment on attachment 218612 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:266
> +static Mutex initializeMutex;

This is a static initializer / destructor.
Comment 11 peavo 2013-12-06 12:40:20 PST
(In reply to comment #10)
> (From update of attachment 218612 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218612&action=review
> 
> > Source/JavaScriptCore/dfg/DFGWorklist.cpp:266
> > +static Mutex initializeMutex;
> 
> This is a static initializer / destructor.

Yes, I have tried to come up with something which can avoid that, but I find it hard to get around this without using a static global object for platform which doesn't use pthreads ...
Comment 12 Brent Fulgham 2013-12-06 12:45:14 PST
Comment on attachment 218612 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:266
>>> +static Mutex initializeMutex;
>> 
>> This is a static initializer / destructor.
> 
> Yes, I have tried to come up with something which can avoid that, but I find it hard to get around this without using a static global object for platform which doesn't use pthreads ...

Geof: Given the lack of pthread_once_t support on Windows, can you suggest an appropriate construct that doesn't involve a static initializer?
Comment 13 Geoffrey Garen 2013-12-06 13:00:11 PST
You either have to write your own mutex class that uses only POD data and has a trivial constructor / destructor, or you need to use std::call_once.
Comment 14 peavo 2013-12-06 13:09:40 PST
(In reply to comment #13)
> You either have to write your own mutex class that uses only POD data and has a trivial constructor / destructor, or you need to use std::call_once.

Thanks, would it be ok to use the PlatformMutex type, and add a couple of new functions to lock and unlock a mutex of this type?
Comment 15 peavo 2013-12-06 14:04:54 PST
Created attachment 218623 [details]
Patch
Comment 16 peavo 2013-12-06 14:05:47 PST
(In reply to comment #13)
> You either have to write your own mutex class that uses only POD data and has a trivial constructor / destructor, or you need to use std::call_once.

Updated patch to use PlatformMutex type, and added functions to create, lock, and unlock this mutex type.
Comment 17 peavo 2013-12-06 14:10:32 PST
(In reply to comment #16)
> (In reply to comment #13)
> > You either have to write your own mutex class that uses only POD data and has a trivial constructor / destructor, or you need to use std::call_once.
> 
> Updated patch to use PlatformMutex type, and added functions to create, lock, and unlock this mutex type.

I considered creating another mutex class, but to avoid having yet another mutex class, I thought it would be better to reuse an existing, + adding a couple of functions to manipulate the mutex.
Comment 18 peavo 2013-12-06 14:31:27 PST
Created attachment 218625 [details]
Patch
Comment 19 peavo 2013-12-06 14:41:50 PST
(In reply to comment #18)
> Created an attachment (id=218625) [details]
> Patch

Corrected compile error.
Comment 20 peavo 2014-01-06 11:51:46 PST
This is now fixed by using std::call_once.