Arginfo: reuse zend_string objects for initializing attribute values#19241
Conversation
Avoid initializing the same string content multiple times and make use of the fact that the strings created to initialize attribute values are not freed by simply making use of an existing zend_string with the same content if one is available.
DanielEScherzer
requested review from
bukka,
derickr,
kamil-tekiela and
kocsismate
as code owners
|
The gen_stub code is kind of ugly, but I'm working on cleaning up that file and will continue to do so, it shouldn't block this improvement This is the third part of my follow-up to #18780 (after #19075 and #19141) dealing with reducing the work of registering attributes on constants (and other things)
Yes I agree. Originally posted by @nielsdos in #18780 (comment) |
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
There's a bit of messiness / code churn in the gen_stub file. Conceptually this looks good though. Leaving the final review for @kocsismate
Thinking about the interning suggestion: it's probably better to not intern these but keep it as-is, otherwise we will also take up shm which is kinda pointless.
| "attribute_{$escapedAttributeName}_{$nameSuffix}_arg{$i}_str" | ||
| ); | ||
| if ($arg->value instanceof Node\Scalar\String_) { | ||
| $declaredStrings[$arg->value->value] = "attribute_{$escapedAttributeName}_{$nameSuffix}_arg{$i}_str"; |
There was a problem hiding this comment.
Perhaps this string that you construct (i.e. the variable name) should be split out to a separate variable, as you already construct it previously on line 3361 too.
There was a problem hiding this comment.
"this string" being "attribute_{$escapedAttributeName}_{$nameSuffix}_arg{$i}_str" ?
There was a problem hiding this comment.
Given that the other instance where the variable is declared is just 3 lines earlier, I don't think a variable is needed
Yeah, its pretty messy, if you look in the history of that file though I've been working to clean it up and given how few people probably look at that file I think the messiness is acceptable |
|
sure |
3 participants
Avoid initializing the same string content multiple times and make use of the fact that the strings created to initialize attribute values are not freed by simply making use of an existing zend_string with the same content if one is available.