perf: ZTS: move AG and SCNG into native __thread storage, replay of #22231

[henderkes]

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

[arnaud-lb]

/* 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().

[henderkes]

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.

[arnaud-lb]

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().

[henderkes]

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).

[arnaud-lb]

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?

[henderkes]

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?

[henderkes]

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.

[arnaud-lb]

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]

Looks good to me otherwise!

[arnaud-lb]

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]

Thank you @henderkes!

[henderkes]

@arnaud-lb I'm kind of running out of ideas on how to prevent tls address reloads after every function call. Neither compiler gives the option and neither compiler is able to deem it safe to optimise. So as long as EG and CG stay in heap storage, pointed at by _tsrm_ls_cache + const offset it will always generate a two-instruction reload sequence after every function call. Less of an issue on gcc since all handlers get inlined the giant execute_ex(), but still not optimal.

I'm thinking about ignoring the tls surplus issue and just setting apache2handler's tls model to global-exec. It would obviously degrade apache2 handler quite a bit, but it would benefit cli, cgi, fpm and, notably what I care about, embed/frankenphp.

What do you think? Would this be acceptable? Perhaps cc @iliaal as an apache2handler maintainer.

[arnaud-lb]

Does it also happen with LTO? The compiler should be able to figure that _tsrm_ls_cache doesn't change when calling visible functions

An idea would be to declare _tsrm_ls_cache as a const pointer: __thread void * const _tsrm_ls_cache but updating its value is UB.

Otherwise, it would be interesting to benchmark the effect of global-exec on apache2handler. Maybe the other optimisations offset it?

[henderkes]

Clang 21 lto doesn't do it (it's beneficial, but doesn't omit tls reloads) and gcc can't compile with lto. I patched that to disable lto only on the zend_execute files (where the global register args break lto) but it didn't seem beneficial (though I didn't look at tls reloads).

An idea would be to declare _tsrm_ls_cache as a const pointer: __thread void * const _tsrm_ls_cache but updating its value is UB.

Hmm, that may be fine within core. I'll need to test it, but I can't imagine it works.

Otherwise, it would be interesting to benchmark the effect of global-exec on apache2handler. Maybe the other optimisations offset it?

I strongly doubt it... the globals are hot enough that the tls sequences going from 3 to 1 load account for most of the remaining difference. A function call which means register saving would be very costly, especially on Clang.

I'll benchmark it, if it's acceptable it would be preferable over keeping two entirely different tls layouts. One main motivation for all this is to get rid of NTS and reduce ZTS code complexity and ABI surface, keeping two different layouts in ZTS wouldn't work towards that goal.