[webkit-reviews] review granted: [Bug 174716] Merge WTFThreadData to Thread::current : [Attachment 316871] 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 316871: Patch

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




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

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

r=me with remaining issues resolved.

> Source/WTF/wtf/ThreadHolder.h:83
> +    WTF_EXPORT_PRIVATE static void ensureKey();

I think you can call this initializeKey() or keep the original name
initializeOnce().  I suggested ensureKey() back when I was thinking that it
will do the check for "if (UNLIKELY(ThreadHolder::m_key ==
InvalidThreadSpecificKey))".  Since that's not necessary anymore,
initializeOnce is a better name here.

> Source/WTF/wtf/ThreadHolderPthreads.cpp:40
> +void ThreadHolder::ensureKey()

Let's change this back to initializeOnce (for reasons above).

> Source/WTF/wtf/ThreadHolderWin.cpp:52
> -void ThreadHolder::initializeOnce()
> +void ThreadHolder::ensureKey()

Let's change this back to initializeOnce (for reasons above).

> Source/WTF/wtf/Threading.cpp:100
> +    if (m_stack.isEmpty())
> +	   m_stack = StackBounds::currentThreadStackBounds();

I just re-read Thread::create() and saw that "thread->m_stack =
StackBounds::newThreadStackBounds(thread->m_handle)" is only executed after the
new thread has started.  That means there's a race to set m_stack.  However,
since both should yield the same result, it should not matter that they
overwrite each other with the same m_stack value.  If you think we cannot
guarantee that m_stack will end up with the same value, then we can eliminate
the race by making initializeInThread() take a "needStackBoundInitialization"
argument and only initialize it based on that.

> Source/WTF/wtf/Threading.cpp:287
> +	   ThreadHolder::ensureKey();

Let's rename this back to initializeOnce (for reasons above).