-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Arginfo: reuse zend_string objects for initializing attribute values #19241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
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)
the same strings are allocated twice
It should probably be safe to just intern them all. They are always allocated on start-up, so the memory will be used either way. Interning should just reduce it, no?
Yes I agree.
If you pinky promise that you're gonna look at it before 8.5's release I'll allow it.
Originally posted by @nielsdos in #18780 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this string" being "attribute_{$escapedAttributeName}_{$nameSuffix}_arg{$i}_str" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the other instance where the variable is declared is just 3 lines earlier, I don't think a variable is needed
There's a bit of messiness / code churn in the gen_stub file
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.