Bug 208223 – Allow setting of stack sizes for threads

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Don Olmstead]

Currently all threads use the default stack size and no attempt is made to change the size. This might not be ideal on some platforms or some thread types.

[Don Olmstead]

Created attachment 391698 [details]
WIP Patch

[Don Olmstead]

Created attachment 391749 [details]
WIP Patch

[Don Olmstead]

Created attachment 391760 [details]
Patch

[Don Olmstead]

Created attachment 391762 [details]
Patch

[Mark Lam]

Comment on attachment 391762 [details]
Patch

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

r=me with fixes.

> Source/WTF/ChangeLog:11
> +        based on their own observations. Additionally a default stack size can be specified
> +        during by defining DEFAULT_THREAD_STACK_SIZE_IN_KB if the default stack for a platform is

Can you clarify what you mean here by "specified during"?  I suspect you're missing some details.

> Source/WTF/wtf/Threading.cpp:53
>  
> +#if defined(DEFAULT_THREAD_STACK_SIZE_IN_KB) && DEFAULT_THREAD_STACK_SIZE_IN_KB > 0
> +static constexpr size_t defaultStackSize = DEFAULT_THREAD_STACK_SIZE_IN_KB * 1024;
> +#else
> +// Use the platform's default stack size
> +static constexpr size_t defaultStackSize = 0;
> +#endif

Just move this into Thread::create() and initialize an Optional<size_t> there.

> Source/WTF/wtf/Threading.cpp:162
> +    // If the stack size is set to 0 then use the system default. Platforms can tune the values here.

Please add a comma after '0'.

> Source/WTF/wtf/Threading.h:247
> +    bool establishHandle(NewThreadContext*, size_t);

Please use Optional<size_t> instead of size_t.  Then we don't have to worry about the convention of a size of 0 meaning something special.

> Source/WTF/wtf/posix/ThreadingPOSIX.cpp:204
> +bool Thread::establishHandle(NewThreadContext* context, size_t stackSize)

Use Optional<size_t>.

> Source/WTF/wtf/win/ThreadingWin.cpp:156
> +bool Thread::establishHandle(NewThreadContext* data, size_t stackSize)

Use Optional<size_t>.

[Mark Lam]

Comment on attachment 391762 [details]
Patch

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

>> Source/WTF/wtf/Threading.cpp:53
>> +#endif
> 
> Just move this into Thread::create() and initialize an Optional<size_t> there.

On 2nd thought, let's have a static defaultStackSize() function that returns an Optional<size_t>, and have Thread::create() call that.

[Don Olmstead]

Created attachment 391768 [details]
Patch

Addressing review feedback

[Don Olmstead]

Created attachment 391769 [details]
Patch

Address review feedback

[Mark Lam]

Comment on attachment 391769 [details]
Patch

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

> Source/WTF/wtf/Threading.cpp:48
> +static Optional<size_t> defaultStackSize() {

WebKit style requires the { to be on the next line.

> Source/WTF/wtf/Threading.cpp:173
> +#if PLATFORM(PLAYSTATION)
> +    if (threadType == ThreadType::JavaScript)
> +        stackSize = 512 * 1024;
> +#else
> +    UNUSED_PARAM(threadType);
> +#endif

Hmmm ... further improvement: let rename defaultStackSize() to stackSize() and pass it the ThreadType.  With that, you can move this logic in there, and we can have the computation of the stack size to use all in one place.

> Source/WTF/wtf/posix/ThreadingPOSIX.cpp:212
> +    if (stackSize.hasValue())

You can just say "if (stackSize)" instead.

> Source/WTF/wtf/win/ThreadingWin.cpp:159
> +    unsigned initFlag = stackSize.hasValue() ? STACK_SIZE_PARAM_IS_A_RESERVATION : 0;

Ditto: just use "staskSize ?" instead of "stackSize.hasValue() ?".

[Don Olmstead]

Created attachment 391773 [details]
Patch

More cleanup. Good idea with the stackSize function!

[Yusuke Suzuki]

Comment on attachment 391769 [details]
Patch

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

>> Source/WTF/wtf/Threading.cpp:173
>> +#endif
> 
> Hmmm ... further improvement: let rename defaultStackSize() to stackSize() and pass it the ThreadType.  With that, you can move this logic in there, and we can have the computation of the stack size to use all in one place.

Sounds better.

>> Source/WTF/wtf/win/ThreadingWin.cpp:159
>> +    unsigned initFlag = stackSize.hasValue() ? STACK_SIZE_PARAM_IS_A_RESERVATION : 0;
> 
> Ditto: just use "staskSize ?" instead of "stackSize.hasValue() ?".

Or, use `stackSize.valueOr(0)`.

[Yusuke Suzuki]

Comment on attachment 391773 [details]
Patch

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

r=me

> Source/WTF/wtf/Threading.cpp:174
> +

Remove this line.

[Don Olmstead]

Created attachment 391774 [details]
Patch

Patch for landing

[WebKit Commit Bot]

Comment on attachment 391774 [details]
Patch

Clearing flags on attachment: 391774

Committed r257521: <https://trac.webkit.org/changeset/257521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59824199>