perf: ZTS: move AG and SCNG into native __thread storage, replay of #22231#22595
perf: ZTS: move AG and SCNG into native __thread storage, replay of #22231#22595henderkes wants to merge 3 commits into
Conversation
henderkes
force-pushed
the
perf/native_tls
branch
2 times, most recently
from
3a4c35a to
31eb0f9
Compare
henderkes
force-pushed
the
perf/native_tls
branch
from
31eb0f9 to
6287e33
Compare
| /* A __thread block of a foreign thread is inaccessible. */ | ||
| if (resource_types_table[i].tls_addr && !own_thread) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
/* A __thread block of a foreign thread is inaccessible. */
Is that because the thread may not exist anymore, or is there another reason?
We need to ensure that shutdown_memory_manager(CG(unclean_shutdown), 1); is called in php_module_shutdown. Currently we avoid calling it in ZTS builds because we rely on the TSRM dtor, I believe.
I would go as far as removing the dtor argument from ts_allocate_tls_id since it's not going to be called for most threads during tsrm_shutdown().
There was a problem hiding this comment.
Is that because the thread may not exist anymore, or is there another reason?
It's pointing to __thread memory of another thread, but the only way it got here is if a foreign thread already died, so we'd be accessing dangling memory. It's technically accessible but there's no telling what running the dtor would lead to.
We need to ensure that shutdown_memory_manager(CG(unclean_shutdown), 1); is called in php_module_shutdown. Currently we avoid calling it in ZTS builds because we rely on the TSRM dtor, I believe.
Is this a general request? I don't change the main thread shutdown path here (it gets dtor defined and is guaranteed to run its own thread). If it's just a general request, I would postpone that to a follow-up PR.
I would go as far as removing the dtor argument from ts_allocate_tls_id since it's not going to be called for most threads during tsrm_shutdown().
Can't currently do that for AG because it does pass and run the dtor.
There was a problem hiding this comment.
The dtor does run but only for the thread that calls tsrm_shutdown() and not for other threads. Before your changes, the dtor was called for all threads during tsrm_shutdown().
There was a problem hiding this comment.
The dtor does run but only for the thread that calls tsrm_shutdown() and not for other threads. Before your changes, the dtor was called for all threads during tsrm_shutdown().
The dtor does run for all threads that exit via ts_free_thread() -> ts_free_resources(). The clean shutdown logic is that no live threads will be registered here anymore except for the main thread during tsrm_shutdown, only failed/crashed threads (for which we can no-longer safely run the dtor for).
There was a problem hiding this comment.
I now see that FrankenPHP calls ts_free_thread(), but other SAPIs don't: they rely on the current behavior of tsrm_shutdown() which is to cleanup all threads at once.
Let's keep the dtor argument since it's useful for SAPIs that call ts_free_thread().
Could you add a comment on tsrm_shutdown() and a note in UPGRADING.INTERNALS?
There was a problem hiding this comment.
Wait... what multi threaded sapi's don't call ts_free_thread()? Cli/fpm are single threaded and other webservers (pasir) or ext-parallel do call it.
Is this the apache handler again?
There was a problem hiding this comment.
It is, of course it is. I don't have a good solution for it, because it doesn't seem like Apache lets you install a thread shutdown hook that we could let ts_free_thread run on. We also can't safely reach into the threads memory without risking a segfault, so the memory just has to leak.
There was a problem hiding this comment.
Yes this is Apache :) I think that's ok in Apache's context since its threads are only stopped when the process itself is going to be killed, which will release those resources.
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
Looks good to me otherwise!
| /* A __thread block of a foreign thread is inaccessible. */ | ||
| if (resource_types_table[i].tls_addr && !own_thread) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Yes this is Apache :) I think that's ok in Apache's context since its threads are only stopped when the process itself is going to be killed, which will release those resources.
Co-authored-by: Arnaud Le Blanc <365207+arnaud-lb@users.noreply.github.com>
|
Thank you @henderkes! |
2 participants
So I largely copied the approach of #22231, but this time for AG and SCNG, because they're small in size (256 bytes, so we don't step back into the TLS surplus issue) and I don't need to touch the JIT.
EG and CG are "fine" to keep in the heap storage with the const offsets, because EG is mostly hot in dispatch and the __thread symbol is already kept loaded in the hybrid VM. For clang compile-heavy code still suffers a bit from tsrm_ls_cache base reloads, but I'm still brainstorming for that.
This mostly benefits the lexer and allocations.
@arnaud-lb