Remove WASM_WORKERS_NO_TLS option #16476
Conversation
…ion by hiding it as an implementation detail - TLS block will be grafted onto the memory area set for the thread stack. Also fix MINIMAL_RUNTIME+WASM_WORKERS+WASM=0 build combination and add a test for that.
juj
force-pushed
the
remove_wasm_workers_no_tls
branch
from
06646f4 to
a21a37b
Compare
There was a problem hiding this comment.
Great simplification!
Do I understand correctly that the usable stack size reduced more TLS memory you use? (i.e. a stack size of 1024 will actually have less than 1024 usable size if the program has TLS?)
BTW, do you think we can also adopt the combined stack+tls approach in the pthreads implementation?
| @@ -1041,8 +1040,7 @@ def __init__(self, **kwargs): | |||
| def get_cflags(self): | |||
| cflags = ['-pthread', | |||
| '-D_DEBUG' if self.debug else '-Oz', | |||
| '-DSTACK_OVERFLOW_CHECK=' + ('2' if self.stack_check else '0'), | |||
| '-DWASM_WORKER_NO_TLS=' + ('0' if self.tls else '1')] | |||
| '-DSTACK_OVERFLOW_CHECK=' + ('2' if self.stack_check else '0')] | |||
There was a problem hiding this comment.
Can you also remove the STACK_OVERFLOW_CHECK variant? As far as I can tell there is no usage of the STACK_OVERFLOW_CHECK macro in the C source code.. unless I'm missing something?
There was a problem hiding this comment.
I can look into that as a separate PR.
Yes, when creating a wasm worker with a "stack size" of 1024, there will then be a stack size of 1024-tlsSize actually available. Maybe the creation parameter could be renamed from "stackLowestAddress" to "stackAndTlsAddress" or something similar, but I was not able to come up with a good name, so kept it as is. So the TLS and stack live side by side in a Wasm Worker.
I think it should be possible, though the interaction with pthread_attr_t semantics needs to be carefully looked at. With Wasm Workers as a new API it is easy to say that if one creates a new worker with X bytes of combined stack+tls, there will be fewer bytes of stack actually available. But if one creates a pthread with pthread_attr_t stack size of X, getting X-some might be a portability issue, so the size calculation might have to be done in the other direction. |
Remove WASM_WORKERS_NO_TLS option, and simplify Wasm Worker TLS creation by hiding it as an implementation detail - TLS block and stack will be initialized to the combined linear memory area passed for the Wasm Worker creation function.
This avoids the need to pass the TLS block size from main thread to Worker thread when creating the Wasm Worker.
Also fix MINIMAL_RUNTIME+WASM_WORKERS+WASM=0 build combination and add a test for that.