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.
Created attachment 391698 [details] WIP Patch
Created attachment 391749 [details] WIP Patch
Created attachment 391760 [details] Patch
Created attachment 391762 [details] Patch
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>.
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.
Created attachment 391768 [details] Patch Addressing review feedback
Created attachment 391769 [details] Patch Address review feedback
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() ?".
Created attachment 391773 [details] Patch More cleanup. Good idea with the stackSize function!
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)`.
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.
Created attachment 391774 [details] Patch Patch for landing
Comment on attachment 391774 [details] Patch Clearing flags on attachment: 391774 Committed r257521: <https://trac.webkit.org/changeset/257521>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59824199>