Incorrect usage of ThreadingOnce class.

• Component: JavaScriptCore
• Status: RESOLVED INVALID

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.

Created attachment 218513 [details]
Patch

[Geoffrey Garen]

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.

Created attachment 218528 [details]
Patch

(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.

[Brent Fulgham]

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.

(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?

[Geoffrey Garen]

Comment on attachment 218528 [details]
Patch

Also not thread-safe.

Created attachment 218612 [details]
Patch

(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 :)

[Geoffrey Garen]

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.

(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 ...

[Brent Fulgham]

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?

[Geoffrey Garen]

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.

(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?

Created attachment 218623 [details]
Patch

(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.

(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.

Created attachment 218625 [details]
Patch

(In reply to comment #18)
> Created an attachment (id=218625) [details]
> Patch

Corrected compile error.

This is now fixed by using std::call_once.