Skip to content

Arginfo: avoid using temporary zvals for initializing attribute values #19141

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Arginfo: avoid using temporary zvals for initializing attribute values #19141

wants to merge 1 commit into from
+154 −419

Conversation

DanielEScherzer
Copy link
Member

Instead of

  • adding a zval on the stack
  • initializing it
  • copying the value to the attribute

Just initialize the value directly in the zend_attribute_arg

@DanielEScherzer
Copy link
Member Author

This is the second part of my follow-up to #18780 (after #19075) 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)

@DanielEScherzer
Arginfo: avoid using temporary zvals for initializing attribute values
bc43982 
Instead of
* adding a zval on the stack
* initializing it
* copying the value to the attribute

Just initialize the value directly in the zend_attribute_arg
@TimWolla TimWolla removed their request for review July 15, 2025 21:24
@DanielEScherzer
Copy link
Member Author

Comparing the benchmarking results with those from the most recent prior commit that ran CI (a22dc67) (differences in bold)

Test Master 19141/merge
Zend/bench.php 2233862171 2233857298
Zend/bench.php JIT 601830715 601828645
Symfony Demo 2.2.3 39047924 39048781
Symfony Demo 2.2.3 JIT 33662329 33530937
Wordpress 6.2 122425904 122418421
Wordpress 6.2 JIT 93335578 93327850

So everything but Symfony Demo 2.2.3 improved. @nielsdos would you mind checking if this matches for you with php-cgi?

@nielsdos
Copy link
Member

So everything but Symfony Demo 2.2.3 improved. @nielsdos would you mind checking if this matches for you with php-cgi?

The difference you're observing can be attributed to noise. Anything around 0.01% difference is too small to say anything useful about. Intuitively, this patch shouldn't make things worse.

@DanielEScherzer
Copy link
Member Author

Okay, in that case I plan to merge this in a few days if there are no objections (CC @kocsismate)

nielsdos
nielsdos approved these changes Jul 19, 2025
View reviewed changes
build/gen_stub.php
$forStringDef = "{$zvalName}_str";
}
$code .= "\tzend_string *$forStringDef = zend_string_init($cExpr, strlen($cExpr), 1);\n";
$code .= "\tZVAL_STR(&$zvalName, $forStringDef);\n";
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this can even be ZVAL_NEW_STR as they're not interned.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, all strings that are coming from KNOWN_STRINGS can be set via ZVAL_INTERNED_STR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

@bukka bukka Awaiting requested review from bukka bukka is a code owner

@Girgias Girgias Awaiting requested review from Girgias Girgias is a code owner

@zeriyoshi zeriyoshi Awaiting requested review from zeriyoshi zeriyoshi is a code owner

@devnexen devnexen Awaiting requested review from devnexen devnexen is a code owner

@kamil-tekiela kamil-tekiela Awaiting requested review from kamil-tekiela kamil-tekiela is a code owner

@derickr derickr Awaiting requested review from derickr derickr is a code owner

@adoy adoy Awaiting requested review from adoy adoy is a code owner

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

Assignees
No one assigned
Labels
ABI break Category: Build System Category: Engine Extension: curl Extension: date Extension: dom Extension: enchant Extension: filter Extension: intl Extension: libxml Extension: mysqli Extension: openssl Extension: pgsql Extension: random Extension: reflection Extension: shmop Extension: soap Extension: spl Extension: standard Extension: xml Extension: zend_test Extension: zip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DanielEScherzer @nielsdos