Bug 174303 – [WTF] Drop Thread initialization wait in some platforms by introducing StackBounds::newThreadStackBounds(PlatformThreadHandle&)

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Let's do this after bug 174291.

[Yusuke Suzuki]

http://www.openwall.com/lists/musl/2013/03/31/3 stack information.

[Yusuke Suzuki]

Created attachment 315924 [details]
Patch

[Build Bot]

Comment on attachment 315924 [details]
Patch

Attachment 315924 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4148604

New failing tests:
wasm.yaml/wasm/js-api/test_memory_constructor.js.wasm-no-call-ic
stress/dont-reserve-huge-capacity-lexer.js.dfg-eager-no-cjit-validate
stress/typedarray-access-neutered.js.ftl-eager
stress/dont-reserve-huge-capacity-lexer.js.ftl-no-cjit-b3o1
stress/dont-reserve-huge-capacity-lexer.js.ftl-no-cjit-small-pool
stress/unshiftCountSlowCase-correct-postCapacity.js.default
wasm.yaml/wasm/js-api/test_memory_constructor.js.default-wasm
stress/activation-sink-osrexit-default-value-tdz-error.js.ftl-eager
wasm.yaml/wasm/js-api/test_memory_constructor.js.wasm-no-tls-context
stress/dont-reserve-huge-capacity-lexer.js.ftl-eager-no-cjit
stress/dont-reserve-huge-capacity-lexer.js.ftl-no-cjit-no-inline-validate
microbenchmarks/map-constant-key.js.dfg-eager-no-cjit-validate
wasm.yaml/wasm/js-api/test_memory_constructor.js.wasm-eager-jettison
stress/activation-sink-default-value-tdz-error.js.dfg-eager
stress/dont-reserve-huge-capacity-lexer.js.dfg-eager
stress/spread-forward-call-varargs-stack-overflow.js.dfg-eager
stress/dont-reserve-huge-capacity-lexer.js.no-llint
stress/dont-reserve-huge-capacity-lexer.js.ftl-no-cjit-validate-sampling-profiler
stress/dont-reserve-huge-capacity-lexer.js.ftl-eager
stress/call-apply-exponential-bytecode-size.js.ftl-eager
stress/dont-reserve-huge-capacity-lexer.js.dfg-maximal-flush-validate-no-cjit
stress/dont-reserve-huge-capacity-lexer.js.ftl-eager-no-cjit-b3o1
stress/class-subclassing-function.js.ftl-eager
stress/dont-reserve-huge-capacity-lexer.js.ftl-no-cjit-no-put-stack-validate
stress/dont-reserve-huge-capacity-lexer.js.no-cjit-collect-continuously
stress/dont-reserve-huge-capacity-lexer.js.default
stress/array-slice-osr-exit-2.js.dfg-eager
stress/dont-reserve-huge-capacity-lexer.js.no-cjit-validate-phases
wasm.yaml/wasm/js-api/test_memory_constructor.js.wasm-no-cjit-yes-tls-context
stress/class-subclassing-function.js.dfg-eager
stress/rest-parameter-many-arguments.js.ftl-eager
stress/spread-forward-call-varargs-stack-overflow.js.ftl-eager
stress/value-to-boolean.js.ftl-eager
apiTests

[Yusuke Suzuki]

Comment on attachment 315924 [details]
Patch

Fixing.

[Build Bot]

Comment on attachment 315924 [details]
Patch

Attachment 315924 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4148725

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 315934 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315924 [details]
Patch

Attachment 315924 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4148711

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 315935 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315924 [details]
Patch

Attachment 315924 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4148717

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 315937 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 315942 [details]
Patch

[Yusuke Suzuki]

Created attachment 315993 [details]
Patch

[Yusuke Suzuki]

Created attachment 315994 [details]
Patch

[Yusuke Suzuki]

Created attachment 316058 [details]
Patch

OK, let's go

[Mark Lam]

Comment on attachment 316058 [details]
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".

[Yusuke Suzuki]

Created attachment 316605 [details]
Patch for landing

[Yusuke Suzuki]

Comment on attachment 316058 [details]
Patch

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

Thank you!

>> 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".

Changed.

>> 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.

Oops, fixed.

>> 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...".

Nice. Fixed.

>> Source/WTF/ChangeLog:17
>> +        a new thread, we will use StackBounds::stackBoundsForNewThread to get StackBounds if possible. As a result, we
> 
> Let's rename "stackBoundsForNewThread" to "threadStackBounds".

Fixed.

>> 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 ..."

Nice.

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

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:51
>> +StackBounds StackBounds::stackBoundsForNewThread(PlatformThreadHandle thread)
> 
> Let's rename to "threadStackBounds".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:59
>> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()
> 
> Let's rename to "currentThreadStackBoundsInternal".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:71
>> +    return stackBoundsForNewThread(pthread_self());
> 
> Let's rename to "threadStackBounds".

Fixed. Renamed it to newThreadStackBounds.

>> Source/WTF/wtf/StackBounds.cpp:76
>> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()
> 
> Let's rename to "currentThreadStackBoundsInternal".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:89
>> +StackBounds StackBounds::stackBoundsForNewThread(PlatformThreadHandle thread)
> 
> Let's rename to "threadStackBounds".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:104
>> +StackBounds StackBounds::stackBoundsForNewThread(PlatformThreadHandle thread)
> 
> Let's rename to "threadStackBounds".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:128
>> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()
> 
> Let's rename to "currentThreadStackBoundsInternal".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:130
>> +    auto result = stackBoundsForNewThread(pthread_self());
> 
> Let's rename to "threadStackBounds".

Fixed.

>> Source/WTF/wtf/StackBounds.cpp:132
>> +    return result;
> 
> This is redundant because currentThreadStackBounds() will already call checkConsistency().  Please remove and simplify.

Right. Dropped.

>> Source/WTF/wtf/StackBounds.cpp:137
>> +StackBounds StackBounds::stackBoundsForCurrentThreadInternal()
> 
> Let's rename to "currentThreadStackBoundsInternal".

Fixed.

>> Source/WTF/wtf/StackBounds.h:49
>> +    static StackBounds stackBoundsForCurrentThread()
> 
> Let's keep the old name "currentThreadStackBounds" since this function replaces it.

Fixed.

>> Source/WTF/wtf/StackBounds.h:51
>> +        auto result = stackBoundsForCurrentThreadInternal();
> 
> Let's rename this "currentThreadStackBoundsInternal" to be consistent with "currentThreadStackBounds".

Fixed.

>> Source/WTF/wtf/StackBounds.h:139
>> +    WTF_EXPORT_PRIVATE static StackBounds stackBoundsForCurrentThreadInternal();
> 
> Let's rename to "currentThreadStackBoundsInternal".

Fixed.

>> Source/WTF/wtf/Threading.cpp:96
>> +void Thread::entryPoint(NewThreadContext* data)
> 
> "data" seems out of place.  How about naming this "newThreadContext"?

Sounds nice. Fixed.

>> Source/WTF/wtf/Threading.cpp:113
>> +        context->thread->m_stack = StackBounds::stackBoundsForCurrentThread();
> 
> Let's rename to "currentThreadStackBounds".

Fixed.

>> Source/WTF/wtf/Threading.cpp:133
>> +    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.

Sounds very fine. I've rephrased this comment with yours.

> Source/WTF/wtf/Threading.cpp:137
> +            return nullptr;

I've added context->deref() for the failure case.

>> Source/WTF/wtf/Threading.cpp:141
>> +        thread->m_stack = StackBounds::stackBoundsForNewThread(thread->m_handle);
> 
> Let's rename to "threadStackBounds".

I think renaming "newThreadStackBounds" is better since this is only effective for newly created threads.
For example, macOS StackBounds::newThreadStackBounds may return invalid value for main thread.
Fixed.

>> 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".

Fixed.

>> Source/WTF/wtf/Threading.cpp:146
>> +        // for completion of initialization.
> 
> You can drop the last sentence "After establishing ...".  That part is clearly evident in the code right here.

Fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:297
>> +    thread->m_stack = StackBounds::stackBoundsForCurrentThread();
> 
> Let's rename to "currentThreadStackBounds".

Fixed.

>> Source/WTF/wtf/ThreadingWin.cpp:259
>> +    thread->m_stack = StackBounds::stackBoundsForCurrentThread();
> 
> Let's rename to "currentThreadStackBounds".

Fixed.

>> Source/WTF/wtf/WTFThreadData.cpp:43
>> +    : m_stackBounds(StackBounds::stackBoundsForCurrentThread())
> 
> Let's rename to "currentThreadStackBounds".

Fixed.

[Yusuke Suzuki]

Committed r219994: <http://trac.webkit.org/changeset/219994>

[Radar WebKit Bug Importer]

<rdar://problem/33596036>

[Radar WebKit Bug Importer]

<rdar://problem/33596013>