perf: move EG() and CG() in ZTS builds into __thread storage - #23227
perf: move EG() and CG() in ZTS builds into __thread storage#23227henderkes wants to merge 32 commits into
Conversation
… packagers want to set the glibc tls surplus in their httpd package
…s are in TLS storage)
arnaud-lb
left a comment
There was a problem hiding this comment.
I like the --with-tsrm-tls-model idea.
Did a first pass, but I will take the time to review carefully.
b78839c to
5d9e202
Compare
henderkes
left a comment
There was a problem hiding this comment.
It's a bit of a pain to find these on mobile, so I'll pin just these two, but there's more.
It could eventually be reworked when all the symbols move directly into thread storage, but I've not even begun thinking about it. There's probably no point except for zend_ini_scanner_globals and what we still have in the front.
| #if defined(ZEND_WIN32) && !defined(LIBZEND_EXPORTS) | ||
| /* Windows can't dllexport __declspec(thread) symbols, so outside Zend each module | ||
| * keeps a per-module `void *` pointer and reaches EG/CG via the resource-id indirection. */ | ||
| # define ZEND_TSRMLS_CACHE_T void * |
There was a problem hiding this comment.
The symbol is referenced as just a void* on windows
|
Created the aarch64 global-dynamic first first, but I may as well look into teaching the JIT to take the address of _tsrm_ls_cache + offset instead of the mandatory load for a bit of JIT speedup. |
|
@arnaud-lb out of zend_ini_scanner_globals))
virtual_cwd_globals))
zend_signal_globals_t))
zend_gc_globals_size())
php_core_globals))
sapi_globals_struct))
zend_accel_globals))
zend_jit_globals))Which do you think would make sense moving too? gc globals probably and maybe virtual_cwd_globals? |
|
alloc_globals is likely the most accessed global as every emalloc/efree and related fetch |
Proving too far outside my expertise. I think this is reviewable like this now, the JIT optimization could be done later in IR side to let us drop the Unless @dstogov would like to have a go at it. |
arnaud-lb
left a comment
There was a problem hiding this comment.
The changes look good in general, but it's a bit hard to follow. Possibly grouping abstractions in TSRM would help? Right now you need to have TSRM.*, zend.c, zend_globals_macros.h to understand what's going on.
The Windows changes make sense, but maybe @shivammathur can take a look as well?
ts_free_resources(), ts_free_thread(), ts_free_id() and ts_apply_for_id() matched entries with p->thread_id == tsrm_thread_id(). That is ambiguous in exactly the case the surrounding code exists to handle: a stale entry of a dead thread can carry the live thread's recycled id. Compare against the entry that tsrm_tls_get() hands out instead. The recycle path in ts_resource_ex() no longer runs the stale entry's destructors. It used to point the TLS cache at an entry whose native TLS block had died with its thread, so any destructor reaching for EG or CG read freed memory. Leaking the dead thread's module globals is the better trade; a child process that recycles thread ids gets respawned by the SAPI anyway. Hold the shutdown marker across the window so that signal handlers stay away from both the stale entry and its replacement. Also reset tls_key, the TSRM tables and TSRMLS_CACHE on shutdown.
f5c7bcc to
4c08c6c
Compare
From @iliaal. I'll try approaching this with global per-thread cleanup hooks, but leave persistent_list in EG and wire ts_free_thread to clean it up. |
…free_thread` at thread exit SAPI's should already been doing this on their own before, but the apache2handler doesn't get control over threads, so it didn't call ts_free_thread and therefore started leaking resources with the move of per-thread globals from heap storage to native __thread storage
| } | ||
| do { | ||
| zend_try { | ||
| zend_destroy_rsrc_list(&EG(persistent_list)); |
There was a problem hiding this comment.
this is technically a bugfix, plist previously leaked on threads that unregistered with ts_free_thread().
|
@iliaal could you take a look to see if the change looks acceptable to you? |
| free(data); | ||
| return; | ||
| } | ||
| if (FlsGetValue(tsrm_exit_key) != data) { |
There was a problem hiding this comment.
Does FlsGetValue(tsrm_exit_key) still return data when this callback fires at thread exit? MSDN documents lpFlsData only as "the value stored in the FLS slot" and says nothing about whether the slot is cleared first. If it is cleared, this returns before ts_free_thread() and the Windows half of the automatic cleanup never runs. I have no Windows box to settle it.
There was a problem hiding this comment.
Yes it runs, I've tested on musl, glibc and Windows.
Edit: clarifying wording, I've fully tested everything on musl, glibc and windows. Fls callbacks obviously only on Windows.
| ZEND_API void zend_win_tsrm_cache_shutdown(void); | ||
| ZEND_API zend_tsrm_ls_cache *zend_win_tsrm_cache_fallback(void); | ||
| # ifdef __clang__ | ||
| static __inline__ __attribute__((const, always_inline)) zend_tsrm_ls_cache *zend_win_tsrm_cache_ptr(void) |
There was a problem hiding this comment.
const is formally wrong here: the function reads zend_win_tsrm_cache_offset and gs: memory. It is harmless in practice since the published value is thread-invariant, and the CSE it enables is safer than re-reading gs after a TlsFree. __readgsqword under const gives the same instruction on clang-cl, so the clang branch and its asm could go and leave one implementation.
There was a problem hiding this comment.
Formally wrong, but safe in our circumstances and it gets clang to cse where it otherwise wouldn't.
Technically we can always &_tsrm_ls_cache, it's just not optimal codegen.
__readgsqword under const gives the same instruction on clang-cl, so the clang branch and its asm could go and leave one implementation.
Not on PC to test this atm but I'm 98% sure it resulted in unnecessary reloads under Clang 21 and 22.
| AS_VAR_IF([PHP_THREAD_SAFETY], [yes], [ | ||
| AS_CASE([$PHP_TSRM_TLS_MODEL], | ||
| [auto], [ | ||
| AS_VAR_IF([php_tsrm_dlopened_sapi], [yes], |
There was a problem hiding this comment.
apache2handler is the only SAPI setting this. sapi/embed also builds a libphp.so under --enable-embed=shared, so a host that dlopens it rather than linking against it gets initial-exec and the static TLS surplus failure this flag exists to avoid. Setting it there too, or stating that embed is link-time only, would close the gap.
There was a problem hiding this comment.
I'm not aware of any embed users that don't link against it. If there are, I'm happy to declare embed sapi as dlopened too, I can always overwrite the model.
| - AG and SCNG are now allocated with ts_allocate_tls_id() and live in native | ||
| __thread storage on ZTS builds. | ||
| - tsrm_shutdown() and the bundled library-unload hooks automatically disarm | ||
| thread-exit cleanup. Embedders must stop and join all PHP worker threads |
There was a problem hiding this comment.
Two cases this does not cover. A detached thread runs ts_free_thread() from the new exit callback but can never be joined, so "stop and join all PHP worker threads" is unsatisfiable for it. And when a thread is still alive here, what actually survives is its persistent PDO/mysqli/pgsql connections, which is worth naming since it is the visible consequence.
There was a problem hiding this comment.
Poor wording, I think. Embedders should explicitly call ts_free_thread, only if they don't, the callbacks do the cleanup automatically, unless they call zend_shutdown before (which is an obviously terrible idea, but is unchanged by this PR).
|
Nothing blocking from me, the inline notes are all nits. |
replay of #22231
Moves EG and CG into __thread storage after all. We first moved them into constant offsets (#22287) from *_tsrm_ls_cache, but I couldn't find a way to stop gcc or clang from reloading _tsrm_ls_cache base pointer between function calls, leading to an extra pointer load once per function.
This eliminates the pointer load, making the access sequence to EG/CG just a single mov (x64) / mrs + add + ldr (aarch64) under local-exec. initial-exec likewise loses the pointer load so 3 -> 2 instructions (x64).
cc @arnaud-lb
What I'm adding here to counter the global-dynamic fallback slowdown is the option to explicitly opt-in to initial-exec, under the knowledge that host programs loading it will need to increase the static tls surplus. This is not an issue for package providers.
PS: Actually figured out that the explicit model choice doesn't happen for musl, not sure about IE (static tls surplus on musl?), but LE should work just fine. That's for another PR though.