Bug 59984

Created attachment 92327 [details] Patch

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?

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?

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.

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

Comment on attachment 92327 [details] Patch Changing to "isKnownAsMainThread" which will change the code some.

Created attachment 92352 [details] Patch

Created attachment 92354 [details] Patch

Comment on attachment 92354 [details] Patch I should add a similar atomicIncrement to initializeMainThreadToProcessMainThreadOnce().

Created attachment 92433 [details] Patch

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.

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.