VAR|TMP overhaul

[iluuu1994]

The aim of this PR is twofold:

• Reduce the number of highly similar TMP|VAR handlers
• Avoid ZVAL_DEREF in most of these cases

This is achieved by guaranteeing that all zend_compile_expr() calls, as well as all other compile calls with BP_VAR_R, will result in a TMP variable. This implies that the result will not contain an IS_INDIRECT or IS_REFERENCE value, which was mostly already the case, with two exceptions:

• Calls to return-by-reference functions. Because return-by-reference functions are quite rare, this is solved by delegating the DEREF to the RETURN_BY_REF handler, which will examine the stack to check whether the caller expects a VAR or TMP to understand whether the DEREF is needed.

• By-reference assignments, including both $a = &$b, as well as [&$a] = $b. When the result of these expressions is used in a BP_VAR_R context, it will be passed to a new ZEND_DEREF opcode beforehand. This is exceptionally rare.

Preliminary testing shows a 1.1% wall time improvement in Symfony Demo and roughly 0.5% in Wordpress. Edit: Sadly I can now only measure a 0.15% improvement for Symfony, but 0.8% for Wordpress. Zend/bench.php improves by ~3% in my tests. There seems to be quite a bit of volatility involved, potentially in relation to binary layout. Regardless, I think this is unlikely to cause true slowdowns for code that doesn't use return-by-ref.

TODOs:

• Verify this doesn't break important optimizations
• Use zend_unwrap_reference(). I missed this function, I was looking for a macro.
• Check why JIT i-count regresses. Edit: Symfony Demo now reduces jitted code by 0.35%, Wordpress by 0.02%.
• Try to replace DEREF with QM_ASSIGN.
• Add checks to R/IS compile-paths, asserting no VARs are generated.

[iluuu1994]

YIELD_FROM from generators can throw if the generator is closed, even for empty arrays. So this optimization may be unsound.

[bwoebi]

I would not mind doing the inverse - changing YIELD_FROM to not throw on empty array, though...

[dstogov]

Taking into account the "closed" generator, this is wrong.

[dstogov]

I think this should work, but the effect is not great.
(I see 30KB reduction in PHP code size and very slight performance difference).

The path breaks Symfony demo with function JIT (probably because of missing changes in FETCH_(DIM|OBJ)_FUNC_ARG handlers). This needs to be fixed of course.

[dstogov]

Replacing QM_ASSIGN with FREE opens possibility to deeper application of the same optimization.

[dstogov]

Taking into account the "closed" generator, this is wrong.

[dstogov]

I think the patch makes sense, but still has problems.
The executor changes looks fine (see minor comments).
I'm not able to analyse zend_compile.c changes.
I see introduced memory leaks related to NEW on Symfony community tests.

[iluuu1994]

I see introduced memory leaks related to NEW on Symfony community tests.

Thank you, I missed those. I'll have a look.

[github-actions]

AWS x86_64 (c7i.24xl)

Attribute	Value
Environment	aws
Instance type	c7i.metal-24xl (dedicated)
Architecture	x86_64
CPU	Intel(R) Xeon(R) Platinum 8488C, 48 cores @ 2400 MHz
CPU settings	disabled deeper C-states, disabled turbo boost, disabled hyper-threading
RAM	188 GB
Kernel	6.1.158-178.288.amzn2023.x86_64
OS	Amazon Linux 2023.9.20251117
GCC	14.2.1
Time	2026-01-28 17:38:56 UTC
Job details	https://github.com/php/php-src/actions/runs/21448985323 (Artifacts)
Changeset	https://github.com/php/php-src/compare/62c94b6926..39de75b32a

Laravel 12.11.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 %	Skewness	Z-stat	P-value	Memory
PHP - baseline@62c94b6	0.44725	0.45399	0.00090	0.20%	0.44788	0.00%	0.44771	0.00%	6.022	0.000	1.000	27.00 MB
PHP - vm-early-deref	0.44585	0.44924	0.00040	0.09%	0.44650	-0.31%	0.44646	-0.28%	3.506	11.902	0.000	27.01 MB

Symfony 2.8.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 %	Skewness	Z-stat	P-value	Memory
PHP - baseline@62c94b6	0.76487	0.77847	0.00313	0.40%	0.77414	0.00%	0.77533	0.00%	-1.695	0.000	1.000	27.01 MB
PHP - vm-early-deref	0.76329	0.77703	0.00299	0.39%	0.77259	-0.20%	0.77369	-0.21%	-1.762	6.739	0.000	27.01 MB

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

PHP	Min	Max	Std dev	Rel std dev %	Mean	Mean diff %	Median	Median diff %	Skewness	Z-stat	P-value	Memory
PHP - baseline@62c94b6	0.66303	0.66616	0.00043	0.06%	0.66415	0.00%	0.66411	0.00%	0.877	0.000	1.000	27.01 MB
PHP - vm-early-deref	0.65959	0.68008	0.00201	0.30%	0.66069	-0.52%	0.66048	-0.55%	9.251	11.971	0.000	27.01 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 %	Skewness	Z-stat	P-value	Memory
PHP - baseline@62c94b6	0.42315	0.44177	0.00205	0.48%	0.42656	0.00%	0.42640	0.00%	4.242	0.000	1.000	27.01 MB
PHP - vm-early-deref	0.42300	0.44239	0.00230	0.54%	0.42637	-0.04%	0.42618	-0.05%	3.405	0.705	0.481	27.01 MB

[mbeccati]

@iluuu1994 I'm afraid there are issues with this commit. In my CI both Symfony and Revive Adserver cannot run the test suite.

Symfony 7.4 doesn't even start:

# ../php-src/sapi/cli/php ./phpunit
php: /home/matteo/OSS/php-src/Zend/zend_compile.c:12306: zend_compile_var_inner: Assertion `ast->child[0]->kind == ZEND_AST_ARRAY && zend_propagate_list_refs(ast->child[0])' failed.
[1] 69836 IOT instruction (core dumped) ../php-src/sapi/cli/php ./phpunit

In order to replicate just clone symfony 7.4, run composer install, then run ./phpunit with a freshly compiled php from master.

In the Revive Adserver suite, I'm getting:

Fatal error: Uncaught Error: SimpleErrorTrappingInvoker::__construct(): Argument #1 ($invoker) could not be passed by reference in /home/runner/work/php-latest-builds/php-latest-builds/lib/simpletest/test_case.php:109

In this case it might just be some deprecation being promoted to error perhaps. Code is very much legacy, so it might just be something that needs to be adjusted on our side. I will report back my findings on this.

[iluuu1994]

@mbeccati Thanks for letting me know. I'll have a look today. I did test this change with nightly, which runs all Symfony tests (along with many others).

[mbeccati]

@iluuu1994 Thanks! I can confirm that the Revive Test Suite fails to run because of:

PHP Notice: Only variables should be passed by reference in /home/matteo/OSS/php-src/r.php on line 8

vs:

Fatal error: Uncaught Error: A::__construct(): Argument #1 ($o) could not be passed by reference in /home/matteo/OSS/php-src/r.php:8

which I'm totally OK with, but will probably need to be listed in UPGRADING (or RFC?).

Simple enough to replicate:

<?php
class A {
public function __construct(&$o) {
}
}
var_dump(new A(new \StdClass()));

[iluuu1994]

@mbeccati Changing behavior was not the intention. I'll make sure this keeps working. The first case might just be a bad assertion, we'll see.

[iluuu1994]

@mbeccati The first issue should be solved. It was just a faulty assertion ("surely adding this code after testing is fine, right?"). The other will take more consideration. Sadly, PHP has inconsistent error types when passing non-vars as references: https://3v4l.org/lE72G Maybe it wouldn't be such a bad idea to unify this case after all, but that would need an RFC. I'll see how easy it is to find a resolution tonight, I'm out of time for now.

[iluuu1994]

@mbeccati After thinking about this some more, and discussing with @bwoebi, I think the second case can be considered a fix. As mentioned, PHP already errors when things that can never be references are passed to by-ref parameters. For example:

https://3v4l.org/Irv4C

namespaceFoo;
functiontakesRef(&$ref) {}
takesRef(count([])); // This will warntakesRef(\count([])); // This will throw

The first may be a locally-shadowing count function that returns by-ref, which makes PHP only emit a notice. The second is a known call to the global count function, so PHP will throw an exception. I won't try to argue for why that would be sensible behavior (it's not). I'd support the consolidation of behavior (either promoting or relaxing both errors, I don't particularly care which way around). In either case, special-handling new here probably doesn't make sense. I'll make sure to add a note in the UPRADING file.

[mbeccati]

@iluuu1994 I agree about the behaviour, but I'm not convinced 100% we can get away with it without a proper RFC.

[bwoebi]

@mbeccati What would need a RFC would be reversing the behaviour. This is a pure bugfix.
Allowing new or any other arbitrary expression to be passed to args expecting by ref.
I would welcome that change though.

[mbeccati]

My point is that the bug has existed long enough that the bugfix is effectively a BC-break. Either way I'm fine with it, as I've been busy fixing the legacy code that was not working with master anymore.

[bwoebi]

@mbeccati Yes, that happens. I've also fixed a handful of long-standing bugs recently - in the end it's still considered a bugfix, but only applied to the next minor version (rather than patch version).