Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Use the same zend_arg_info struct for internal and user functions #19022

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

Open
arnaud-lb wants to merge 4 commits into php:master
base: master
Choose a base branch
Loading
from arnaud-lb:zstr-arg-info

Conversation

Copy link
Member

@arnaud-lb arnaud-lb commented Jul 3, 2025 •
edited
Loading

Internal functions use char* strings to represent arg names and default values. This differs from user functions, which use zend strings.

Here I unify this by using the same struct, zend_arg_info in both function types.

This simplifies accesses to arg infos. Also, in the PFAs RFC this avoids converting internal arg infos at runtime when applying an internal function.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having a look at this, I'm overall in favour of unifying those, as it makes it easier to reason about.

See some of my comments, which are mostly nits. :)

arnaud-lb reacted with thumbs up emoji
}
smart_str_append_printf(str, "$%s", has_internal_arg_info(fptr)
? ((zend_internal_arg_info*)arg_info)->name : ZSTR_VAL(arg_info->name));
smart_str_append_printf(str, "$%s", ZSTR_VAL(arg_info->name));
Copy link
Member

@Girgias Girgias Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this with a printf? Can't we just append a char and then the zend_string?

Copy link
Member Author

@arnaud-lb arnaud-lb Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, but I want to avoid changes that are not directly related

Copy link
Member

@Girgias Girgias Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, let's keep this for a follow-up then :)

Comment on lines 36 to 39
if (arginfo) {
if (func->type == ZEND_INTERNAL_FUNCTION) {
arg_name = (char *) ((zend_internal_arg_info *) &arginfo[i])->name;
} else {
arg_name = ZSTR_VAL(arginfo[i].name);
}
arg_name = ZSTR_VAL(arginfo[i].name);
}
smart_str_appends(s, arg_name ? arg_name : "?");
Copy link
Member

@Girgias Girgias Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be improved to prevent a strlen computation within the smart_str_appends.

Copy link
Member

nielsdos commented Jul 3, 2025

You can probably get rid of ZEND_ACC_USER_ARG_INFO

Copy link
Member Author

Right :) I wanted to do it, but it's not trivial because the flag is also used to signal how arg default values should be fetched. I may try to remove it later.

@arnaud-lb arnaud-lb marked this pull request as ready for review July 3, 2025 16:16
fprintf(stderr, "Evaluating %s via AST\n", ZSTR_VAL(default_value));
#endif
return get_default_via_ast(default_value_zval, default_value);
return get_default_via_ast(default_value_zval, ZSTR_VAL(default_value));
Copy link
Member

@Girgias Girgias Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up, the get_default_via_ast() function should be changed to accept a zend_string to get rid of an unnecessary strlen() computation :)

arnaud-lb reacted with thumbs up emoji
Copy link
Member

nielsdos commented Jul 3, 2025

This does seem to have a negative impact on the Valgrind instruction counts though. Probably because of the slightly higher memory requirement if you have the zend_string header too.

if (!required && !ZEND_ARG_IS_VARIADIC(arg_info)) {
if (fptr->type == ZEND_INTERNAL_FUNCTION) {
smart_str_appends(str, " = ");
/* TODO: We don't have a way to fetch the default value for an internal function
Copy link
Member

@DanielEScherzer DanielEScherzer Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still correct?

Copy link
Member Author

@arnaud-lb arnaud-lb Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is still correct. This is also related to why we can not remove ZEND_ACC_USER_ARG_INFO immediately.

Copy link
Member Author

@nielsdos I believe this is purely startup/shutdown overhead. When ignoring startup/shutdown, I see absolutely zero degradation under valgrind.
nielsdos reacted with thumbs up emoji

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object.
This wasn't made before to avoid the waste of memory.
Now names are going to be duplicated from .text to heap and shm.
This is not a big problem, but it's better to measure this in some way.

arnaud-lb reacted with thumbs up emoji
Copy link
Member Author

I've measured zero regression on wall time or valgrind icount on the symfony benchmark. I will measure the memory impact once I get back from vacations.

NB: I've also tried declaring zend strings in static storage, but there are a number of complications that make the current solution more practicable. The most annoying one is that we can't initialize a zend string directly due to the variable size member.

Copy link
Member

dstogov commented Jul 7, 2025

NB: I've also tried declaring zend strings in static storage, but there are a number of complications that make the current solution more practicable. The most annoying one is that we can't initialize a zend string directly due to the variable size member.

I know. I tried this years ago and gave up :)
arnaud-lb reacted with laugh emoji

Copy link
Member Author

I don't object. This wasn't made before to avoid the waste of memory. Now names are going to be duplicated from .text to heap and shm. This is not a big problem, but it's better to measure this in some way.

With a realistic set of extensions: Core,ctype,date,dom,FFI,fileinfo,filter,gmp,hash,iconv,intl,json,lexbor,libxml,mbstring,mysqli,mysqlnd,openssl,pcre,PDO,pdo_mysql,pdo_sqlite,Phar,posix,random,Reflection,session,SimpleXML,sockets,SPL,sqlite3,standard,tokenizer,uri,xml,xmlreader,xmlwriter,Zend OPcache,zend_test,zlib

I measured the memory usage and interned strings usage when running the following script:

readfile("/proc/self/status");
var_dump(opcache_get_status()["interned_strings_usage"]);
Metric Before After Change
VmPeak 514684 kB 515032 kB +348 kB (+0.07%)
VmSize 514684 kB 515032 kB +348 kB (+0.07%)
VmHWM 26076 kB 26284 kB +208 kB (+0.80%)
VmRSS 26076 kB 26284 kB +208 kB (+0.80%)
VmData 7584 kB 7936 kB +352 kB (+4.64%)
VmStk 132 kB 132 kB +0 kB (+0.00%)
VmExe 5772 kB 5768 kB -4 kB (-0.07%)
VmLib 15720 kB 15720 kB +0 kB (+0.00%)
VmPTE 168 kB 172 kB +4 kB (+2.38%)

Opcache interned strings:

Metric Before After Change
buffer_size 8192 kB 8192 kB +0 kB (+0.00%)
used_memory 2403.03 kB 2428.09 kB +25 kB (+1.04%)
free_memory 5788.97 kB 5763.91 kB -25 kB (-0.43%)
number_of_strings 7693 8286 +593 (+7.71%)

The relative increase of VmData is not negligible, but the absolute difference (352 KiB) is not really high when considering this is likely to be shared between processes. And this is a "fixed" overhead: This does not grow proportionally to the program size or its own memory usage.

Given there is zero time or instruction overhead in the symfony benchmark, this seems acceptable to me.

if (ZEND_TYPE_HAS_LITERAL_NAME(*type)) {
// gen_stubs.php does not support codegen for DNF types in arg infos.
// As a temporary workaround, we split the type name on `|` characters,
// converting it to an union type if necessary.
Copy link
Member

@iluuu1994 iluuu1994 Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. It's referring to DNF but is handling only union types.

Edit: I see this comes from a moved code block. Would still be nice to clarify. /cc @Girgias

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the unification, and it looks reasonable to me. Should probably be delayed for 8.6.

arnaud-lb reacted with thumbs up emoji
Copy link

AWS x86_64 (c7i.24xl)

Attribute Value
Environment aws
Runner host
Instance type c7i.metal-24xl (dedicated)
Architecture x86_64
CPU 48 cores
CPU settings disabled deeper C-states, disabled turbo boost, disabled hyper-threading
RAM 188 GB
Kernel 6.1.147-172.266.amzn2023.x86_64
OS Amazon Linux 2023年8月20日250818
GCC 14.2.1
Time 2025年09月24日 08:25:16 UTC

Laravel 12.2.0 demo app - 100 consecutive runs, 50 warmups, 100 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skew P-value Instr count Memory
PHP - baseline@0d4f 0.46883 0.47452 0.00065 0.14% 0.46986 0.00% 0.46975 0.00% 3.961 0.999 176189851 44.31 MB
PHP - zstr-arg-info 0.45962 0.46843 0.00112 0.24% 0.46688 -0.63% 0.46696 -0.59% -5.211 0.000 176178123 44.35 MB

Symfony 2.7.0 demo app - 100 consecutive runs, 50 warmups, 100 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skew P-value Instr count Memory
PHP - baseline@0d4f 0.72283 0.74757 0.00234 0.32% 0.73446 0.00% 0.73400 0.00% 1.493 0.999 287326394 40.57 MB
PHP - zstr-arg-info 0.73016 0.73881 0.00154 0.21% 0.73213 -0.32% 0.73181 -0.30% 1.903 0.000 287334101 40.66 MB

Wordpress 6.2 main page - 100 consecutive runs, 20 warmups, 20 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skew P-value Instr count Memory
PHP - baseline@0d4f 0.57929 0.58250 0.00070 0.12% 0.58067 0.00% 0.58047 0.00% 0.609 0.999 1120214656 43.92 MB
PHP - zstr-arg-info 0.57805 0.58103 0.00057 0.10% 0.57930 -0.23% 0.57921 -0.22% 0.649 0.000 1120070048 43.98 MB

bench.php - 100 consecutive runs, 10 warmups, 2 requests (sec)

PHP Min Max Std dev Rel std dev % Mean Mean diff % Median Median diff % Skew P-value Instr count Memory
PHP - baseline@0d4f 0.42860 0.53828 0.02537 5.78% 0.43899 0.00% 0.43170 0.00% 3.414 0.999 2020595036 26.96 MB
PHP - zstr-arg-info 0.42682 0.59648 0.03064 7.00% 0.43790 -0.25% 0.43008 -0.37% 4.034 0.000 2020595059 27.02 MB 

Using zend_convert_internal_arg_info() at runtime breaks ZEND_RC_MOD_CHECK()
assertions. Add a "persistent" param to make it possible to convert an internal
arg info to a non-persistent arg info.
Non-persistent arg infos allocated by pdo_hash_methods() break
zend_get_arg_offset_by_name() again.
Fix zend_get_arg_offset_by_name() by excluding ZEND_ACC_NEVER_CACHE instead of
ZEND_ACC_USER_ARG_INFO. Also flag Closure::__invoke() with ZEND_ACC_NEVER_CACHE
(It was already flagged with ZEND_ACC_CALL_VIA_HANDLER, which is synonymous of
ZEND_ACC_NEVER_CACHE WRT caching).
This would allow to remove ZEND_ACC_USER_ARG_INFO later.
Copy link
Member Author

Updated the branch to fix two things:

  • bb512d0: Calling zend_convert_internal_arg() at runtime would break ZEND_RC_MOD_CHECK(), as it allocated persistent strings. This would break pdo_hash_methods().
  • 9ccfb2f: Non-persistent arg infos break zend_get_arg_offset_by_name() again. Updating zend_get_arg_offset_by_name() to look at ZEND_ACC_NEVER_CACHE instead of ZEND_ACC_USER_ARG_INFO fixed that.

Copy link
Member

@SakiTakamachi SakiTakamachi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[PDO part] LGTM :)

arnaud-lb reacted with thumbs up emoji
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimWolla TimWolla TimWolla left review comments

@dstogov dstogov dstogov left review comments

@Girgias Girgias Girgias left review comments

@DanielEScherzer DanielEScherzer DanielEScherzer left review comments

@iluuu1994 iluuu1994 iluuu1994 approved these changes

@SakiTakamachi SakiTakamachi SakiTakamachi approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /