Bug 204341

Summary: [Win] Implement WTF::ThreadSpecific in WTF::Thread
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: Web Template FrameworkAssignee: 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 Flags
WIP patch
none
Patch
none
Patch for landing none

Description Fujii Hironori 2019-11-18 21:41:28 PST
[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
Comment 1 Fujii Hironori 2019-11-18 21:41:50 PST
Created attachment 383839 [details]
WIP patch
Comment 2 Fujii Hironori 2019-11-19 00:41:34 PST
Created attachment 383851 [details]
Patch
Comment 3 Fujii Hironori 2019-11-19 18:11:07 PST
review?
Comment 4 Brent Fulgham 2019-11-19 19:23:33 PST
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 5 Yusuke Suzuki 2019-11-19 19:28:39 PST
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 6 Fujii Hironori 2019-11-19 19:48:38 PST
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 7 Fujii Hironori 2019-11-19 19:57:17 PST
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 8 Fujii Hironori 2019-11-19 20:10:16 PST
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.
Comment 9 Fujii Hironori 2019-11-19 22:59:46 PST
Created attachment 383946 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-11-20 08:04:59 PST
Comment on attachment 383946 [details]
Patch for landing

Clearing flags on attachment: 383946

Committed r252687: <https://trac.webkit.org/changeset/252687>
Comment 11 WebKit Commit Bot 2019-11-20 08:05:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-11-20 08:05:22 PST
<rdar://problem/57359750>