Req #80835 [Csd]: suggested code cleanup for ZEND_ENABLE_STATIC_TSRMLS_CACHE
Edit report at https://bugs.php.net/bug.php?id=80835&edit=1
ID: 80835
User updated by: theultramage at gmail dot com
Reported by: theultramage at gmail dot com
Summary: suggested code cleanup for
ZEND_ENABLE_STATIC_TSRMLS_CACHE
Status: Closed
Type: Feature/Change Request
Package: Scripting Engine problem
Operating System: FreeBSD 12.2
PHP Version: 8.0.3
Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
> Feel free to provide pull requests
I am not a code contributor, nor am I familiar with the codebase. Submitting TSRM issue #80814
required reverse-engineering its code's purpose, and the excessive amount of macro stacking,
wrappers and conditional compilation flags made researching the issue take way longer than it should
have. This write-up describes one of the frustrations I had with the code. Unfortunately I do not
have any more spare time to invest in this. I might revisit this in the future.
> external extensions
Ah, so third-party extensions come into play here. If I understood right, the STATIC_TSRMLS_CACHE
thing is just a cpu optimization to save a few ticks when accessing threaded global variables, so
there is no functional difference whether the feature is turned on or off, and thus no real reason
to care about it when writing a third-party module. If this is indeed the case, then since every
phpext makefile already force enables it, the proposed cleanup would just get rid of all references
to the flag and instead set it to 1 inside Zend.h right before the place it's used. If
third-party backwards compat is a concern, the header could instead check if the flag is being
explicitly set to 0.
> The additional wrappers are likely for loose coupling. Zend should ideally not rely on TSRM.
Staring at that part of Zend.h with this statement in mind, it does seem like all it's doing is
implementing an on/off toggle for the cache. What I'm not sure about is why it needs to be in
Zend.h, rebranded as ZEND_*. I guess it does implement a sort of loose coupling when the cache
feature is turned off and you reaaaaally want to avoid including tsrm.h. This is mainly due to those
global setup macros that the cache feature forces onto every module.
Previous Comments:
------------------------------------------------------------------------
[2021-05-07 17:36:51] cmb@php.net
Since we don't have control over external extensions, setting the
flag for each extension is likely necessary. The additional
wrappers are likely for loose coupling (Zend/* should ideally not
rely on TSRM/*).
Anyhow, I don't think it is helpful to keep this ticket open. Feel
free to provide pull requests[1] for code cleanup, instead.
[1] <https://github.com/php/php-src/pulls>
------------------------------------------------------------------------
[2021-03-05 14:11:39] theultramage at gmail dot com
Description:
------------
In 2014, a bunch of defines for the experimental TSRM cache were added to zend.h, and across
multiple commits, they were applied to the base code and extensions code, as well as their build
scripts.
https://github.com/php/php-src/commit/76081df168829a5cc0409fac47c217d4927ec6f6
https://github.com/php/php-src/commit/5749b4a9979cd3ff85996323bed9adc1bd182f76
In particular, the ZEND_ENABLE_STATIC_TSRMLS_CACHE define acts as an on/off toggle. And for some
reason, the developer decided to add the compiler flag ZEND_ENABLE_STATIC_TSRMLS_CACHE=1 into every
possible m4 makefile. I have questions.
- Was it necessary to add this level of control over that flag, making a mess in the process? I now
suspect that the sole reason it's there is because the developer was going through the
extensions one by one, and only wanted to turn the cache defines on after they made the required
code edits. In that case, this shim should have been removed before the development branch was
merged.
- Was it necessary to add a set of wrapper defines to zend.h that just point to tsrm.h?
Couldn't this have been contained within tsrm.h, to avoid having to rebrand the TSRM api as
ZEND_TSRM through a bunch of cosmetic src edits? All the extensions still #include
"TSRM.h", and 18 of those don't even use TSRM.
- In several places the work wasn't done, and those still directly use the non-cache TSRM
macros. In some others, the code directly calls cache-specific TSRM macros, leading to a compilation
error if the cache is disabled. See https://bugs.php.net/bug.php?id=80823
------------------------------------------------------------------------
--
Edit this bug report at https://bugs.php.net/bug.php?id=80835&edit=1
Thread (3 messages)