Avoid useless string releases in arginfo.h files #17344
Open
+362
−1,829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
github-actions
bot
added
Category: Build System
Category: Engine
Extension: intl
Extension: bcmath
Extension: curl
Extension: date
Extension: dom
Extension: ffi
Extension: libxml
Extension: mysqli
Extension: pdo (core)
Extension: pdo_dblib
Extension: pdo_firebird
Extension: pdo_mysql
Extension: pdo_odbc
Extension: pdo_pgsql
Extension: pdo_sqlite
Extension: phar
Extension: reflection
Extension: snmp
Extension: soap
Extension: spl
Extension: sqlite3
Extension: standard
Extension: tidy
Extension: tokenizer
Extension: xmlreader
Extension: xsl
Extension: zend_test
labels
nielsdos
requested review from
bukka,
Girgias,
TimWolla,
zeriyoshi,
SakiTakamachi,
devnexen,
NattyNarwhal,
kamil-tekiela,
dstogov,
derickr,
adoy and
kocsismate
as code owners
TimWolla
approved these changes
Jan 4, 2025
kocsismate
approved these changes
Jan 5, 2025
dstogov
reviewed
Jan 9, 2025
Comment on lines
87
to
-90
| ZVAL_LONG(&const_TARGET_CLASS_value, ZEND_ATTRIBUTE_TARGET_CLASS); | ||
| zend_string *const_TARGET_CLASS_name = zend_string_init_interned("TARGET_CLASS", sizeof("TARGET_CLASS") - 1, 1); | ||
| zend_declare_typed_class_constant(class_entry, const_TARGET_CLASS_name, &const_TARGET_CLASS_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_LONG)); | ||
| zend_string_release(const_TARGET_CLASS_name); |
There was a problem hiding this comment.
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.
dstogov
reviewed
Jan 9, 2025
Comment on lines
93
to
-96
| zend_string *const_TARGET_FUNCTION_name = zend_string_init_interned("TARGET_FUNCTION", sizeof("TARGET_FUNCTION") - 1, 1); | ||
| zend_declare_typed_class_constant(class_entry, const_TARGET_FUNCTION_name, &const_TARGET_FUNCTION_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_LONG)); | ||
| zend_string_release(const_TARGET_FUNCTION_name); |
There was a problem hiding this comment.
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.
dstogov
reviewed
Jan 9, 2025
Comment on lines
128
to
+130
| zend_string *property_flags_name = zend_string_init("flags", sizeof("flags") - 1, 1); | ||
| zend_declare_typed_property(class_entry, property_flags_name, &property_flags_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_LONG)); | ||
| zend_string_release(property_flags_name); | ||
| zend_string_release_ex(property_flags_name, 1); |
There was a problem hiding this comment.
Why do we use zend_string_init here?
|
Accidentally messed up the PR id... Sorry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.