[Win] Implement WTF::ThreadSpecific in WTF::Thread * Can remove tricky TLS destruction code * Can remove threadMap and threadMapMutex in ThreadingWin.cpp which is a part of cause of Bug 204192. See also bug #204192 comment #8
Created attachment 383839 [details] WIP patch
Created attachment 383851 [details] Patch
review?
Comment on attachment 383851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383851&action=review > Source/WTF/wtf/ThreadSpecific.h:103 > + void static destroy(void* ptr); I think we can remove THREAD_SPECIFIC_CALL entirely now. > Source/WTF/wtf/Threading.h:114 > + class SpecificStorage { Should this class be protected by OS(WINDOWS)?
Comment on attachment 383851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383851&action=review r=me too. Later, if we replace FLS use for Thread::current() to `thread_local` only on Windows, we could even get performance improvement since FLS is known as very slow. > Source/WTF/wtf/Threading.h:127 > + static constexpr size_t s_maxKeys = 32; > + static Atomic<int> s_numberOfKeys; > + static std::array<Atomic<DestroyFunction>, s_maxKeys> s_destroyFunctions; > + std::array<void*, s_maxKeys> m_slots; We could do it a bit better, like, for first 32 entries, use pre-allocated array like this. And latter, we have Vector<> and resize when some key is is accessed. We do not need to have a lock here since it is TLS so it is only called from the current thread, and IIRC, this is how pthread TLS is implemented typically (but not 100% confident). Can you add FIXME for this extension? Then, we can support more keys. > Source/WTF/wtf/Threading.h:-354 > -#if OS(WINDOWS) > - if (auto* thread = currentDying()) > - return *thread; > -#endif NICE > Source/WTF/wtf/win/ThreadingWin.cpp:307 > + m_slots.fill(nullptr); You don't need this if you define default initializer in class field like, `m_slots { }`.
Comment on attachment 383851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383851&action=review >> Source/WTF/wtf/ThreadSpecific.h:103 >> + void static destroy(void* ptr); > > I think we can remove THREAD_SPECIFIC_CALL entirely now. Not possible because WTF::Thread is using FlsAlloc at the moment. >> Source/WTF/wtf/Threading.h:114 >> + class SpecificStorage { > > Should this class be protected by OS(WINDOWS)? Yes. This code is placed in inside #if OS(WINDOWS). >> Source/WTF/wtf/Threading.h:127 >> + std::array<void*, s_maxKeys> m_slots; > > We could do it a bit better, like, for first 32 entries, use pre-allocated array like this. > And latter, we have Vector<> and resize when some key is is accessed. We do not need to have a lock here since it is TLS so it is only called from the current thread, and IIRC, this is how pthread TLS is implemented typically (but not 100% confident). > Can you add FIXME for this extension? Then, we can support more keys. SpecificStorage::allocKey is called from various threads, I think a Vector for destroy functions needs a lock. WTF::ThreadSpecific is only used in WebKit. Do we really need to support the flexible number of keys?
Comment on attachment 383851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383851&action=review > Source/WTF/wtf/Threading.h:118 > + WTF_EXPORT_PRIVATE static bool allocKey(int& key, DestroyFunction); Will rename allocKey → allocateKey https://webkit.org/code-style-guidelines/#names-full-words
Comment on attachment 383851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383851&action=review >> Source/WTF/wtf/Threading.h:118 >> + WTF_EXPORT_PRIVATE static bool allocKey(int& key, DestroyFunction); > > Will rename allocKey → allocateKey > https://webkit.org/code-style-guidelines/#names-full-words Will rename allocKey → allocateKey https://webkit.org/code-style-guidelines/#names-full-words >>> Source/WTF/wtf/Threading.h:127 >>> + std::array<void*, s_maxKeys> m_slots; >> >> We could do it a bit better, like, for first 32 entries, use pre-allocated array like this. >> And latter, we have Vector<> and resize when some key is is accessed. We do not need to have a lock here since it is TLS so it is only called from the current thread, and IIRC, this is how pthread TLS is implemented typically (but not 100% confident). >> Can you add FIXME for this extension? Then, we can support more keys. > > SpecificStorage::allocKey is called from various threads, I think a Vector for destroy functions needs a lock. > WTF::ThreadSpecific is only used in WebKit. Do we really need to support the flexible number of keys? pthreads also has a fixed limit PTHREAD_KEYS_MAX.
Created attachment 383946 [details] Patch for landing
Comment on attachment 383946 [details] Patch for landing Clearing flags on attachment: 383946 Committed r252687: <https://trac.webkit.org/changeset/252687>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57359750>