[webkit-reviews] review granted: [Bug 174303] [WTF] Drop Thread initialization wait in some platforms by introducing StackBounds::stackBoundsForNewThread(Thread&) : [Attachment 316058] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 27 11:59:36 PDT 2017 

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 174303: [WTF] Drop Thread initialization wait in some platforms by
introducing StackBounds::stackBoundsForNewThread(Thread&)
https://bugs.webkit.org/show_bug.cgi?id=174303

Attachment 316058: Patch

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




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

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

Nice work.  r=me with issues resolved.	Please make sure EWS bots are still
green after edits before landing.  Thanks.

> Source/WTF/ChangeLog:3
> +	   [WTF] Drop Thread initialization wait in some platforms by
introducing StackBounds::stackBoundsForNewThread(Thread&)

I suggest renaming "stackBoundsForNewThread" to simply "threadStackBounds". 
The fact that it takes a Thread& as its argument already tells is that it can
be for any thread.  The name "threadStackBounds" also contrasts well with
"currentThreadStackBounds".

> Source/WTF/ChangeLog:8
> +	   Currently, the caller thread of Thread::create() need to wait
completion of the initialization of the target thread.

wait *for* completion.

> Source/WTF/ChangeLog:10
> +	   StackBounds is retrieved only from the target thread itself.
However, it potentially causes context-switching between

I suggest rephrasing as "StackBounds can only be retrieved by the target thread
itself. However, this potentially causes...".

> Source/WTF/ChangeLog:17
> +	   In this patch, we introduce HAVE_STACK_BOUNDS_FOR_NEW_THREAD and
StackBounds::stackBoundsForNewThread. When creating
> +	   a new thread, we will use StackBounds::stackBoundsForNewThread to
get StackBounds if possible. As a result, we

Let's rename "stackBoundsForNewThread" to "threadStackBounds".

> Source/WTF/ChangeLog:20
> +	   While some documents say that Windows also has a way to get
StackBounds in the similar way[1]. But it relies on

I suggest rephrasing this as "While some documents claim that it is possible on
Windows to get the StackBounds of another thread, the method relies on ..."

> Source/WTF/ChangeLog:22
> +	   use the conservative approach simply waiting completion of thread
initialization.

waiting *for* completion.

> Source/WTF/wtf/StackBounds.cpp:51
> +StackBounds StackBounds::stackBoundsForNewThread(PlatformThreadHandle
thread)

Let's rename to "threadStackBounds".

> Source/WTF/wtf/StackBounds.cpp:59
> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()

Let's rename to "currentThreadStackBoundsInternal".

> Source/WTF/wtf/StackBounds.cpp:71
> +    return stackBoundsForNewThread(pthread_self());

Let's rename to "threadStackBounds".

> Source/WTF/wtf/StackBounds.cpp:76
> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()

Let's rename to "currentThreadStackBoundsInternal".

> Source/WTF/wtf/StackBounds.cpp:89
> +StackBounds StackBounds::stackBoundsForNewThread(PlatformThreadHandle
thread)

Let's rename to "threadStackBounds".

> Source/WTF/wtf/StackBounds.cpp:104
> +StackBounds StackBounds::stackBoundsForNewThread(PlatformThreadHandle
thread)

Let's rename to "threadStackBounds".

> Source/WTF/wtf/StackBounds.cpp:128
> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()

Let's rename to "currentThreadStackBoundsInternal".

> Source/WTF/wtf/StackBounds.cpp:130
> +    auto result = stackBoundsForNewThread(pthread_self());

Let's rename to "threadStackBounds".

> Source/WTF/wtf/StackBounds.cpp:132
> +    result.checkConsistency();
> +    return result;

This is redundant because currentThreadStackBounds() will already call
checkConsistency().  Please remove and simplify.

> Source/WTF/wtf/StackBounds.cpp:137
> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()

Let's rename to "currentThreadStackBoundsInternal".

> Source/WTF/wtf/StackBounds.h:47
> +    static StackBounds stackBoundsForNewThread(PlatformThreadHandle);

Let's rename to "threadStackBounds".

> Source/WTF/wtf/StackBounds.h:49
> +    static StackBounds stackBoundsForCurrentThread()

Let's keep the old name "currentThreadStackBounds" since this function replaces
it.

> Source/WTF/wtf/StackBounds.h:51
> +	   auto result = stackBoundsForCurrentThreadInternal();

Let's rename this "currentThreadStackBoundsInternal" to be consistent with
"currentThreadStackBounds".

> Source/WTF/wtf/StackBounds.h:139
> +    WTF_EXPORT_PRIVATE static StackBounds
stackBoundsForCurrentThreadInternal();

Let's rename to "currentThreadStackBoundsInternal".

> Source/WTF/wtf/Threading.cpp:96
> +void Thread::entryPoint(NewThreadContext* data)

"data" seems out of place.  How about naming this "newThreadContext"?

> Source/WTF/wtf/Threading.cpp:113
> +	   context->thread->m_stack =
StackBounds::stackBoundsForCurrentThread();

Let's rename to "currentThreadStackBounds".

> Source/WTF/wtf/Threading.cpp:133
> +    // Increment ref for the created thread. We do not just use unique_ptr
and leak it to the created thread
> +    // because the ownership of the context should be kept in
> +    // 2. the created thread if HAVE(STACK_BOUNDS_FOR_NEW_THREAD) is true
since Thread::create could exit before starting using context in the created
thread, or
> +    // 1. the creator thread if HAVE(STACK_BOUNDS_FOR_NEW_THREAD) is false
since it needs to wait for completion of the initialization.
> +    // To simplify memory management, we just use ThreadSafeRefCounted and
the creator and created threads share context.
> +    context->ref();

I suggest rephrasing this comment as:

"Increment the context ref on behalf of the created thread. We do not just use
a unique_ptr and leak it to the created thread because both the creator and
created thread has a need to keep the context alive:
1. the created thread needs to keep it alive because Thread::create() can exit
before the created thread has a chance to use the context.
2. the creator thread (if HAVE(STACK_BOUNDS_FOR_NEW_THREAD) is false) needs to
keep it alive because the created thread may exit before the creator has a
chance to wake up from waiting for the completion of the created thread's
initialization. This waiting uses a condition variable in the context.

Hence, a joint ownership model is needed if HAVE(STACK_BOUNDS_FOR_NEW_THREAD)
is false. To simplify the code, we just go with joint ownership by both the
creator and created threads, and make the context ThreadSafeRefCounted."

I think your current version is misleading in 2 places: 
1. "Increment ref for the created thread" sounds like ref'ing the new Thread
instead of NewThreadContext, and 
2. "ownership of the context should be kept in" implies that the motivating
need is to have ownership of the context.  This is not true.  The motivating
need is that the creator and created threads need to keep the context alive
long enough for what they are doing.

> Source/WTF/wtf/Threading.cpp:141
> +	   thread->m_stack =
StackBounds::stackBoundsForNewThread(thread->m_handle);

Let's rename to "threadStackBounds".

> Source/WTF/wtf/Threading.cpp:143
> +	   // In platforms which do not support
StackBounds::stackBoundsForNewThread(), we do not have the way to get stack

Let's rename to "threadStackBounds".  Replace "have the way" with "have a way".

> Source/WTF/wtf/Threading.cpp:146
> +	   // and wait for completion of initialization in the caller side.
After establishing Thread, release the mutex and wait
> +	   // for completion of initialization.

You can drop the last sentence "After establishing ...".  That part is clearly
evident in the code right here.

> Source/WTF/wtf/ThreadingPthreads.cpp:297
> +    thread->m_stack = StackBounds::stackBoundsForCurrentThread();

Let's rename to "currentThreadStackBounds".

> Source/WTF/wtf/ThreadingWin.cpp:259
> +    thread->m_stack = StackBounds::stackBoundsForCurrentThread();

Let's rename to "currentThreadStackBounds".

> Source/WTF/wtf/WTFThreadData.cpp:43
> +    : m_stackBounds(StackBounds::stackBoundsForCurrentThread())

Let's rename to "currentThreadStackBounds".

More information about the webkit-reviews mailing list