Bug 175013 – Merge ThreadHolder to WTF::Thread itself

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

[Yusuke Suzuki]

By doing so, we can put WTF::Thread* directly to TLS (instead of putting ThreadHolder*, which holds Ref<Thread>).
It reduces one level indirection. Yeah, ThreadHolder design is clean and great, but if this is important part, we should take more efficient design.
Actually, we use fast TLS in some platforms (which is pre-allocated TLS by the system, and we can access it with super fast operation, like using segment registers).
If we can drop one level indirection here, it would be awesome.

TLS -> ThreadHolder* -> Thread*
TLS -> Thread*

[Yusuke Suzuki]

Created attachment 317098 [details]
Patch

[Build Bot]

Attachment 317098 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ThreadingWin.cpp:353:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 317110 [details]
Patch

[Build Bot]

Attachment 317110 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/ThreadingWin.cpp:350:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 317110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317110&action=review

r=me with issues resolved (including broken windows build) and EWS bots are green.

> Source/WTF/wtf/Threading.h:69
>  class Thread : public ThreadSafeRefCounted<Thread> {

I didn't think of this before but we need to double check something here: ThreadHolder is not allocated with bmalloc, but ThreadSafeRefCounted is.  Therefore, Thread is also allocated with bmalloc.  We need to make sure that there's no unsafe interaction between bmalloc and TLS (basically, does bmalloc also depend on TLS ... if so, we need to make sure thread TLS clean up happens before bmalloc TLS clean up).  Otherwise, we need to use a non-bmalloc ThreadSafeRefCounted.

But since this is not an issue introduced in this patch, we can consider fixing it in a subsequent patch (if needed).

> Source/WTF/wtf/Threading.h:240
> +    // Holds Thread in the thread-specific storage. The destructor of current thread TLS reliably destroy Thread.

I would rephrase this as:
The Thread instance is ref'ed and held in thread-specific storage. It will be deref'ed by destructTLS at thread destruction time.

> Source/WTF/wtf/ThreadingPthreads.cpp:477
> +    auto& threadInTLS = thread.leakRef();

I suggest adding a comment before this to say something like:
We leak the ref to keep the Thread alive while it is held in TLS. destructTLS will deref it later at thread destruction time.

> Source/WTF/wtf/ThreadingWin.cpp:314
> +    auto& threadInTLS = thread.leakRef();

Please add the comment here as suggested in ThreadingPthreads about why we leakRef.

[Yusuke Suzuki]

Comment on attachment 317110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317110&action=review

>> Source/WTF/wtf/Threading.h:69
>>  class Thread : public ThreadSafeRefCounted<Thread> {
> 
> I didn't think of this before but we need to double check something here: ThreadHolder is not allocated with bmalloc, but ThreadSafeRefCounted is.  Therefore, Thread is also allocated with bmalloc.  We need to make sure that there's no unsafe interaction between bmalloc and TLS (basically, does bmalloc also depend on TLS ... if so, we need to make sure thread TLS clean up happens before bmalloc TLS clean up).  Otherwise, we need to use a non-bmalloc ThreadSafeRefCounted.
> 
> But since this is not an issue introduced in this patch, we can consider fixing it in a subsequent patch (if needed).

Sounds reasonable. I think bmalloc should do well even in the face of TLS. Otherwise, we cannnot use any values allocated in bmalloc in TLS.
And I think this issue remains even before introducing WTF::Thread. Before that, we used HashMap to manage thread handle and id.
If we remove id from the hash map, the hash map could call delete to the bucket. And since the hash map and its buckets are allocated in bmalloc, yeah, it causes issues.

I've checked it in the current macOS. And it turns out that macOS FastTLS is ok because bmalloc's FastTLS number is smaller than this TLS. (I've tested it in my macOS laptop).
But anyway, it's safer to use non bmalloc things in TLS destroyed related things. I'll file it separately.

>> Source/WTF/wtf/Threading.h:240
>> +    // Holds Thread in the thread-specific storage. The destructor of current thread TLS reliably destroy Thread.
> 
> I would rephrase this as:
> The Thread instance is ref'ed and held in thread-specific storage. It will be deref'ed by destructTLS at thread destruction time.

OK, fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:477
>> +    auto& threadInTLS = thread.leakRef();
> 
> I suggest adding a comment before this to say something like:
> We leak the ref to keep the Thread alive while it is held in TLS. destructTLS will deref it later at thread destruction time.

Sounds fine. Fixed.

>> Source/WTF/wtf/ThreadingWin.cpp:314
>> +    auto& threadInTLS = thread.leakRef();
> 
> Please add the comment here as suggested in ThreadingPthreads about why we leakRef.

OK, fixed.

[Yusuke Suzuki]

Committed r220214: <http://trac.webkit.org/changeset/220214>

[Radar WebKit Bug Importer]

<rdar://problem/33704658>

[Yusuke Suzuki]

Comment on attachment 317110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317110&action=review

>>> Source/WTF/wtf/Threading.h:69
>>>  class Thread : public ThreadSafeRefCounted<Thread> {
>> 
>> I didn't think of this before but we need to double check something here: ThreadHolder is not allocated with bmalloc, but ThreadSafeRefCounted is.  Therefore, Thread is also allocated with bmalloc.  We need to make sure that there's no unsafe interaction between bmalloc and TLS (basically, does bmalloc also depend on TLS ... if so, we need to make sure thread TLS clean up happens before bmalloc TLS clean up).  Otherwise, we need to use a non-bmalloc ThreadSafeRefCounted.
>> 
>> But since this is not an issue introduced in this patch, we can consider fixing it in a subsequent patch (if needed).
> 
> Sounds reasonable. I think bmalloc should do well even in the face of TLS. Otherwise, we cannnot use any values allocated in bmalloc in TLS.
> And I think this issue remains even before introducing WTF::Thread. Before that, we used HashMap to manage thread handle and id.
> If we remove id from the hash map, the hash map could call delete to the bucket. And since the hash map and its buckets are allocated in bmalloc, yeah, it causes issues.
> 
> I've checked it in the current macOS. And it turns out that macOS FastTLS is ok because bmalloc's FastTLS number is smaller than this TLS. (I've tested it in my macOS laptop).
> But anyway, it's safer to use non bmalloc things in TLS destroyed related things. I'll file it separately.

It seems that the current bmalloc is OK with this. Even if bmalloc Cache in TLS is destroyed, it will be re-generated if it becomes necessary (when destroying bmalloc things). And it will be re-destroyed after that.
In the case of macOS, currently, bmalloc TLS is destroyed after the other TLS things are destroyed. I'm not sure it is ensured. But even if it is not ensured, the above mechanism makes it safe.

But this design potentially has several issues.
1. In Windows, TLS revival is not supported. Thus, when porting bmalloc to Windows, we need to do it extra-carefully.
2. If bmalloc Cache is constantly revived, it is simply not efficient. We need to investigate what actually happens especially in Linux platforms. (since macOS uses fast TLS and I ensured it works fine).

[Yusuke Suzuki]

Committed r220217: <http://trac.webkit.org/changeset/220217>