Need a way to determine if the main thread has been initialized.

• Component: Web Template Framework
• Status: RESOLVED INVALID

[David Levin]

wtf may be used without initializing the main thread. Due to these cases, it would be useful to know if the main thread has been initialized before calling isMainThread.

[David Levin]

Created attachment 92327 [details]
Patch

[Darin Adler]

Comment on attachment 92327 [details]
Patch

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

> Source/JavaScriptCore/wtf/MainThread.cpp:253
> +bool internalIsMainThreadInitialized()
> +{
> +#ifndef NDEBUG
> +    return mainThreadInitialized;
> +#else
> +    return false;
> +#endif
> +}

It doesn’t make sense to have this return false in NDEBUG builds. Maybe put a CRASH() here instead or in addition?

[Alexey Proskuryakov]

Comment on attachment 92327 [details]
Patch

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

> Source/JavaScriptCore/wtf/MainThread.cpp:246
> +bool internalIsMainThreadInitialized()

It's slightly inelegant to have a function that returns a boolean, but only "true" results can be trusted. Trusting a "false" results would be a race condition.

Can this be changed into a check-style function that asserts internally?

[Alexey Proskuryakov]

Comment on attachment 92327 [details]
Patch

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

> Source/JavaScriptCore/wtf/MainThread.cpp:248
> +#ifndef NDEBUG

Another extremely minor nit. ASSERTION_ENABLED is not exactly the same as !NDEBUG. People sometimes want to build release mode with assertions - I know that I did that once, with a significant but practical amount of code changes.

[David Levin]

(In reply to comment #3)
> (From update of attachment 92327 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92327&action=review
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:246
> > +bool internalIsMainThreadInitialized()
> 
> It's slightly inelegant to have a function that returns a boolean, but only "true" results can be trusted. 

Per discussion in irc, will change the name to mayCallIsMainThread.

(In reply to comment #4)
> (From update of attachment 92327 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92327&action=review
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:248
> > +#ifndef NDEBUG
> 
> Another extremely minor nit. ASSERTION_ENABLED is not exactly the same as !NDEBUG. People sometimes want to build release mode with assertions - I know that I did that once, with a significant but practical amount of code changes.

Thanks. Will fix.

[David Levin]

Comment on attachment 92327 [details]
Patch

Changing to "isKnownAsMainThread" which will change the code some.

[David Levin]

Created attachment 92352 [details]
Patch

[David Levin]

Created attachment 92354 [details]
Patch

[David Levin]

Comment on attachment 92354 [details]
Patch

I should add a similar atomicIncrement to initializeMainThreadToProcessMainThreadOnce().

[David Levin]

Created attachment 92433 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 92433 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        (WTF::initializeMainThreadOnce): Ditto (for OSX).
> +        (WTF::initializeMainThreadToProcessMainThreadOnce): Ditto (for OSX).

"OS X".

> Source/JavaScriptCore/wtf/MainThread.cpp:76
> +static int mainThreadInitialized;

It's quite confusing to have both mainThreadInitialized and initializedMainThread in this code.

> Source/JavaScriptCore/wtf/MainThread.cpp:113
> +    // Flush all writes and set mainThreadInitialized to 1 to indicate that the initialization has been done.
> +    atomicIncrement(&mainThreadInitialized);

Not sure if the fact that setting a variable named mainThreadInitialized to 1 indicates that the initialization has been done needs a comment.

That said, I can see why the use of atomicIncrement to insert a memory barrier needs some explanation. But after an IRC discussion, I also don't think that it's doing the job. There is no way to guarantee memory consistency without a memory barrier on a thread that checks mainThreadInitialized - it's not enough to use on the thread that changes it.

Also, I'm not sure if we want or do guarantee a full memory barrier in atomicIncrement implementations. Clearly, it's not required to increment one integer.

> Source/JavaScriptCore/wtf/MainThread.cpp:257
> +    CRASH();

This probably need a comment.

[David Levin]

Instead of doing this, I'm looking into making current thread much faster.

currentThread takes about 3x the time of isMainThread in debug and more than 2x in release, which is the reason for preferring to use isMainThread when possible.

On the other hand, if currentThread were faster than it is now, then this issue may go away.