Hi internals,
Is there any hope to have https://github.com/php/php-src/pull/16565 merged before 8.5? It's too late to address all the locks, but having it as part of the ABI would be nice so we can start fixing them for 8.6/9.0. In FrankenPHP, if something takes a lock, it can significantly affect performance. By using r/w locks instead of exclusive locks, we can use shared locks when reading instead of exclusive locks.
— Rob
Hi internals,
Is there any hope to have https://github.com/php/php-src/pull/16565 merged before 8.5? It's too late to address all the locks, but having it as part of the ABI would be nice so we can start fixing them for 8.6/9.0. In FrankenPHP, if something takes a lock, it can significantly affect performance. By using r/w locks instead of exclusive locks, we can use shared locks when reading instead of exclusive locks.
— Rob
My educated guess would be that environ is per process.
-- hakre
Hi internals,
Is there any hope to have https://github.com/php/php-src/pull/16565 merged before 8.5? It's too late to address all the locks, but having it as part of the ABI would be nice so we can start fixing them for 8.6/9.0. In FrankenPHP, if something takes a lock, it can significantly affect performance. By using r/w locks instead of exclusive locks, we can use shared locks when reading instead of exclusive locks.
— Rob
My educated guess would be that environ is per process.
-- hakre
Hi Hakre,
We already solved the environment locks in FrankenPHP (which uses threads instead of processes in ZTS mode) by basically “deleting” the built-in environment functions and replacing them. Environment access is not thread safe, and Go (the language of FrankenPHP) uses its own locks around environment access. Thus we needed to prevent concurrent access to the environment in the two runtimes (TSRM vs. Go). That part of the PR will just help with other SAPI performance when using PHP compiled in ZTS mode.
The main feature of the PR is not the environment locks, but the adding of non-exclusive locks to TSRM. For example, there are TSRM locks in OPcache, but they’re only used during writes and don’t protect reads. This can lead to very subtle bugs and crashes that we see sporadically in FrankenPHP. By enabling r/w locks (the new tsrm_rwlock_rlock in this PR), we can protect reads and writes, without degrading existing performance.
TSRM itself could use this as well, so that every time FrankenPHP spawns a new PHP thread, it doesn’t need to contend for reading/writing globals during startup (and for dealing with cases where TSRM cache can’t be used, which affects ALL global reads/writes if I understand correctly — which is possible I don’t).
I believe there are several other instances that would benefit from r/w locks as well, such as mysqlnd, but it has been awhile since I looked at this, and I’m just going off notes.
— Rob
Hi internals,
Is there any hope to have https://github.com/php/php-src/pull/16565 merged before 8.5? It's too late to address all the locks, but having it as part of the ABI would be nice so we can start fixing them for 8.6/9.0. In FrankenPHP, if something takes a lock, it can significantly affect performance. By using r/w locks instead of exclusive locks, we can use shared locks when reading instead of exclusive locks.
— Rob
My educated guess would be that environ is per process.
-- hakre
Hi Hakre,
We already solved the environment locks in FrankenPHP (which uses threads instead of processes in ZTS mode) by basically “deleting” the built-in environment functions and replacing them. Environment access is not thread safe, and Go (the language of FrankenPHP) uses its own locks around environment access. Thus we needed to prevent concurrent access to the environment in the two runtimes (TSRM vs. Go). That part of the PR will just help with other SAPI performance when using PHP compiled in ZTS mode.
Well if you go go-routine level read-only this is probably safe, but it probably depends on what you expect to find on the top of _SERVER per each SAPI. IIRC for FrankenPHP this is EMBED via CGO.
The main feature of the PR is not the environment locks, but the adding of non-exclusive locks to TSRM. For example, there are TSRM locks in OPcache, but they’re only used during writes and don’t protect reads. This can lead to very subtle bugs and crashes that we see sporadically in FrankenPHP. By enabling r/w locks (the new tsrm_rwlock_rlock in this PR), we can protect reads and writes, without degrading existing performance.
If you see a benefit and it's across SAPIs -- why not consider it? If it's that stable and works under any process model that folllows the C semantics of GO (and PHP), this should be portable/transparent, shouldn't it?
TSRM itself could use this as well, so that every time FrankenPHP spawns a new PHP thread, it doesn’t need to contend for reading/writing globals during startup (and for dealing with cases where TSRM cache can’t be used, which affects ALL global reads/writes if I understand correctly — which is possible I don’t).
I believe there are several other instances that would benefit from r/w locks as well, such as mysqlnd, but it has been awhile since I looked at this, and I’m just going off notes.
I don't know those API/ABI good enough, but perhaps Johannes could know on spot for your question.
— Rob
-- hakre