Bug 204341 – [Win] Implement WTF::ThreadSpecific in WTF::Thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Fujii Hironori]

[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

[Fujii Hironori]

Created attachment 383839 [details]
WIP patch

[Fujii Hironori]

Created attachment 383851 [details]
Patch

[Fujii Hironori]

review?

[Brent Fulgham]

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)?

[Yusuke Suzuki]

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 { }`.

[Fujii Hironori]

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?

[Fujii Hironori]

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

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 383946 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383946 [details]
Patch for landing

Clearing flags on attachment: 383946

Committed r252687: <https://trac.webkit.org/changeset/252687>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57359750>