-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Avoid useless string releases in arginfo.h files #17344
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
Conversation
These just contribute to code bloat. For the interned strings we don't need to release them and for the non-interned ones we can use the _ex variant to avoid generating the branching code. Reduces the size of the text section of the CLI binary by 0.166% on a default build.
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.
I was planning to send a patch to do the same thing in the next few days, don't have approval rights but makes sense to me
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.
I would think about specialized functions that accept char* and size_t```. Also may be add specialization by type. Actually, we already have zend_declare_class_constant_long()`` that we used before...
Probably API should be adjusted according to the new requirements.
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.
Slightly surprising, the difference is that zend_declare_class_constant_long (and zend_declare_class_constant etc) create a constant whose value is a long but has no type declaration:
Line 4910 in 9283abc
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.
I have assumption, that in some edge case zend_string_init_interned() may return a non-interned string.
e.g. when DSO extension is loaded at run-time and opcache is active, or when opcache.interned_strings_buffer is insufficient.
Can you please check this.
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.
Why do we use zend_string_init here?
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.
I was not able to find the reason why. zend_declare_property_string etc also do not use an interned string when it creates the string.
It does get interned here however, but only for persistent modules:
Lines 4626 to 4628 in b8ee4c2
Accidentally messed up the PR id... Sorry
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.
zend_string_release_ex(zend_string *s, bool persistent)
Shouldn't that 1 therefore be a true ?
There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.
These just contribute to code bloat. For the interned strings we don't need to release them and for the non-interned ones we can use the _ex variant to avoid generating the branching code.
Reduces the size of the text section of the CLI binary by 0.166% on a default build.