Bug 40103 – ThreadingPthread create thread with default stack size which might not be enough

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[Lyon Chen]

In createThreadInternal() of ThreadingPthread.cpp, pthread_create() is called without pthread_attr_t, which means it uses default stack size for thread created. This might not be enough. So we need to check whether the default stack size is smaller than a predefined minimal stack size, and use the predefined minimal stack size instead when necessary.

[Lyon Chen]

Created attachment 57734 [details]
patch for 40103.

[WebKit Review Bot]

Attachment 57734 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lyon Chen]

Created attachment 57735 [details]
patch for 40103 with coding style fix.

This fixes all unrelated coding style issues in ThreadingPthread.cpp file.

[Alexey Proskuryakov]

Comment on attachment 57735 [details]
patch for 40103 with coding style fix.

> This might not be enough.

What exactly the bug is? Secondary thread stack size is also smaller on Mac OS X, and some may say it's "not enough".

From all I can see, this patch just adds mysterious code that's not enabled on any platform. Some questions the code and/or ChangeLog should help to answer are:
- is this code still needed, or can it be removed?
- do I want this on my platform?
- if yes, how do I choose minimal stack size?

> +        Use macro ENABLE_THREAD_STACK_SIZE_CHECK to get this code change compiled so it will not affect most platforms. And macro MINIMAL_THREAD_STACK_SIZE is used to define the minimal thread stack size. This code change will only use MINIMAL_THREAD_STACK_SIZE if the default stack size is smaller than MINIMAL_THREAD_STACK_SIZE.

This line is too long to fit in a reasonably sized window. Please stick to how other ChangeLog entries are formatted.

+    if (stackSize < MINIMAL_THREAD_STACK_SIZE) {

It's best to include the measurement unit in constant name to avoid confusion. Maybe MINIMAL_THREAD_STACK_BYTES?

-    if (result == 0)
+    if (!result)
         return true;

I don't really think that changes like this are an improvement. And in any case, it's best to not make lots of unrelated coding style changes in patches for real bugs.

+    ASSERT(!pthread_attr_init(&attr));

This is wrong - this code won't be compiled in release builds. Same mistake is repeated elsewhere.

+    if (!threadHandle)
+        return 0;
+
+    return establishIdentifierForPthreadHandle(threadHandle);

No need to duplicate the common part of the function.

[Lyon Chen]

(In reply to comment #4)
> What exactly the bug is? Secondary thread stack size is also smaller on Mac OS X, and some may say it's "not enough".
> 
> From all I can see, this patch just adds mysterious code that's not enabled on any platform. Some questions the code and/or ChangeLog should help to answer are:
> - is this code still needed, or can it be removed?
> - do I want this on my platform?
> - if yes, how do I choose minimal stack size?

This patch is needed for any platform that use pthread and its pthread_create() uses too small stack size that it will cause stack overflow. For me 64 * 1024 is fine so far.

> This line is too long to fit in a reasonably sized window. Please stick to how other ChangeLog entries are formatted.
> 

OK, will reformat the change log message.

> +    if (stackSize < MINIMAL_THREAD_STACK_SIZE) {
> 
> It's best to include the measurement unit in constant name to avoid confusion. Maybe MINIMAL_THREAD_STACK_BYTES?

Yeah, this is clearer.

> 
> -    if (result == 0)
> +    if (!result)
>          return true;
> 
> I don't really think that changes like this are an improvement. And in any case, it's best to not make lots of unrelated coding style changes in patches for real bugs.

This is not what I planned to do, but without this change the coding style checked failed my patch, as what it did to my first patch.

> 
> +    ASSERT(!pthread_attr_init(&attr));
> 
> This is wrong - this code won't be compiled in release builds. Same mistake is repeated elsewhere.

Yeah, you are right, forgot that ASSERT won't work on release build.

> No need to duplicate the common part of the function.

OK, I can change this too.

[Alexey Proskuryakov]

> This patch is needed for any platform that use pthread and its pthread_create()
> uses too small stack size that it will cause stack overflow. For me 64 * 1024 is fine so far.

I don't think that this answers all of the questions I posted. I don't ask for complete explanations of things like "how do I choose stack size", but some information for developers who later look at this code would help a lot (with the current patch, someone refactoring this code would likely just delete this part as dead code).

JavaScriptCore limits call depth (for calls that use native stack), and that limit is adjustable. Are you talking about stack overflow in JS code, or in some other code? Can this be resolved by adjusting JS call depth for your platform instead?

> This is not what I planned to do, but without this change the coding style checked
> failed my patch, as what it did to my first patch.

You should feel free to ignore style bot complaints if it's complaining about code you didn't change.

[Lyon Chen]

(In reply to comment #6)
New patch is still building, so I didn't upload it. But if the default stack size is too small (say 1024 bytes), do you think it's enough for all threads, like database thread, worker thread? Even after we adjust the JavaScriptCore call depth?

OK, glad to know I can ignore unrelated coding style errors. But I was asked to fix non-related coding style error before, in bug 35530.

[Lyon Chen]

Created attachment 57841 [details]
patch for 40103.

[Lyon Chen]

(In reply to comment #6)

Just verified without the code change it will crash, even after I change MaxSmallThreadReentryDepth from 32 to 16.

And I think this code change could help other platforms, especially the mobile platforms which tend to give out smaller stack size.

[Adam Barth]

Comment on attachment 57841 [details]
patch for 40103.

This patch is blocked on answering ap's questions above.

[Yong Li]

(In reply to comment #6)
> > This patch is needed for any platform that use pthread and its pthread_create()
> > uses too small stack size that it will cause stack overflow. For me 64 * 1024 is fine so far.
> 
> I don't think that this answers all of the questions I posted. I don't ask for complete explanations of things like "how do I choose stack size", but some information for developers who later look at this code would help a lot (with the current patch, someone refactoring this code would likely just delete this part as dead code).
> 
> JavaScriptCore limits call depth (for calls that use native stack), and that limit is adjustable. Are you talking about stack overflow in JS code, or in some other code? Can this be resolved by adjusting JS call depth for your platform instead?
> 
> > This is not what I planned to do, but without this change the coding style checked
> > failed my patch, as what it did to my first patch.
> 
> You should feel free to ignore style bot complaints if it's complaining about code you didn't change.

This is not for JSC stack, but for threads WebKit creates for other purposes like database, html5 worker, blabla.

This was an issue on one of our platforms where the default stack size isn't big enough.

However Rob is trying to working on this with a new bug https://bugs.webkit.org/show_bug.cgi?id=91899

[Alexey Proskuryakov]

> This is not for JSC stack, but for threads WebKit creates for other purposes like database, html5 worker, blabla.

I don't understand how this answers the question. Web Workers run JavaScript, too.

[Yong Li]

(In reply to comment #12)
> > This is not for JSC stack, but for threads WebKit creates for other purposes like database, html5 worker, blabla.
> 
> I don't understand how this answers the question. Web Workers run JavaScript, too.

There was no actual code path to blame, because the default stack size was insanely small at that moment, IIRC. But it would be nice if WebKit can create a thread with specified stack size (or stack size types), so we can use small stacks for some threads like BlockFreeingThread, and large stacks for web workers.