[webkit-reviews] review granted: [Bug 174716] Merge WTFThreadData to Thread::current : [Attachment 316785] 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 174716: Merge WTFThreadData to Thread::current
https://bugs.webkit.org/show_bug.cgi?id=174716

Attachment 316785: Patch

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




--- Comment #32 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 316785
  --> https://bugs.webkit.org/attachment.cgi?id=316785
Patch

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

Nice work.  r=me with fixes.

> Source/WTF/wtf/ThreadHolder.h:33
> -#pragma once
> +#ifndef ThreadHolder_h
> +#define ThreadHolder_h

Please add a comment to document why this is necessary here.  Or revert if this
is not necessary.

> Source/WTF/wtf/ThreadHolder.h:59
>      // Creates and puts an instance of ThreadHolder into thread-specific
storage.
> -    static void initialize(Thread&);
> +    WTF_EXPORT_PRIVATE static ThreadHolder& initialize(Ref<Thread>&&);
> +    WTF_EXPORT_PRIVATE static ThreadHolder& initializeCurrent();

Let's remove the WTF_EXPORT_PRIVATE for initialize() since it should only be
called from slow paths in ThreadHolder*.cpp and Threading*.cpp.
Let's also make them private and add Thread as a friend class.	I think there's
less risk with Thread doing the wrong thing (since Thread is crafted to work
with ThreadHolder) than these being called from client code that should not be
calling it (since there's move semantics on the Thread passed to ThreadHolder,
and initialize() allocates a new ThreadHolder unconditonally).

> Source/WTF/wtf/ThreadHolder.h:88
> +    static WTF_EXPORTDATA ThreadSpecificKey m_key;

Please #if !HAVE(FAST_TLS) m_key.

> Source/WTF/wtf/ThreadHolder.h:112
> +    // WRT WebCore:
> +    //    ThreadHolder is used on main thread before it could possibly be
used
> +    //    on secondary ones, so there is no need for synchronization here.
> +    // WRT JavaScriptCore:
> +    //    Thread::current() is initially called from initializeThreading(),
ensuring
> +    //    this is initially called in a pthread_once locked context.
> +#if !HAVE(FAST_TLS)
> +    if (UNLIKELY(ThreadHolder::m_key == InvalidThreadSpecificKey))
> +	   initializeOnce();
> +#endif

This is not safe.  initializeOnce used to be called under a std::call_once
guard in initializeThreading.  I recommend that you create a
ThreadHolder::initializeOnce in ThreadHolder.cpp that does the std::call_once
guard, and in turn calls ThreadHolder::platformInitializeOnce which is the old
initializeOnce in the Pthread and Win ThreadHolder files.

Oh wait, I just realize that the comment above applies to synchronization of
the call to initializeOnce.  Still, I think this is fragile (it always was). 
But since you're changing the code to always ensure that initializeOnce() is
called and it is only a slow path, I think we can get rid of this comment if we
guard initializeOnce() with a std::call_once.

Also put #if !HAVE(FAST_TLS) around initializeOnce and platformInitializeOnce
(maybe can skip for Windows since it is always !HAVE(FAST_TLS)).

> Source/WTF/wtf/ThreadHolderPthreads.cpp:51
> +    // Ideally we'd have this as a release assert everywhere, but that would
hurt performance.
> +    // Having this release assert here means that we will catch "didn't call
> +    // WTF::initializeThreading() soon enough" bugs in release mode.
>      ASSERT(m_key != InvalidThreadSpecificKey);

Let's make this a RELEASE_ASSERT as the comment indicates.  It doesn't hurt
perf that much since this should only be done once per thread.

On second thought, since the only way to call initialize() is via
ThreadHolder::current(), and ThreadHolder::current() always ensures that
initializeOnce() is called first, then we're not dependent on
WTF::initializeThreading() to be called first anymore.	You can remove this
comment, and just leave the assert as a debug ASSERT as a sanity check.

> Source/WTF/wtf/ThreadHolderWin.cpp:82
> +    // Ideally we'd have this as a release assert everywhere, but that would
hurt performance.
> +    // Having this release assert here means that we will catch "didn't call
> +    // WTF::initializeThreading() soon enough" bugs in release mode.
> +    ASSERT(m_key != InvalidThreadSpecificKey);

Ditto.	You can remove this obsolete comment.

> Source/WTF/wtf/Threading.cpp:99
> +    m_savedLastStackTop = stack().origin();

Can we assert ASSERT(!Thread::current().stack().isEmpty()) before this?

I think it would be nicer if we can just initialize m_stack before this instead
(to consolidate the initialization of the Thread instance initialization, but I
think we can do that in a separate patch if desired.  You can make the
initialization of m_stack on "if (m_stack.isEmpty())".

> Source/WTF/wtf/Threading.cpp:124
>	   context->stage = NewThreadContext::Stage::Initialized;
>	   context->condition.signal();

If we consolidate initialization of m_stack in initializeInThread(), we can
move this signaling part after the call to initializeInThread() below.

> Source/WTF/wtf/Threading.cpp:284
> +	   Thread::current();

If we made ThreadHolder::initializeOnce() guarded with a std::call_once, do we
still need this?

Actually, your ChangeLog claims that "WTF::Thread::current() can be accessed
even before calling WTF::initializeThreading".	That means the 2 are now
independent.  Which, in turn, means:
1. you can remove this.
2. you should definitely make ThreadHolder::initializeOnce() safe by guarding
it with a std::call_once.

> Source/WTF/wtf/Threading.h:278
> +inline Thread& Thread::current()
> +{
> +    return ThreadHolder::current().thread();
> +}

nit: since this is a single line, would you mind just inlining it in the Thread
class definition as:
    static Thread& current() { return ThreadHolder::current().thread(); }

That will make it easier to see at a glance that it's just a forwarding
wrapper.