Use the same zend_arg_info struct for internal and user functions by arnaud-lb · Pull Request #19022 · php/php-src

arnaud-lb · 2025-07-03T14:06:43Z

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.

Girgias

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. :)

Girgias · 2025-07-03T14:27:13Z

@@ -790,17 +785,15 @@ static void _parameter_string(smart_str *str, zend_function *fptr, struct _zend_
 	if (ZEND_ARG_IS_VARIADIC(arg_info)) {
 smart_str_appends(str, "...");
 	}
-	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));


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

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

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

Girgias · 2025-07-03T14:28:50Z

 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 : "?");


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

ndossche · 2025-07-03T15:54:16Z

You can probably get rid of ZEND_ACC_USER_ARG_INFO

arnaud-lb · 2025-07-03T16:16:00Z

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.

Girgias · 2025-07-03T16:39:24Z

 #endif
-	return get_default_via_ast(default_value_zval, default_value);
+	return get_default_via_ast(default_value_zval, ZSTR_VAL(default_value));


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 :)

ndossche · 2025-07-03T16:43:21Z

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.

DanielEScherzer · 2025-07-03T18:46:27Z

@@ -790,17 +785,15 @@ static void _parameter_string(smart_str *str, zend_function *fptr, struct _zend_
 	if (ZEND_ARG_IS_VARIADIC(arg_info)) {
 smart_str_appends(str, "...");
 	}
-	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));

 	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


Is this comment still correct?

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

arnaud-lb · 2025-07-04T14:00:00Z

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

dstogov

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 · 2025-07-07T08:57:04Z

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.

dstogov · 2025-07-07T09:05:39Z

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 · 2025-07-30T10:04:15Z

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.

iluuu1994

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

github-actions · 2025-09-24T09:15:05Z

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.20250818
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

arnaud-lb · 2025-09-24T15:12:55Z

Updated the branch to fix two things:

bb512d08106a1d84c5da7aba0ba7946a745015eb: Calling zend_convert_internal_arg() at runtime would break ZEND_RC_MOD_CHECK(), as it allocated persistent strings. This would break pdo_hash_methods().
9ccfb2fcd2335edc7875ee10f5e5fc08ab7297fe: 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.

SakiTakamachi

[PDO part] LGTM :)

The arg_info member of zend_function is now always a zend_arg_info*. Before, it was a zend_internal_arg_info* on internal functions, unless the ZEND_ACC_USER_ARG_INFO flag was set. ClosesphpGH-19022

* Fix zend_call_trampoline_arginfo arg name Name is "arguments" in documentation: https://www.php.net/__call#language.oop5.overloading.methods * Use zend_call_trampoline_arginfo in zend_get_call_trampoline_func() * Copy the original arg_info in zend_closure_from_frame None of these changes are observable, but this is cleaner, and this becomes observable in GH-20848. ClosesGH-20951

github-actionsBot added Category: Engine Extension: opcache Extension: pdo (core) Extension: reflection SAPI: phpdbg ABI break labels Jul 3, 2025

Girgias reviewed Jul 3, 2025
View reviewed changes

TimWolla reviewed Jul 3, 2025
View reviewed changes

Comment threadZend/zend_API.c Outdated

arnaud-lb marked this pull request as ready for review July 3, 2025 16:16

arnaud-lb requested review from DanielEScherzer, SakiTakamachi and dstogov as code owners July 3, 2025 16:16

Girgias reviewed Jul 3, 2025
View reviewed changes

DanielEScherzer reviewed Jul 3, 2025
View reviewed changes

dstogov reviewed Jul 7, 2025
View reviewed changes

iluuu1994 approved these changes Aug 26, 2025
View reviewed changes

Comment threadZend/zend_API.h Outdated
Comment threadZend/zend_API.c Outdated
Comment threadZend/zend_API.c Outdated
Comment threadZend/zend_API.c Outdated

iluuu1994 approved these changes Aug 26, 2025
View reviewed changes

arnaud-lb force-pushed the zstr-arg-info branch from 753d82c to 95237d5Compare September 24, 2025 08:23

arnaud-lb force-pushed the zstr-arg-info branch from 90575d2 to 9e6a5bfCompare September 24, 2025 15:07

github-actionsBot added the Extension: zend_test label Sep 24, 2025

arnaud-lb force-pushed the zstr-arg-info branch from 9e6a5bf to 9ccfb2fCompare September 24, 2025 15:10

SakiTakamachi approved these changes Sep 25, 2025
View reviewed changes

arnaud-lb force-pushed the zstr-arg-info branch from 9ccfb2f to 7f2d73aCompare December 15, 2025 11:22

Unify arg info representation for internal and user functions
cb480de
The arg_info member of zend_function is now always a zend_arg_info*. Before, it was a zend_internal_arg_info* on internal functions, unless the ZEND_ACC_USER_ARG_INFO flag was set. ClosesphpGH-19022

arnaud-lb force-pushed the zstr-arg-info branch from 7f2d73a to cb480deCompare December 15, 2025 12:00

arnaud-lb requested a review from kocsismate as a code owner December 15, 2025 12:00

arnaud-lb closed this in 626f3c3Dec 15, 2025

This was referenced Jan 16, 2026
Followup GH-19022 #20951
Merged
[PFA 2/n] Support PFA functionality #20848
Merged

Uh oh!

Conversation

arnaud-lb commented Jul 3, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GirgiasJul 3, 2025

Choose a reason for hiding this comment

Uh oh!

arnaud-lbJul 3, 2025

Choose a reason for hiding this comment

Uh oh!

GirgiasJul 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GirgiasJul 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ndossche commented Jul 3, 2025

Uh oh!

arnaud-lb commented Jul 3, 2025

Uh oh!

GirgiasJul 3, 2025

Choose a reason for hiding this comment

Uh oh!

ndossche commented Jul 3, 2025

Uh oh!

DanielEScherzerJul 3, 2025

Choose a reason for hiding this comment

Uh oh!

arnaud-lbJul 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arnaud-lb commented Jul 4, 2025

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

arnaud-lb commented Jul 7, 2025

Uh oh!

dstogov commented Jul 7, 2025

Uh oh!

arnaud-lb commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBot commented Sep 24, 2025

AWS x86_64 (c7i.24xl)

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

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

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

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

Uh oh!

arnaud-lb commented Sep 24, 2025

Uh oh!

SakiTakamachi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

arnaud-lb commented Jul 3, 2025•
edited
Loading