[webkit-reviews] review granted: [Bug 175013] Merge ThreadHolder to WTF::Thread itself : [Attachment 317110] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 175013: Merge ThreadHolder to WTF::Thread itself
https://bugs.webkit.org/show_bug.cgi?id=175013

Attachment 317110: Patch

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




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 317110
  --> https://bugs.webkit.org/attachment.cgi?id=317110
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.