Summary: | [Win] Implement WTF::ThreadSpecific in WTF::Thread | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||
Component: | Web Template Framework | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, annulen, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dbates, don.olmstead, ews-watchlist, gyuyoung.kim, pvollan, ross.kirsling, ryuan.choi, sergio, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Fujii Hironori
2019-11-18 21:41:28 PST
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. |