It is great!
Important thing is that Thread::current() should not require WTF::initializeThreading(). It is achieved if we carefully integreate WTFThreadData's way to Thread::current().
Created attachment 316662 [details] Patch
Created attachment 316664 [details] Patch
Created attachment 316667 [details] Patch
Attachment 316667 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316706 [details] Patch
Attachment 316706 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316709 [details] Patch
Attachment 316709 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316710 [details] Patch
Attachment 316710 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316711 [details] Patch
Attachment 316711 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316712 [details] Patch
Attachment 316712 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316717 [details] Patch
Attachment 316717 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r220060: <http://trac.webkit.org/changeset/220060>
<rdar://problem/33622976>
This broke Apple internal builds.
(In reply to Brady Eidson from comment #20) > This broke Apple internal builds. Oops! Is it related to build failure? Or is it related to test failures? (like crash?) And can I have any log about that?
(In reply to Yusuke Suzuki from comment #21) > And can I have any log about that? The build failure is due to: ThreadHolder.h:96:67: error: use of undeclared identifier 'WTF_THREAD_DATA_KEY'
(In reply to Mark Lam from comment #22) > (In reply to Yusuke Suzuki from comment #21) > > And can I have any log about that? > > The build failure is due to: > ThreadHolder.h:96:67: error: use of undeclared identifier > 'WTF_THREAD_DATA_KEY' OK, can we include <wtf/FastTLS.h> in ThreadHolder.h? I can create a patch for that, but I don't have the way to ensure that internal build works fine.
Reverted r220060 for reason: This broke our internal builds. Contact reviewer of patch for more information. Committed r220069: <http://trac.webkit.org/changeset/220069>
Created attachment 316785 [details] Patch
Attachment 316785 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #26) > Attachment 316785 [details] did not pass style-queue: > > > ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of > #ifndef for header guard. [build/header_guard] [5] > Total errors found: 1 in 37 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. It seems like you ignored this very valid style warning the first time around. Could you please fix it this time? It's weird that this file *was* correct with #pragma once and it seems you reverted that on purpose...?
(In reply to Brady Eidson from comment #27) > (In reply to Build Bot from comment #26) > > Attachment 316785 [details] did not pass style-queue: > > > > > > ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of > > #ifndef for header guard. [build/header_guard] [5] > > Total errors found: 1 in 37 files > > > > > > If any of these errors are false positives, please file a bug against > > check-webkit-style. > > It seems like you ignored this very valid style warning the first time > around. > > Could you please fix it this time? > > It's weird that this file *was* correct with #pragma once and it seems you > reverted that on purpose...? This is intentional. WTF headers are a bit special ones becasue they are copied to build directory. If someone includes WTF headers from different places (build dir / source dir), the compiler is confused with the two different paths and includes the same file twice. Actually, I first used "#pragma once" for this. But it caused the compile error due to the above double inclusions :( I think this should be fixed, but I don't think this should be done in this patch.
(In reply to Yusuke Suzuki from comment #28) > (In reply to Brady Eidson from comment #27) > > (In reply to Build Bot from comment #26) > > > Attachment 316785 [details] did not pass style-queue: > > > > > > > > > ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of > > > #ifndef for header guard. [build/header_guard] [5] > > > Total errors found: 1 in 37 files > > > > > > > > > If any of these errors are false positives, please file a bug against > > > check-webkit-style. > > > > It seems like you ignored this very valid style warning the first time > > around. > > > > Could you please fix it this time? > > > > It's weird that this file *was* correct with #pragma once and it seems you > > reverted that on purpose...? > > This is intentional. WTF headers are a bit special ones becasue they are > copied to build directory. > If someone includes WTF headers from different places (build dir / source > dir), the compiler is confused with the two different paths and includes the > same file twice. > Actually, I first used "#pragma once" for this. But it caused the compile > error due to the above double inclusions :( > I think this should be fixed, but I don't think this should be done in this > patch. I'm confused as to why so many of the other WTF headers successfully use pragma once if this is a global problem with WTF
(In reply to Brady Eidson from comment #29) > (In reply to Yusuke Suzuki from comment #28) > > (In reply to Brady Eidson from comment #27) > > > (In reply to Build Bot from comment #26) > > > > Attachment 316785 [details] did not pass style-queue: > > > > > > > > > > > > ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of > > > > #ifndef for header guard. [build/header_guard] [5] > > > > Total errors found: 1 in 37 files > > > > > > > > > > > > If any of these errors are false positives, please file a bug against > > > > check-webkit-style. > > > > > > It seems like you ignored this very valid style warning the first time > > > around. > > > > > > Could you please fix it this time? > > > > > > It's weird that this file *was* correct with #pragma once and it seems you > > > reverted that on purpose...? > > > > This is intentional. WTF headers are a bit special ones becasue they are > > copied to build directory. > > If someone includes WTF headers from different places (build dir / source > > dir), the compiler is confused with the two different paths and includes the > > same file twice. > > Actually, I first used "#pragma once" for this. But it caused the compile > > error due to the above double inclusions :( > > I think this should be fixed, but I don't think this should be done in this > > patch. > > I'm confused as to why so many of the other WTF headers successfully use > pragma once if this is a global problem with WTF It is not limited to this ThreadHolder.h file. It depends on how the file is used. For example, I remember that "wtf/text/StringView.h" is also such a thing. https://bugs.webkit.org/show_bug.cgi?id=164686 https://bugs.webkit.org/show_bug.cgi?id=166676#c5 https://trac.webkit.org/changeset/210314/webkit
(In reply to Yusuke Suzuki from comment #30) > (In reply to Brady Eidson from comment #29) > > (In reply to Yusuke Suzuki from comment #28) > > > (In reply to Brady Eidson from comment #27) > > > > (In reply to Build Bot from comment #26) > > > > > Attachment 316785 [details] did not pass style-queue: > > > > > > > > > > > > > > > ERROR: Source/WTF/wtf/ThreadHolder.h:32: Use #pragma once instead of > > > > > #ifndef for header guard. [build/header_guard] [5] > > > > > Total errors found: 1 in 37 files > > > > > > > > > > > > > > > If any of these errors are false positives, please file a bug against > > > > > check-webkit-style. > > > > > > > > It seems like you ignored this very valid style warning the first time > > > > around. > > > > > > > > Could you please fix it this time? > > > > > > > > It's weird that this file *was* correct with #pragma once and it seems you > > > > reverted that on purpose...? > > > > > > This is intentional. WTF headers are a bit special ones becasue they are > > > copied to build directory. > > > If someone includes WTF headers from different places (build dir / source > > > dir), the compiler is confused with the two different paths and includes the > > > same file twice. > > > Actually, I first used "#pragma once" for this. But it caused the compile > > > error due to the above double inclusions :( > > > I think this should be fixed, but I don't think this should be done in this > > > patch. > > > > I'm confused as to why so many of the other WTF headers successfully use > > pragma once if this is a global problem with WTF > > It is not limited to this ThreadHolder.h file. It depends on how the file is > used. > For example, I remember that "wtf/text/StringView.h" is also such a thing. > https://bugs.webkit.org/show_bug.cgi?id=164686 > https://bugs.webkit.org/show_bug.cgi?id=166676#c5 > https://trac.webkit.org/changeset/210314/webkit And this is why we still sometimes uses ifndef guards for WTF while we already rewrote the headers of JSC, WebCore etc.
Comment on attachment 316785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316785&action=review Nice work. r=me with fixes. > Source/WTF/wtf/ThreadHolder.h:33 > -#pragma once > +#ifndef ThreadHolder_h > +#define ThreadHolder_h Please add a comment to document why this is necessary here. Or revert if this is not necessary. > Source/WTF/wtf/ThreadHolder.h:59 > // Creates and puts an instance of ThreadHolder into thread-specific storage. > - static void initialize(Thread&); > + WTF_EXPORT_PRIVATE static ThreadHolder& initialize(Ref<Thread>&&); > + WTF_EXPORT_PRIVATE static ThreadHolder& initializeCurrent(); Let's remove the WTF_EXPORT_PRIVATE for initialize() since it should only be called from slow paths in ThreadHolder*.cpp and Threading*.cpp. Let's also make them private and add Thread as a friend class. I think there's less risk with Thread doing the wrong thing (since Thread is crafted to work with ThreadHolder) than these being called from client code that should not be calling it (since there's move semantics on the Thread passed to ThreadHolder, and initialize() allocates a new ThreadHolder unconditonally). > Source/WTF/wtf/ThreadHolder.h:88 > + static WTF_EXPORTDATA ThreadSpecificKey m_key; Please #if !HAVE(FAST_TLS) m_key. > Source/WTF/wtf/ThreadHolder.h:112 > + // WRT WebCore: > + // ThreadHolder is used on main thread before it could possibly be used > + // on secondary ones, so there is no need for synchronization here. > + // WRT JavaScriptCore: > + // Thread::current() is initially called from initializeThreading(), ensuring > + // this is initially called in a pthread_once locked context. > +#if !HAVE(FAST_TLS) > + if (UNLIKELY(ThreadHolder::m_key == InvalidThreadSpecificKey)) > + initializeOnce(); > +#endif This is not safe. initializeOnce used to be called under a std::call_once guard in initializeThreading. I recommend that you create a ThreadHolder::initializeOnce in ThreadHolder.cpp that does the std::call_once guard, and in turn calls ThreadHolder::platformInitializeOnce which is the old initializeOnce in the Pthread and Win ThreadHolder files. Oh wait, I just realize that the comment above applies to synchronization of the call to initializeOnce. Still, I think this is fragile (it always was). But since you're changing the code to always ensure that initializeOnce() is called and it is only a slow path, I think we can get rid of this comment if we guard initializeOnce() with a std::call_once. Also put #if !HAVE(FAST_TLS) around initializeOnce and platformInitializeOnce (maybe can skip for Windows since it is always !HAVE(FAST_TLS)). > Source/WTF/wtf/ThreadHolderPthreads.cpp:51 > + // Ideally we'd have this as a release assert everywhere, but that would hurt performance. > + // Having this release assert here means that we will catch "didn't call > + // WTF::initializeThreading() soon enough" bugs in release mode. > ASSERT(m_key != InvalidThreadSpecificKey); Let's make this a RELEASE_ASSERT as the comment indicates. It doesn't hurt perf that much since this should only be done once per thread. On second thought, since the only way to call initialize() is via ThreadHolder::current(), and ThreadHolder::current() always ensures that initializeOnce() is called first, then we're not dependent on WTF::initializeThreading() to be called first anymore. You can remove this comment, and just leave the assert as a debug ASSERT as a sanity check. > Source/WTF/wtf/ThreadHolderWin.cpp:82 > + // Ideally we'd have this as a release assert everywhere, but that would hurt performance. > + // Having this release assert here means that we will catch "didn't call > + // WTF::initializeThreading() soon enough" bugs in release mode. > + ASSERT(m_key != InvalidThreadSpecificKey); Ditto. You can remove this obsolete comment. > Source/WTF/wtf/Threading.cpp:99 > + m_savedLastStackTop = stack().origin(); Can we assert ASSERT(!Thread::current().stack().isEmpty()) before this? I think it would be nicer if we can just initialize m_stack before this instead (to consolidate the initialization of the Thread instance initialization, but I think we can do that in a separate patch if desired. You can make the initialization of m_stack on "if (m_stack.isEmpty())". > Source/WTF/wtf/Threading.cpp:124 > context->stage = NewThreadContext::Stage::Initialized; > context->condition.signal(); If we consolidate initialization of m_stack in initializeInThread(), we can move this signaling part after the call to initializeInThread() below. > Source/WTF/wtf/Threading.cpp:284 > + Thread::current(); If we made ThreadHolder::initializeOnce() guarded with a std::call_once, do we still need this? Actually, your ChangeLog claims that "WTF::Thread::current() can be accessed even before calling WTF::initializeThreading". That means the 2 are now independent. Which, in turn, means: 1. you can remove this. 2. you should definitely make ThreadHolder::initializeOnce() safe by guarding it with a std::call_once. > Source/WTF/wtf/Threading.h:278 > +inline Thread& Thread::current() > +{ > + return ThreadHolder::current().thread(); > +} nit: since this is a single line, would you mind just inlining it in the Thread class definition as: static Thread& current() { return ThreadHolder::current().thread(); } That will make it easier to see at a glance that it's just a forwarding wrapper.
Comment on attachment 316785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316785&action=review Thank you! >> Source/WTF/wtf/ThreadHolder.h:33 >> +#define ThreadHolder_h > > Please add a comment to document why this is necessary here. Or revert if this is not necessary. Added the comment here. >> Source/WTF/wtf/ThreadHolder.h:59 >> + WTF_EXPORT_PRIVATE static ThreadHolder& initializeCurrent(); > > Let's remove the WTF_EXPORT_PRIVATE for initialize() since it should only be called from slow paths in ThreadHolder*.cpp and Threading*.cpp. > Let's also make them private and add Thread as a friend class. I think there's less risk with Thread doing the wrong thing (since Thread is crafted to work with ThreadHolder) than these being called from client code that should not be calling it (since there's move semantics on the Thread passed to ThreadHolder, and initialize() allocates a new ThreadHolder unconditonally). Make sense. >> Source/WTF/wtf/ThreadHolder.h:88 >> + static WTF_EXPORTDATA ThreadSpecificKey m_key; > > Please #if !HAVE(FAST_TLS) m_key. Fixed :) >> Source/WTF/wtf/ThreadHolder.h:112 >> +#endif > > This is not safe. initializeOnce used to be called under a std::call_once guard in initializeThreading. I recommend that you create a ThreadHolder::initializeOnce in ThreadHolder.cpp that does the std::call_once guard, and in turn calls ThreadHolder::platformInitializeOnce which is the old initializeOnce in the Pthread and Win ThreadHolder files. > > Oh wait, I just realize that the comment above applies to synchronization of the call to initializeOnce. Still, I think this is fragile (it always was). But since you're changing the code to always ensure that initializeOnce() is called and it is only a slow path, I think we can get rid of this comment if we guard initializeOnce() with a std::call_once. > > Also put #if !HAVE(FAST_TLS) around initializeOnce and platformInitializeOnce (maybe can skip for Windows since it is always !HAVE(FAST_TLS)). This comment and style is originally used in wtfThreadData(). Since previous Thread::current() is only used after calling initializeThreading(), the above comment is still valid. (Because the previous wtfThreadData() is valid with the above comment.). Anyway, using std::call_one in initializeOnce is nice. >> Source/WTF/wtf/ThreadHolderPthreads.cpp:51 >> ASSERT(m_key != InvalidThreadSpecificKey); > > Let's make this a RELEASE_ASSERT as the comment indicates. It doesn't hurt perf that much since this should only be done once per thread. > > On second thought, since the only way to call initialize() is via ThreadHolder::current(), and ThreadHolder::current() always ensures that initializeOnce() is called first, then we're not dependent on WTF::initializeThreading() to be called first anymore. You can remove this comment, and just leave the assert as a debug ASSERT as a sanity check. OK, changed it to ASSERT and drop the above invalid comment. >> Source/WTF/wtf/ThreadHolderWin.cpp:82 >> + ASSERT(m_key != InvalidThreadSpecificKey); > > Ditto. You can remove this obsolete comment. Done. >> Source/WTF/wtf/Threading.cpp:99 >> + m_savedLastStackTop = stack().origin(); > > Can we assert ASSERT(!Thread::current().stack().isEmpty()) before this? > > I think it would be nicer if we can just initialize m_stack before this instead (to consolidate the initialization of the Thread instance initialization, but I think we can do that in a separate patch if desired. You can make the initialization of m_stack on "if (m_stack.isEmpty())". This function is also called for existing non WTF thread. And m_stack is initialized in very different matter in different platforms. (For the platforms having StackBounds::newThreadStackBounds, we initialize it in the caller thread.). I added the above assert here. >> Source/WTF/wtf/Threading.cpp:124 >> context->condition.signal(); > > If we consolidate initialization of m_stack in initializeInThread(), we can move this signaling part after the call to initializeInThread() below. Because of the above issue, we cannot move this to initializeInThread(). >> Source/WTF/wtf/Threading.cpp:284 >> + Thread::current(); > > If we made ThreadHolder::initializeOnce() guarded with a std::call_once, do we still need this? > > Actually, your ChangeLog claims that "WTF::Thread::current() can be accessed even before calling WTF::initializeThreading". That means the 2 are now independent. Which, in turn, means: > 1. you can remove this. > 2. you should definitely make ThreadHolder::initializeOnce() safe by guarding it with a std::call_once. I think we should keep this. We only ensured that Thread::current() is safe only when the previous wtfThreadData() style is guaranteed. >> Source/WTF/wtf/Threading.h:278 >> +} > > nit: since this is a single line, would you mind just inlining it in the Thread class definition as: > static Thread& current() { return ThreadHolder::current().thread(); } > > That will make it easier to see at a glance that it's just a forwarding wrapper. Changed, thank you :)
Created attachment 316862 [details] Patch for landing
Attachment 316862 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:37: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 316871 [details] Patch
Attachment 316871 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ThreadHolder.h:37: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 316871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316871&action=review r=me with remaining issues resolved. > Source/WTF/wtf/ThreadHolder.h:83 > + WTF_EXPORT_PRIVATE static void ensureKey(); I think you can call this initializeKey() or keep the original name initializeOnce(). I suggested ensureKey() back when I was thinking that it will do the check for "if (UNLIKELY(ThreadHolder::m_key == InvalidThreadSpecificKey))". Since that's not necessary anymore, initializeOnce is a better name here. > Source/WTF/wtf/ThreadHolderPthreads.cpp:40 > +void ThreadHolder::ensureKey() Let's change this back to initializeOnce (for reasons above). > Source/WTF/wtf/ThreadHolderWin.cpp:52 > -void ThreadHolder::initializeOnce() > +void ThreadHolder::ensureKey() Let's change this back to initializeOnce (for reasons above). > Source/WTF/wtf/Threading.cpp:100 > + if (m_stack.isEmpty()) > + m_stack = StackBounds::currentThreadStackBounds(); I just re-read Thread::create() and saw that "thread->m_stack = StackBounds::newThreadStackBounds(thread->m_handle)" is only executed after the new thread has started. That means there's a race to set m_stack. However, since both should yield the same result, it should not matter that they overwrite each other with the same m_stack value. If you think we cannot guarantee that m_stack will end up with the same value, then we can eliminate the race by making initializeInThread() take a "needStackBoundInitialization" argument and only initialize it based on that. > Source/WTF/wtf/Threading.cpp:287 > + ThreadHolder::ensureKey(); Let's rename this back to initializeOnce (for reasons above).
Comment on attachment 316871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316871&action=review >> Source/WTF/wtf/ThreadHolder.h:83 >> + WTF_EXPORT_PRIVATE static void ensureKey(); > > I think you can call this initializeKey() or keep the original name initializeOnce(). I suggested ensureKey() back when I was thinking that it will do the check for "if (UNLIKELY(ThreadHolder::m_key == InvalidThreadSpecificKey))". Since that's not necessary anymore, initializeOnce is a better name here. OK, I'll rename it to initializeKey(). >> Source/WTF/wtf/ThreadHolderPthreads.cpp:40 >> +void ThreadHolder::ensureKey() > > Let's change this back to initializeOnce (for reasons above). Fixed. >> Source/WTF/wtf/ThreadHolderWin.cpp:52 >> +void ThreadHolder::ensureKey() > > Let's change this back to initializeOnce (for reasons above). Fixed. >> Source/WTF/wtf/Threading.cpp:100 >> + m_stack = StackBounds::currentThreadStackBounds(); > > I just re-read Thread::create() and saw that "thread->m_stack = StackBounds::newThreadStackBounds(thread->m_handle)" is only executed after the new thread has started. That means there's a race to set m_stack. However, since both should yield the same result, it should not matter that they overwrite each other with the same m_stack value. If you think we cannot guarantee that m_stack will end up with the same value, then we can eliminate the race by making initializeInThread() take a "needStackBoundInitialization" argument and only initialize it based on that. Actually, it does not have race. This is because the newly created thread need to wait the establishment in the creator side. And the creator assigns m_stack if it can :) >> Source/WTF/wtf/Threading.cpp:287 >> + ThreadHolder::ensureKey(); > > Let's rename this back to initializeOnce (for reasons above). Fixed.
Committed r220186: <http://trac.webkit.org/changeset/220186>
Committed r220187: <http://trac.webkit.org/changeset/220187>