[webkit-reviews] review granted: [Bug 208223] Allow setting of stack sizes for threads : [Attachment 391762] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 26 12:03:53 PST 2020


Mark Lam <mark.lam at apple.com> has granted Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 208223: Allow setting of stack sizes for threads
https://bugs.webkit.org/show_bug.cgi?id=208223

Attachment 391762: Patch

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




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 391762
  --> https://bugs.webkit.org/attachment.cgi?id=391762
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>.


More information about the webkit-reviews mailing list