Bug 174291 – [WTF] Remove unnecessary indirection of WTF::Thread entry point

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[WTF] Remove unnecessary indirection of WTF::Thread entry point

[Yusuke Suzuki]

Created attachment 314923 [details]
Patch

[Yusuke Suzuki]

Created attachment 314936 [details]
Patch

[Mark Lam]

Comment on attachment 314936 [details]
Patch

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

> Source/WTF/ChangeLog:13
> +        Now wtfThreadEntryPoint is almost the same. Only the difference is function signature due to platform APIs.
> +        We remove ThreadFunctionInvocation indirection in ThreadingPthread.cpp and ThreadingWin.cpp.
> +
> +        And we also simplify ThreadHolder::initialize a bit. Now Thread::create waits for the completion of Thread
> +        initialization. So, after establishing thread handle, we can call ThreadHolder::initialize before completing
> +        Thread initialization.

I think it is useful to add:
"Also, ThreadFunctionInvocation keeps a RefPtr to the Thread object.  This was previously needed to keep the Thread object alive until the thread itself could install the ThreadHolder into its thread local storage.  The ThreadHolder has a Ref that keeps the Thread object alive for the lifetime of the thread.  Since Thread::create() now waits for the thread to be initialized before returning and Thread::create() hold a Ref to the Thread object, we are guaranteed that the Thread object will be alive long enough for it to be installed in the thread's ThreadHolder, and we no longer need ThreadFunctionInvocation."

Hmmm ... now that I've written that, I vaguely remember Geoff telling me a long time ago that the purpose for the ThreadFunctionInvocation is so that the main thread doesn't need to wait for the new thread to start before continuing.  This reduces the amount of forced context switching every time we start a thread.  

Can you explain again why we need to synchronize thread creation and initialization?  I wonder if this is bad for perf somewhere (especially with worker threads).

> Source/WTF/wtf/Threading.cpp:50
> +    Start, Established, Initialized

Let's rename "Established" to "EstablishedHandle".  I think this is clearer about what that stage does.  Saying that the thread is Established vs Initialized is a bit ambiguous without first digging to find out what it means.

> Source/WTF/wtf/Threading.cpp:116
> +    if (!thread->launch(&context))

Let's name this "establishHandle" instead of "launch" since the result of calling it is that it is that the stage is established.

> Source/WTF/wtf/Threading.cpp:118
> +    context.stage = Stage::Established;

Let's rename Stage::Established to Stage::EstablishedHandle.

> Source/WTF/wtf/Threading.h:139
> +    static void entryPoint(void* data);

Let's add "struct NewThreadContext;" at the top of this file, and make this "NewThreadContext*" instead of "void* data".  I think this improves documentation on the intent of what is passed around.

> Source/WTF/wtf/Threading.h:144
> +    bool launch(void* data);

Let's rename this to "establishHandle".
Let's make this "NewThreadContext*" instead of "void* data".

> Source/WTF/wtf/Threading.h:147
>      void establish(pthread_t);

Let's rename this establishPlatformSpecificHandle.  Ditto for non-PTHREADS version (which is really the Windows version) below.

> Source/WTF/wtf/ThreadingPthreads.cpp:203
> +    Thread::entryPoint(data);

Let's cast data to NewThreadContext* here:
     Thread::entryPoint(reinterpret_cast<NewThreadContext*>(data));

> Source/WTF/wtf/ThreadingPthreads.cpp:207
> +bool Thread::launch(void* data)

Let's rename this to "establishHandle" and make it take "NewThreadContext* context" instead of "void* data".

> Source/WTF/wtf/ThreadingPthreads.cpp:221
> +    establish(threadHandle);

Let's rename this to "establishPlatformSpecificHandle".

> Source/WTF/wtf/ThreadingWin.cpp:158
> +    Thread::entryPoint(data);

Let's cast data to NewThreadContext* here:
     Thread::entryPoint(reinterpret_cast<NewThreadContext*>(data));

> Source/WTF/wtf/ThreadingWin.cpp:162
> +bool Thread::launch(void* data)

Let's rename to establishHandle, and change "void* data" to "NewThreadContext* context".

> Source/WTF/wtf/ThreadingWin.cpp:170
> +    establish(threadHandle, threadIdentifier);

Let's rename this to "establishPlatformSpecificHandle".

[Mark Lam]

(In reply to Mark Lam from comment #3)
> Comment on attachment 314936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314936&action=review
> 
> > Source/WTF/ChangeLog:13
> > +        Now wtfThreadEntryPoint is almost the same. Only the difference is function signature due to platform APIs.
> > +        We remove ThreadFunctionInvocation indirection in ThreadingPthread.cpp and ThreadingWin.cpp.
> > +
> > +        And we also simplify ThreadHolder::initialize a bit. Now Thread::create waits for the completion of Thread
> > +        initialization. So, after establishing thread handle, we can call ThreadHolder::initialize before completing
> > +        Thread initialization.
> 
> I think it is useful to add:
> "Also, ThreadFunctionInvocation keeps a RefPtr to the Thread object.  This
> was previously needed to keep the Thread object alive until the thread
> itself could install the ThreadHolder into its thread local storage.  The
> ThreadHolder has a Ref that keeps the Thread object alive for the lifetime
> of the thread.  Since Thread::create() now waits for the thread to be
> initialized before returning and Thread::create() hold a Ref to the Thread
> object, we are guaranteed that the Thread object will be alive long enough
> for it to be installed in the thread's ThreadHolder, and we no longer need
> ThreadFunctionInvocation."
> 
> Hmmm ... now that I've written that, I vaguely remember Geoff telling me a
> long time ago that the purpose for the ThreadFunctionInvocation is so that
> the main thread doesn't need to wait for the new thread to start before
> continuing.  This reduces the amount of forced context switching every time
> we start a thread.  
> 
> Can you explain again why we need to synchronize thread creation and
> initialization?  I wonder if this is bad for perf somewhere (especially with
> worker threads).

For reference, thread creation and establishment/initialization was introduced in r219176: <http://trac.webkit.org/changeset/219176> for https://bugs.webkit.org/show_bug.cgi?id=173975.

[Mark Lam]

(In reply to Mark Lam from comment #4)
> For reference, thread creation and establishment/initialization was
> introduced in r219176: <http://trac.webkit.org/changeset/219176> for
> https://bugs.webkit.org/show_bug.cgi?id=173975.

I meant to say:

For reference, synchronization of thread creation and establishment/initialization was introduced in r219176: <http://trac.webkit.org/changeset/219176> for https://bugs.webkit.org/show_bug.cgi?id=173975.

[Mark Lam]

Comment on attachment 314936 [details]
Patch

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

> Source/WTF/wtf/ThreadHolderWin.cpp:94
>          // Since Thread is not established yet, we use the given id instead of thread->id().

This comment is now stale in this patch.

[Yusuke Suzuki]

Comment on attachment 314936 [details]
Patch

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

>>> Source/WTF/ChangeLog:13
>>> +        Thread initialization.
>> 
>> I think it is useful to add:
>> "Also, ThreadFunctionInvocation keeps a RefPtr to the Thread object.  This was previously needed to keep the Thread object alive until the thread itself could install the ThreadHolder into its thread local storage.  The ThreadHolder has a Ref that keeps the Thread object alive for the lifetime of the thread.  Since Thread::create() now waits for the thread to be initialized before returning and Thread::create() hold a Ref to the Thread object, we are guaranteed that the Thread object will be alive long enough for it to be installed in the thread's ThreadHolder, and we no longer need ThreadFunctionInvocation."
>> 
>> Hmmm ... now that I've written that, I vaguely remember Geoff telling me a long time ago that the purpose for the ThreadFunctionInvocation is so that the main thread doesn't need to wait for the new thread to start before continuing.  This reduces the amount of forced context switching every time we start a thread.  
>> 
>> Can you explain again why we need to synchronize thread creation and initialization?  I wonder if this is bad for perf somewhere (especially with worker threads).
> 
> For reference, thread creation and establishment/initialization was introduced in r219176: <http://trac.webkit.org/changeset/219176> for https://bugs.webkit.org/show_bug.cgi?id=173975.

Waiting for thread initialization in the caller side is important to move stack from wtfThreadData to Thread.
StackBounds information is only collected in the thread itself. If the caller accesses thread->stack() before the target thread initializes it, we have a bad time.
This is why the caller waits for the target thread initialization.

The other possible design is guard thread->stack() with lock. In that case, we should use globalStaticLock heled in ::suspend()/resume() functions.
If we use the other locks (like thread->m_mutex), we encounter dead locks. The reason is described in https://bugs.webkit.org/show_bug.cgi?id=174081's ChangeLog.
In that case, stack() is guarded by the global lock.

BTW, while waiting for initialization is necessary, we can drop waiting for establishment. I'll change this part in a separate patch.

>> Source/WTF/wtf/ThreadHolderWin.cpp:94
>>          // Since Thread is not established yet, we use the given id instead of thread->id().
> 
> This comment is now stale in this patch.

Fixed.

>> Source/WTF/wtf/Threading.cpp:50
>> +    Start, Established, Initialized
> 
> Let's rename "Established" to "EstablishedHandle".  I think this is clearer about what that stage does.  Saying that the thread is Established vs Initialized is a bit ambiguous without first digging to find out what it means.

Fixed.

>> Source/WTF/wtf/Threading.cpp:116
>> +    if (!thread->launch(&context))
> 
> Let's name this "establishHandle" instead of "launch" since the result of calling it is that it is that the stage is established.

Fixed.

>> Source/WTF/wtf/Threading.cpp:118
>> +    context.stage = Stage::Established;
> 
> Let's rename Stage::Established to Stage::EstablishedHandle.

Fixed.

>> Source/WTF/wtf/Threading.h:139
>> +    static void entryPoint(void* data);
> 
> Let's add "struct NewThreadContext;" at the top of this file, and make this "NewThreadContext*" instead of "void* data".  I think this improves documentation on the intent of what is passed around.

Fixed.

>> Source/WTF/wtf/Threading.h:144
>> +    bool launch(void* data);
> 
> Let's rename this to "establishHandle".
> Let's make this "NewThreadContext*" instead of "void* data".

Fixed.

>> Source/WTF/wtf/Threading.h:147
>>      void establish(pthread_t);
> 
> Let's rename this establishPlatformSpecificHandle.  Ditto for non-PTHREADS version (which is really the Windows version) below.

Fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:203
>> +    Thread::entryPoint(data);
> 
> Let's cast data to NewThreadContext* here:
>      Thread::entryPoint(reinterpret_cast<NewThreadContext*>(data));

Fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:207
>> +bool Thread::launch(void* data)
> 
> Let's rename this to "establishHandle" and make it take "NewThreadContext* context" instead of "void* data".

Fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:221
>> +    establish(threadHandle);
> 
> Let's rename this to "establishPlatformSpecificHandle".

Fixed.

>> Source/WTF/wtf/ThreadingWin.cpp:158
>> +    Thread::entryPoint(data);
> 
> Let's cast data to NewThreadContext* here:
>      Thread::entryPoint(reinterpret_cast<NewThreadContext*>(data));

Fixed.

>> Source/WTF/wtf/ThreadingWin.cpp:162
>> +bool Thread::launch(void* data)
> 
> Let's rename to establishHandle, and change "void* data" to "NewThreadContext* context".

Fixed.

>> Source/WTF/wtf/ThreadingWin.cpp:170
>> +    establish(threadHandle, threadIdentifier);
> 
> Let's rename this to "establishPlatformSpecificHandle".

Fixed.

[Yusuke Suzuki]

Created attachment 314956 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 314936 [details]
Patch

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

>>>> Source/WTF/ChangeLog:13
>>>> +        Thread initialization.
>>> 
>>> I think it is useful to add:
>>> "Also, ThreadFunctionInvocation keeps a RefPtr to the Thread object.  This was previously needed to keep the Thread object alive until the thread itself could install the ThreadHolder into its thread local storage.  The ThreadHolder has a Ref that keeps the Thread object alive for the lifetime of the thread.  Since Thread::create() now waits for the thread to be initialized before returning and Thread::create() hold a Ref to the Thread object, we are guaranteed that the Thread object will be alive long enough for it to be installed in the thread's ThreadHolder, and we no longer need ThreadFunctionInvocation."
>>> 
>>> Hmmm ... now that I've written that, I vaguely remember Geoff telling me a long time ago that the purpose for the ThreadFunctionInvocation is so that the main thread doesn't need to wait for the new thread to start before continuing.  This reduces the amount of forced context switching every time we start a thread.  
>>> 
>>> Can you explain again why we need to synchronize thread creation and initialization?  I wonder if this is bad for perf somewhere (especially with worker threads).
>> 
>> For reference, thread creation and establishment/initialization was introduced in r219176: <http://trac.webkit.org/changeset/219176> for https://bugs.webkit.org/show_bug.cgi?id=173975.
> 
> Waiting for thread initialization in the caller side is important to move stack from wtfThreadData to Thread.
> StackBounds information is only collected in the thread itself. If the caller accesses thread->stack() before the target thread initializes it, we have a bad time.
> This is why the caller waits for the target thread initialization.
> 
> The other possible design is guard thread->stack() with lock. In that case, we should use globalStaticLock heled in ::suspend()/resume() functions.
> If we use the other locks (like thread->m_mutex), we encounter dead locks. The reason is described in https://bugs.webkit.org/show_bug.cgi?id=174081's ChangeLog.
> In that case, stack() is guarded by the global lock.
> 
> BTW, while waiting for initialization is necessary, we can drop waiting for establishment. I'll change this part in a separate patch.

In the case of non-Solaris UNIX, we can get StackBounds from the platform thread handle.
So, we can merge this initialiation phase to establishment phase. In that implementation, the caller no longer need to wait the initialiation.

But it requires tweaks in StackBounds for Solaris and Windows. So it should be done after introducing this refactoring.

[Mark Lam]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 314936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314936&action=review
> 
> >>>> Source/WTF/ChangeLog:13
> >>>> +        Thread initialization.
> >>> 
> >>> I think it is useful to add:
> >>> "Also, ThreadFunctionInvocation keeps a RefPtr to the Thread object.  This was previously needed to keep the Thread object alive until the thread itself could install the ThreadHolder into its thread local storage.  The ThreadHolder has a Ref that keeps the Thread object alive for the lifetime of the thread.  Since Thread::create() now waits for the thread to be initialized before returning and Thread::create() hold a Ref to the Thread object, we are guaranteed that the Thread object will be alive long enough for it to be installed in the thread's ThreadHolder, and we no longer need ThreadFunctionInvocation."
> >>> 
> >>> Hmmm ... now that I've written that, I vaguely remember Geoff telling me a long time ago that the purpose for the ThreadFunctionInvocation is so that the main thread doesn't need to wait for the new thread to start before continuing.  This reduces the amount of forced context switching every time we start a thread.  
> >>> 
> >>> Can you explain again why we need to synchronize thread creation and initialization?  I wonder if this is bad for perf somewhere (especially with worker threads).
> >> 
> >> For reference, thread creation and establishment/initialization was introduced in r219176: <http://trac.webkit.org/changeset/219176> for https://bugs.webkit.org/show_bug.cgi?id=173975.
> > 
> > Waiting for thread initialization in the caller side is important to move stack from wtfThreadData to Thread.
> > StackBounds information is only collected in the thread itself. If the caller accesses thread->stack() before the target thread initializes it, we have a bad time.
> > This is why the caller waits for the target thread initialization.
> > 
> > The other possible design is guard thread->stack() with lock. In that case, we should use globalStaticLock heled in ::suspend()/resume() functions.
> > If we use the other locks (like thread->m_mutex), we encounter dead locks. The reason is described in https://bugs.webkit.org/show_bug.cgi?id=174081's ChangeLog.
> > In that case, stack() is guarded by the global lock.
> > 
> > BTW, while waiting for initialization is necessary, we can drop waiting for establishment. I'll change this part in a separate patch.
> 
> In the case of non-Solaris UNIX, we can get StackBounds from the platform
> thread handle.
> So, we can merge this initialiation phase to establishment phase. In that
> implementation, the caller no longer need to wait the initialiation.
> 
> But it requires tweaks in StackBounds for Solaris and Windows. So it should
> be done after introducing this refactoring.

I still don't understand why the thread creator needs to wait for the created thread to start.  Let's consider the following:

Let's say Thread A creates Thread B.
How things work now is that A instantiates a new Thread object with its StackBounds set to empty, and passes this Thread object to B to be initialized.

1. If B has initialized its StackBounds by the time A suspends B and asks for the stack, everything works as we expect.  Nothing special here.
2. If B has NOT initialized its StackBounds by the time A suspends B and asks for the stack, B's thread will have an empty stack bounds, and A should be fine with iterating an empty stack (i.e. a no-op).

The only issue that can arise is a potential race condition where B has only partially initialized its StackBounds when A suspends B.  As a result, A may read an inconsistent stack bounds for B.  We can prevent this by simply having the thread initialization code hold the global suspension lock while initializing.  That ensures that Thread A will only see an empty stack or a properly initialized stack for Thread B because A should only read B's thread data after suspending B.

We also currently set the ThreadID in the new thread (Thread B).  We can also change to setting the TheadID in the constructor for the Thread object (i.e. do it in Thread A) even before the new thread (Thread B) starts running.

Given the above, why does Thread A have to wait for Thread B to be initialized at all?

If it is true that Thread A doesn't have to wait for Thread B to be initialized, then I think we should not get rid of ThreadFunctionInvocation.  ThreadFunctionInvocation allows Thread A create Thread B without forcing a context switch immediately back and forth between the 2 (which is a perf benefit).

What do you think?  Am I missing something?

[Yusuke Suzuki]

Comment on attachment 314936 [details]
Patch

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

>>>>>> Source/WTF/ChangeLog:13
>>>>>> +        Thread initialization.
>>>>> 
>>>>> I think it is useful to add:
>>>>> "Also, ThreadFunctionInvocation keeps a RefPtr to the Thread object.  This was previously needed to keep the Thread object alive until the thread itself could install the ThreadHolder into its thread local storage.  The ThreadHolder has a Ref that keeps the Thread object alive for the lifetime of the thread.  Since Thread::create() now waits for the thread to be initialized before returning and Thread::create() hold a Ref to the Thread object, we are guaranteed that the Thread object will be alive long enough for it to be installed in the thread's ThreadHolder, and we no longer need ThreadFunctionInvocation."
>>>>> 
>>>>> Hmmm ... now that I've written that, I vaguely remember Geoff telling me a long time ago that the purpose for the ThreadFunctionInvocation is so that the main thread doesn't need to wait for the new thread to start before continuing.  This reduces the amount of forced context switching every time we start a thread.  
>>>>> 
>>>>> Can you explain again why we need to synchronize thread creation and initialization?  I wonder if this is bad for perf somewhere (especially with worker threads).
>>>> 
>>>> For reference, thread creation and establishment/initialization was introduced in r219176: <http://trac.webkit.org/changeset/219176> for https://bugs.webkit.org/show_bug.cgi?id=173975.
>>> 
>>> Waiting for thread initialization in the caller side is important to move stack from wtfThreadData to Thread.
>>> StackBounds information is only collected in the thread itself. If the caller accesses thread->stack() before the target thread initializes it, we have a bad time.
>>> This is why the caller waits for the target thread initialization.
>>> 
>>> The other possible design is guard thread->stack() with lock. In that case, we should use globalStaticLock heled in ::suspend()/resume() functions.
>>> If we use the other locks (like thread->m_mutex), we encounter dead locks. The reason is described in https://bugs.webkit.org/show_bug.cgi?id=174081's ChangeLog.
>>> In that case, stack() is guarded by the global lock.
>>> 
>>> BTW, while waiting for initialization is necessary, we can drop waiting for establishment. I'll change this part in a separate patch.
>> 
>> In the case of non-Solaris UNIX, we can get StackBounds from the platform thread handle.
>> So, we can merge this initialiation phase to establishment phase. In that implementation, the caller no longer need to wait the initialiation.
>> 
>> But it requires tweaks in StackBounds for Solaris and Windows. So it should be done after introducing this refactoring.
> 
> I still don't understand why the thread creator needs to wait for the created thread to start.  Let's consider the following:
> 
> Let's say Thread A creates Thread B.
> How things work now is that A instantiates a new Thread object with its StackBounds set to empty, and passes this Thread object to B to be initialized.
> 
> 1. If B has initialized its StackBounds by the time A suspends B and asks for the stack, everything works as we expect.  Nothing special here.
> 2. If B has NOT initialized its StackBounds by the time A suspends B and asks for the stack, B's thread will have an empty stack bounds, and A should be fine with iterating an empty stack (i.e. a no-op).
> 
> The only issue that can arise is a potential race condition where B has only partially initialized its StackBounds when A suspends B.  As a result, A may read an inconsistent stack bounds for B.  We can prevent this by simply having the thread initialization code hold the global suspension lock while initializing.  That ensures that Thread A will only see an empty stack or a properly initialized stack for Thread B because A should only read B's thread data after suspending B.
> 
> We also currently set the ThreadID in the new thread (Thread B).  We can also change to setting the TheadID in the constructor for the Thread object (i.e. do it in Thread A) even before the new thread (Thread B) starts running.
> 
> Given the above, why does Thread A have to wait for Thread B to be initialized at all?
> 
> If it is true that Thread A doesn't have to wait for Thread B to be initialized, then I think we should not get rid of ThreadFunctionInvocation.  ThreadFunctionInvocation allows Thread A create Thread B without forcing a context switch immediately back and forth between the 2 (which is a perf benefit).
> 
> What do you think?  Am I missing something?

Currently, we do not assume that this 'stack' is only accessed when suspending the thread.
And we also do not assume that the stack value returned from Thread can be changed.
It is difficult to understand this semantics from the `Thread::stack()` signature.
So I would like to take the safer way: `Thread::stack()` always returns the desired value.

Furthermore, I think we can drop this initialization waiting for Darwin and UNIX envirnoments (excluding Solaris),
by adding a new static function, StackBounds::stackBounds(Thread&) which takes arbitrary thread.
Once StackBounds change is done, we can have non-waiting threads (for initialization) and they always get desired stack values. (At least on non-Solaris UNIX environments)
I prefer this design because Thread::stack() can always return desired values without waiting thread initialization.

[Mark Lam]

Comment on attachment 314956 [details]
Patch

r=me

[Yusuke Suzuki]

Committed r219654: <http://trac.webkit.org/changeset/219654>