Arginfo: avoid using temporary zvals for initializing attribute values#19141
Arginfo: avoid using temporary zvals for initializing attribute values#19141DanielEScherzer merged 1 commit intophp:masterfrom
Conversation
DanielEScherzer
requested review from
Girgias,
TimWolla,
adoy,
bukka,
derickr,
devnexen,
kamil-tekiela,
kocsismate,
ndossche and
zeriyoshi
as code owners
|
This is the second part of my follow-up to #18780 (after #19075) dealing with reducing the work of registering attributes on constants (and other things)
Yes I agree. Originally posted by @nielsdos in #18780 (comment) |
github-actions
bot
added
Category: Build System
Extension: openssl
Category: Engine
Extension: intl
Extension: curl
Extension: date
Extension: dom
Extension: enchant
Extension: filter
Extension: libxml
Extension: mysqli
Extension: pgsql
Extension: reflection
Extension: shmop
Extension: soap
Extension: spl
Extension: standard
Extension: xml
Extension: zend_test
labels
Instead of * adding a zval on the stack * initializing it * copying the value to the attribute Just initialize the value directly in the zend_attribute_arg
DanielEScherzer
force-pushed
the
no-copy-value
branch
from
2c509e9 to
bc43982
Compare
|
Comparing the benchmarking results with those from the most recent prior commit that ran CI (a22dc67) (differences in bold)
So everything but |
The difference you're observing can be attributed to noise. Anything around 0.01% difference is too small to say anything useful about. Intuitively, this patch shouldn't make things worse. |
|
Okay, in that case I plan to merge this in a few days if there are no objections (CC @kocsismate) |
| $forStringDef = "{$zvalName}_str"; | ||
| } | ||
| $code .= "\tzend_string *$forStringDef = zend_string_init($cExpr, strlen($cExpr), 1);\n"; | ||
| $code .= "\tZVAL_STR(&$zvalName, $forStringDef);\n"; |
There was a problem hiding this comment.
I suppose this can even be ZVAL_NEW_STR as they're not interned.
There was a problem hiding this comment.
Similarly, all strings that are coming from KNOWN_STRINGS can be set via ZVAL_INTERNED_STR
There was a problem hiding this comment.
This code is used to initialize strings in other places too, not just for the attribute values, so I would want to make a change in the initialization macro in a separate patch
3 participants
Instead of
Just initialize the value directly in the zend_attribute_arg