Stack Size defaults not honored in web workers starting in 3.1.35

[jnickg]

Trying to update a large project to the latest EMSDK version, and I discovered some strange behavior. As late as EMSDK 3.1.34, the threads instantiated by my wasm module (which is spun up in a web worker) roughly honored flags like STACK_SIZE PKA TOTAL_STACK, and DEFAULT_PTHREAD_STACK_SIZE. For example, if I set DEFAULT_PTHREAD_STACK_SIZE=2MB I would see via PTHREADS_DEBUG instrumentation that the stack range shown by establishStackSpace messages was the correct amount. Sometimes values deviated, I'm guessing due to heuristics or something, but generally they were in the order of magnitude I requested, so it appeared the system was attempting to honor the given flags.

Starting in 3.1.35, threads made through the raw pthread interface stopped honoring this. Perhaps related to this line of code? I am guessing is_main evaluates to false on a web worker. Instead the stack size was 64 KiB (perhaps related to this change?) However, threads started up via the std::thread interface still had the correct stack sizes.

Then, starting in 3.1.39, for reasons I am still unsure of, threads started via std::thread (which BTW has no interface for changing stack size programmatically) ALSO started ignoring stack-size flags. The stack was instead 64 KiB.

Full link command (some of them are artifacts from a CMake dependency with public link flags, included below for completeness):

/path/to/emsdk/upstream/emscripten/em++  \
    -Wno-deprecated-copy -Wno-deprecated-declarations \
    -Wno-inconsistent-missing-override -Wno-unknown-pragmas \
    -Wno-unused-local-typedef -Wno-int-in-bool-context \
    -Wno-implicit-function-declaration -Wno-missing-prototypes \
    -Wno-reorder -Wno-shadow -Wno-shorten-64-to-32 \
    -Wno-sign-compare -Wno-unreachable-code \
    -Wno-unused-parameter -Wno-unused-private-field \
    -Wno-unused-variable -Wno-deprecated-non-prototype \
    -Wno-unused-command-line-argument \
    -Wno-pthreads-mem-growth \
    -std=c++20 -fexceptions -frtti -fwasm-exceptions -fPIC \
    -pthread -msimd128 -msse -msse2 -msse3 -DNDEBUG \
    -Oz -fno-inline-functions \
    @CMakeFiles/my_module.dir/objects1.rsp -o my_module.js \ # not dereferencing since it's probably not relevant
     -sEXPORT_NAME=MyModule \
    -sENVIRONMENT=worker \
    -sEXPORTED_FUNCTIONS=_malloc,_free,_main \
    <my_.a_files_to_link> \
    -ldl --bind -sMODULARIZE=1 -sEXPORT_ES6=1 \
    -sSTACK_SIZE=10485760 -sINITIAL_MEMORY=100mb \
    -sALLOW_MEMORY_GROWTH=1 \
    -sERROR_ON_UNDEFINED_SYMBOLS=1 -sSTRICT=1 \
    -sEXPORTED_RUNTIME_METHODS=[ccall,FS,addFunction,getValue] \
    -sALLOW_TABLE_GROWTH -sFORCE_FILESYSTEM 
    -sWASM_BIGINT -sALLOW_UNIMPLEMENTED_SYSCALLS=1 \
    -sPTHREAD_POOL_SIZE=14 -sPTHREAD_POOL_SIZE_STRICT=0 \
    -sDEFAULT_PTHREAD_STACK_SIZE=524288 

Extra detail: In case it's relevant somehow, we are building through CMake. We were updating from 3.1.8, and ran into issues with the latest version. To reproduce this issue, I built/ran our module with different EMSDK versions, changing flags as little as possible. We were able to update to 3.1.34 and get a stable build with the flags above, only having to remove deprecated flags (which I can dig up if you need them), and change TOTAL_STACK to STACK_SIZE.

You can probably guess, but we use large stack values because our software makes ample use of stack space. Which means that when it's ignored and set to 64 KiB, we quickly crash. Where exactly we crash varies since this is related to threading, but it was usually when locking a mutex, creating another thread, or calling pthread_getspecific()/pthread_setspecific()

[kripken]

I would have guessed #18479 might be related. But based on the versions you mention, perhaps not.

Maybe @sbc100 has an idea?

Otherwise, bisecting might find a quick answer here, as this sounds like a regression.

[jnickg]

I bisected the releases versions to figure out the details I originally posted, but should be able to bisect commits as well. Unfortunately due to a separate issue (still ruling out errors on my end) it will have to be manual, so it might take a day or two. I'll follow up ASAP!

[sbc100]

How are you creating the your pthreads? Are you passing a pthread_attr_t? If so, how are you initializing it?

[sbc100]

The idea with https://github.com/emscripten-core/emscripten/blob/516060b/system/lib/pthread/emscripten_thread_init.c#L19 is that it sets __default_stacksize exactly once at startup when the main thread starts. It should be set to the value of DEFAULT_PTHREAD_STACK_SIZE..

[sbc100]

If you open up your generated JS file, can you take a look at ___emscripten_init_main_thread_js and see what is being passed as the final argument to __emscripten_thread_init

[sbc100]

Oh I see that problem is that you are never running the code on the main browser thread.. I will fix https://github.com/emscripten-core/emscripten/blob/516060b/system/lib/pthread/emscripten_thread_init.c#L19 accordingly

[jnickg]

How are you creating the your pthreads? Are you passing a pthread_attr_t? If so, how are you initializing it?

We create threads in a couple ways. One, using std::thread which was no interface for explicitly setting stack size. In those cases, we rely entirely on Emscripten plumbing to make sure those threads are started with the default stack size. In other cases an in-house library we depend on initializes threads using the pthread interface, and our thread-start code looks like this:

void MyThreadWrapper::Start(uint32_t stackSize) {
	const uint32 kMinStackSize = 512 * 1024;
	int result;

	pthread_attr_t attrs;
	result = pthread_attr_init (&attrs);
	if (result != 0) {
		throw std::runtime_error("failed to init pthread attributes");
        }

	if (stackSize != 0) {
		pthread_attr_setstacksize (&attrs,
			MAX(stackSize,
				kMinStackSize));
        }

	result = pthread_create (&mMyThreadHandle, &attrs, MyThreadWorkerLoop, (void*)this);
	pthread_attr_destroy (&attrs);
	if (result != 0) {
		throw std::runtime_error("failed to create thread!");
        }
        mMyThreadIsStarted = true;
}

In the failing threads, we pass stackSize=0 into this function, meaning we do not explicitly set attributes, and rely on system defaults. There may be other subsystems using this code whose and it is a large code base with many internal customers, so I want to avoid changing this unless absolutely necessary. It seems like, if defaults are honored, we shouldn't need to change this. Let me know if that doesn't sound right 👍

[sbc100]

I found the problem, and the fix is in #19577.

[jnickg]

Thanks @sbc100 -- Just for completeness I'm going to share the bisect results:

• Release introducing the failure when raw pthread interface is used (between 3.1.34 and 3.1.35): 7c767197
• Release introducing the failure when std::thread is used (between 3.1.38 and 3.1.39): 30c90bf6