Add consumed_args callback optimization and apply to array_reduce() for carry to improve performance by drealecs · Pull Request #21340 · php/php-src

drealecs · 2026-03-04T14:09:06Z

Related to #8283

For some callback operations, it is safe to consume/reuse a value passed as a parameter instead of copying it first.
This PR adds a generic internal mechanism for that (through zend_fcall_info.consumed_args) and applies it to array_reduce() carry argument, and a few other callback-using functions.

This reduces copy-on-write churn, having the parameter value the only reference, and significantly improves array_reduce() performance for mutable accumulator patterns, while keeping userland semantics unchanged.

The same mechanism can be used in other callback-using functions, and it is also applied here to preg_replace_callback() / preg_replace_callback_array() and ob_start().

TimWolla · 2026-03-04T20:26:56Z

Can you provide the benchmark you used to verify this improves the situation?

drealecs · 2026-03-05T08:40:54Z

Can you provide the benchmark you used to verify this improves the situation?

Yes, this is a good point.
Here is a simple test I used locally:

<?php

declare(strict_types=1);

function test($size) {
    $data = [];
    for ($i = 0; $i < $size; $i++) {
        $data[] = substr(hash('crc32b', (string) $i), 0, 4);
    }

    $start = hrtime(true);
    $result = array_reduce(
        $data,
        static function ($carry, $item) {
            @$carry[$item]++;
            return $carry;
        },
        [],
    );
    $duration = hrtime(true) - $start;

    printf("size=%d, unique=%d, duration=%.3f ms\n", $size, count($result), $duration/1_000_000);
}

test(10_000);
test(20_000);
test(40_000);

On master, this is the output:

$ ./sapi/cli/php test_array_reduce.php
size=10000, unique=9980, duration=1199.907 ms
size=20000, unique=19084, duration=4844.224 ms
size=40000, unique=34066, duration=19404.972 ms

and we can see that the complexity is O(n^2).

On this branch, this is the output:

$ ./sapi/cli/php test_array_reduce.php
size=10000, unique=9980, duration=9.108 ms
size=20000, unique=19084, duration=18.905 ms
size=40000, unique=34066, duration=35.392 ms

with O(n) complexity.

iluuu1994

Nice proposal! The approach makes sense to me. Applying this to all cases will require some care. This is safe for any argument that is owned. So this is fine for $initial (and the return value of $callback in subsequent iterations), but wouldn't be for the elements $array (ZEND_HASH_FOREACH_VAL(htbl, operand)) given operand has not been ADDREFd at this point.

Before merging this, it may make sense to scout the code base to see which functions this may be applied to. Some of the use cases might influence the design.

iluuu1994 · 2026-03-06T12:17:42Z

Zend/zend_API.h

 	 * integrate APIs like call_user_func_array(). The usual restriction that
 	 * there may not be position arguments after named arguments applies. */
 	HashTable *named_params;
+	uint32_t consume_params;


Nit: consumed would indicate this property can diverge between args. args would also be more correct, though I realize you named this after the other fields.

Yes, I got to the same conclusion, that args might be a better name here, but I like consistency more, so I went with params.
Also, instead of consume I was thinking some other words might fit better: move, steal, transfer.

For now I'll stick with consume_params, if nothing comes up as a better alternative.

My comment wasn't fully clear. I'd suggest consumed_args, which indicates better that this is map of args to be consumed. consume_args sounds like a bool to consume all args.

Yeah agreed, the current name is confusing.

Also this should be moved before the named_params field as there is already a 4 bytes gap in the struct on 64bits

Thank you for the review! Great point, I moved it after param_count.
And also renamed it to consumed_args.

drealecs · 2026-03-06T15:42:26Z

Before merging this, it may make sense to scout the code base to see which functions this may be applied to. Some of the use cases might influence the design.

I already did some analysis, and there are a few other places where we could implement it to get some improvements:

preg_replace_callback() / preg_replace_callback_array(): for $matches, valuable if callback would modify it.
ob_start(): for $buffer, valuable if callback would modify it.
there are other cases, but it's not usual to modify the params in the callbacks, like $key for array_find().

What do you think? And related to the design, what can we improve?

Girgias

I'd rather have this split into two PRs. One that adds the consumed_args with various tests added to the zend_test extension (e.g. ref variables as that seems to be explicitly checked and I'd like to understand more) and then a follow-up PR to add support for array_reduce() and whatever other functions.

Girgias · 2026-03-06T23:09:23Z

Zend/zend_API.h

 	 * integrate APIs like call_user_func_array(). The usual restriction that
 	 * there may not be position arguments after named arguments applies. */
 	HashTable *named_params;
+	uint32_t consume_params;


Yeah agreed, the current name is confusing.

Girgias · 2026-03-06T23:13:15Z

Zend/zend_API.h

 	 * integrate APIs like call_user_func_array(). The usual restriction that
 	 * there may not be position arguments after named arguments applies. */
 	HashTable *named_params;
+	uint32_t consume_params;


Also this should be moved before the named_params field as there is already a 4 bytes gap in the struct on 64bits

drealecs · 2026-03-07T12:30:47Z

I'd rather have this split into two PRs. One that adds the consumed_args with various tests added to the zend_test extension (e.g. ref variables as that seems to be explicitly checked and I'd like to understand more) and then a follow-up PR to add support for array_reduce() and whatever other functions.

Thank you for the review, Gina. Great point about the tests.
I implemented them using the zend_test extension, including the reference-sensitive paths. Please let me know if this covers the cases you had in mind.

I’d prefer to keep this as a single PR if that works for you. I did create exactly two commits from the start, and I plan to keep them that way, so the separation is explicit and easier to review. They contain now:

engine-only consumed_args support in zend_call_function() + tests.
adoption in array_reduce() + some other callback-using functions.

If you still prefer two separate PRs, I can split them, but I hoped this structure would keep review manageable while preserving context. And if merging can be done not using squash, it will keep git history relevant, but I don't know if that's something commonly used, so let me know if I should change it or if it works for you this way. Thanks again!

drealecs · 2026-03-10T18:55:03Z

Care should be taken when more than one arg is marked as consumed, since if the same value is passed to those, refcount will be off. Is there a use-case for marking more than one arg as consumed?

Thanks @arnaud-lb for the review and for flagging this.
My understanding is that this would require an already inconsistent refcount setup when zend_call_function() receives params (e.g. multiple arg slots pointing to the same refcounted payload without matching ownership/refcount accounting).
Since consumed_args is internal-only, this doesn’t look directly userland-triggerable. We can treat it as an internal caller contract issue: if multiple args are marked consumed, the caller should ensure parameter ownership/refcount setup is valid.

But I may be missing something in the broader call paths, so happy to adjust if you still think we should add hardening here.
Maybe you can explain it a bit more, on what case you were thinking of.

drealecs · 2026-03-11T12:29:30Z

I'm wondering if this could be implemented as an opcode optimization as well. Then we could apply #20934 to array_reduce().

Yes, that's a nice idea as well. I think this might be possible, but I don't have too much experience.
However, I think it would be more complex than this. And a bit more risky.

arnaud-lb · 2026-03-11T14:07:10Z

My understanding is that this would require an already inconsistent refcount setup when zend_call_function() receives params (e.g. multiple arg slots pointing to the same refcounted payload without matching ownership/refcount accounting).

Not necessarily, as argument passing increases the refcount.

So this is valid:

ZVAL_STR(&args[0], ZSTR_INIT_LITERAL("test", 0));
ZVAL_COPY_VALUE(&args[1], &args[0]);

fci.retval = return_value;
fci.param_count = 2;
fci.params = args;

zend_call_function(&fci, &fci_cache);
zval_ptr_dtor(&args[0]);

But not this:

ZVAL_STR(&args[0], ZSTR_INIT_LITERAL("test", 0));
ZVAL_COPY_VALUE(&args[1], &args[0]);

fci.retval = return_value;
fci.param_count = 2;
fci.params = args;
fci.consumed_args = ZEND_FCI_CONSUMED_ARG(0) | ZEND_FCI_CONSUMED_ARG(1);

zend_call_function(&fci, &fci_cache);

As a result, using consumed_args with more than one argument requires special care. There may be cases in which it could more hidden than this.

drealecs · 2026-03-11T16:18:10Z

As a result, using consumed_args with more than one argument requires special care. There may be cases in which it could more hidden than this.

Thanks for the example.

Without consumed_args, aliasing the same value across arg slots can still be valid.
With multi-arg consumed_args, that same aliasing pattern can become unsafe unless ownership/refcount is accounted for accordingly.
So my understanding is: even though this is internal API usage and callers opting into consumed_args should handle refcounting correctly, we should still harden this to avoid memory-unsafe footguns. Is that correct?

I’m good with either approach:

keep multi-bit masks, but make consume alias-safe somehow (consume first alias, ZVAL_COPY duplicates)
restrict to a single consumed arg for now, either bit-mask or not.
keep it unsafe, or use some simple detection of it and bail out

In current use cases, I only found single-argument optimization cases. But also limiting this makes it less "complete".
Maybe also using a single arg slot for now, and allowing more in the future if needed. And still keeping a bit-mask but allow only 1-bit set.
I'm thinking that detecting duplicate aliases might incur some overhead.

What do you prefer? I'm good either way.

arnaud-lb · 2026-03-11T16:41:34Z

My preference would be to restrict to a single arg for now, especially if there are no use-cases for multiple ones in the code base. Approach 1 would be ok too, but I suspect that duplicate detection would incur a performance penalty. Either way I have no strong objection.

iluuu1994 · 2026-03-11T16:52:14Z

I'd say let's restrict this to a single arg then until we have more use-cases. It's easy enough to extend otherwise.

TimWolla · 2026-03-11T17:54:26Z

Then we could apply #20934 to array_reduce().

I had planned to apply a desugaring for the other array_*() functions once PFA has landed. The array_map() was intended as a proof of concept.

TimWolla · 2026-03-11T18:01:21Z

Zend/zend_execute_API.c

+			if (EXPECTED(fci->consumed_args == 0)) {
+				ZVAL_COPY(param, arg);
+			} else {
+				if (ZEND_FCI_IS_CONSUMED_ARG(fci->consumed_args, i) && !Z_ISREF_P(arg) && arg == &fci->params[i]) {
+					ZVAL_COPY_VALUE(param, arg);
+					ZVAL_UNDEF(arg);
+				} else {
+					ZVAL_COPY(param, arg);
+				}
+			}


Is having the extra branch valuable, or can this just be:

Suggested change

if (EXPECTED(fci->consumed_args == 0)) {

ZVAL_COPY(param, arg);

} else {

if (ZEND_FCI_IS_CONSUMED_ARG(fci->consumed_args, i) && !Z_ISREF_P(arg) && arg == &fci->params[i]) {

ZVAL_COPY_VALUE(param, arg);

ZVAL_UNDEF(arg);

} else {

ZVAL_COPY(param, arg);

}

}

if (EXPECTED(!(ZEND_FCI_IS_CONSUMED_ARG(fci->consumed_args, i) && Z_ISREF_P(arg) && arg == &fci->params[i]))) {

ZVAL_COPY(param, arg);

} else {

ZVAL_COPY_VALUE(param, arg);

ZVAL_UNDEF(arg);

}

Using the internal negation to avoid an UNEXPECTED here.

I changed it, and I think it should be good without the extra branch.
But I kept using UNEXPECTED, as I think it's readable enough. Is there another reason to prefer EXPECTED? Let me know what you think.

Is there another reason to prefer EXPECTED? Let me know what you think.

UNEXPECTED is generally a stronger indicator than just “not EXPECTED”. See also: https://github.com/php/php-src/pull/21340/changes#r2959248306

drealecs · 2026-03-12T12:30:46Z

My preference would be to restrict to a single arg for now, especially if there are no use-cases for multiple ones in the code base.

I'd say let's restrict this to a single arg then until we have more use-cases. It's easy enough to extend otherwise.

Thanks @arnaud-lb and @iluuu1994.
Done, and kept it as a bitmask to allow easier extension in the future, without ABI breaking changes. Not sure if that makes sense, let me know.

iluuu1994

LGTM otherwise

iluuu1994 · 2026-03-18T14:40:11Z

Zend/zend_execute_API.c

 		func, fci->param_count, object_or_called_scope);
+	uint32_t consumed_args = fci->consumed_args;
+
+	if (UNEXPECTED(consumed_args & (consumed_args - 1))) {


IMO this should be an assertion. It's better to warn extensions that this is incorrect usage.

~~Also: Probably harmless, but the UNEXPECTED is currently wrong, because 0 will fail this check.~~ Nevermind, I was mistaken.

Done, I added an assertion in the same if block. Hope it looks ok.

BTW, yes, I rechecked that case for 0 a couple of times myself in two or three distinct days, not being so sure about it. 😄

Please use:

ZEND_ASSERT((consumed_args & (consumed_args - 1)) == 0);

This branch will get eliminated anyway.

I see, re-reading your message, I understand now that you mean to not try and sanitize consumed_args to 0, but just to add the assertion. I pushed it again now.

iluuu1994 · 2026-03-18T14:55:43Z

ext/zend_test/tests/consumed_args_basic.phpt

+<?php
+
+$result = zend_test_call_with_consumed_args(
+    static fn($a, $b) => [$a, $b],


It might be useful to dump the refcount with debug_zval_dump(), given that's what we're actually trying to achieve here. In this case, [1, 2, 3] is a literal. It might also be useful to make this a normal heap-allocated value.

Yes, it makes sense; I did the change and refcount is 3 in this test. And if sending consumed_args as 0, it is 4.

Right, I think you should use range(1, 3) or some other function that creates heap allocated values. The refcount should be 2 (one for debug_zval_dump()s frame, and the other for the closure), which would show the value is properly moved.

Tried it now, and I do still observe refcount(3), not 2, because in this helper path there is still one owner from the args array used to build fci.params, in addition to the closure arg and debug_zval_dump()'s temporary ref.

Simple test case:

--TEST-- Test --FILE-- <?php $result = array_reduce([1, 2, 3], function ($acc, $val) { debug_zval_dump($acc); $acc[] = $val; return $acc; }, []); debug_zval_dump($result); ?> --EXPECT-- array(0) interned { } array(1) packed refcount(2){ [0]=> int(1) } array(2) packed refcount(2){ [0]=> int(1) [1]=> int(2) } array(3) packed refcount(2){ [0]=> int(1) [1]=> int(2) [2]=> int(3) }

Great point, I added the test to the second commit, that adds consumed_args usage for array_reduce().

iluuu1994

Assuming tests pass. Thanks for the changes @drealecs!

iluuu1994 · 2026-03-19T09:56:48Z

ext/zend_test/tests/consumed_args_basic.phpt

I see this one does fail. [1, 2, 3] is immutable in shared memory with opcache. I'd probably just drop this test, as there's ext/standard/tests/array/array_reduce_accumulator_refcount.phpt now.

Yes, I noticed that, and I changed it already to range(1, 3).

Girgias · 2026-03-19T10:54:44Z

Zend/zend_execute_API.c


 		if (EXPECTED(!must_wrap)) {
-			ZVAL_COPY(param, arg);
+			if (UNEXPECTED(consumed_args != 0)


AFAIK UNEXPECTED() is heavily penalized, and it seems more common for consumed_args == 0. I'd remove the CPU hint.

I don't mind removing the hint, though:

https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fexpect

For the purposes of branch prediction optimizations, the probability that a __builtin_expect expression is true is controlled by GCC’s builtin-expect-probability parameter, which defaults to 90%.

The inverse being 10%. That seems appropriate. Though it's actually probably too high for most other of our use-cases for UNEXPECTED.

Frequently code is only reordered and not evicted to cold-sections until the code grows beyond a certain size. I haven't checked what happens in this case.

@Girgias, can you explain why UNEXPECTED() would be "heavily penalized"?
I changed it to if (EXPECTED(consumed_args == 0), as also @TimWolla mentioned it(#21340 (comment)), but my understanding is that it ends up with the same behavior. Maybe it's easier to read now?
Can you check if it looks good now?

TimWolla · 2026-03-19T11:11:39Z

Zend/zend_API.h

+#define ZEND_FCI_CONSUMED_ARG(arg_index) \
+	((arg_index) < 32 ? (1u << (arg_index)) : 0u)
+#define ZEND_FCI_IS_CONSUMED_ARG(consumed_args, arg_index) \
+	((arg_index) < 32 && ((consumed_args) & (1u << (arg_index))))


Can we avoid introducing new macros and just make these static inline functions? Particularly since this is a macro that uses parameters more than once which causes issues if the passed value is not a literal.

Yes, that makes sense to me; I'm not a big fan of that many macros. I changed it now.

But since moving exactly this one to macro was suggested by @arnaud-lb (#21340 (comment)), gently pinging him in case he has any comments.

…tions

drealecs requested review from SakiTakamachi, arnaud-lb, bukka, dstogov and ndossche as code owners March 4, 2026 14:09

github-actions bot added Category: Engine Extension: dom Extension: ffi Extension: pcre Extension: pdo (core) Extension: standard ABI break labels Mar 4, 2026

drealecs force-pushed the improve-array-reduce-consume-carry branch 2 times, most recently from 86f212d to 2f9aa58 Compare March 4, 2026 17:24

drealecs changed the title ~~Draft: Improve array_reduce() consume carry~~ Improve array_reduce() consume carry Mar 4, 2026

drealecs force-pushed the improve-array-reduce-consume-carry branch from 2f9aa58 to 3299db5 Compare March 4, 2026 18:55

drealecs force-pushed the improve-array-reduce-consume-carry branch from 3299db5 to cdbe42a Compare March 5, 2026 11:53

iluuu1994 reviewed Mar 6, 2026

View reviewed changes

iluuu1994 requested a review from Girgias March 6, 2026 13:04

Girgias requested changes Mar 6, 2026

View reviewed changes

drealecs force-pushed the improve-array-reduce-consume-carry branch 2 times, most recently from 008446f to fb5617f Compare March 7, 2026 11:39

drealecs requested a review from kocsismate as a code owner March 7, 2026 11:39

github-actions bot added the Extension: zend_test label Mar 7, 2026

drealecs force-pushed the improve-array-reduce-consume-carry branch from 2847abb to 9171681 Compare March 7, 2026 12:43

drealecs requested a review from Girgias March 7, 2026 12:44

TimWolla reviewed Mar 11, 2026

View reviewed changes

drealecs force-pushed the improve-array-reduce-consume-carry branch 2 times, most recently from b08af29 to 02de4c1 Compare March 12, 2026 12:21

drealecs force-pushed the improve-array-reduce-consume-carry branch 2 times, most recently from 83a5382 to 9bb79ff Compare March 13, 2026 12:22

drealecs requested review from TimWolla, arnaud-lb and iluuu1994 March 16, 2026 14:55

iluuu1994 reviewed Mar 18, 2026

View reviewed changes

drealecs force-pushed the improve-array-reduce-consume-carry branch 4 times, most recently from ba43bb5 to 23853ee Compare March 19, 2026 09:41

iluuu1994 approved these changes Mar 19, 2026

View reviewed changes

iluuu1994 reviewed Mar 19, 2026

View reviewed changes

drealecs force-pushed the improve-array-reduce-consume-carry branch from 23853ee to b54002d Compare March 19, 2026 10:11

Girgias reviewed Mar 19, 2026

View reviewed changes

TimWolla reviewed Mar 19, 2026

View reviewed changes

drealecs force-pushed the improve-array-reduce-consume-carry branch from b54002d to 7eedaf8 Compare March 19, 2026 11:44

drealecs added 2 commits March 19, 2026 14:09

Add consumed_args support for fcall arguments

3d10d28

Use consumed_args for array_reduce and some other callback-using func…

24ca52b

…tions

drealecs force-pushed the improve-array-reduce-consume-carry branch from 7eedaf8 to 24ca52b Compare March 19, 2026 12:09

Conversation

drealecs commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimWolla commented Mar 4, 2026

Uh oh!

drealecs commented Mar 5, 2026

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drealecs commented Mar 6, 2026

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drealecs commented Mar 7, 2026

Uh oh!

drealecs commented Mar 10, 2026

Uh oh!

drealecs commented Mar 11, 2026

Uh oh!

arnaud-lb commented Mar 11, 2026

Uh oh!

drealecs commented Mar 11, 2026

Uh oh!

arnaud-lb commented Mar 11, 2026

Uh oh!

iluuu1994 commented Mar 11, 2026

Uh oh!

TimWolla commented Mar 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drealecs Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drealecs commented Mar 12, 2026

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drealecs Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drealecs Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

drealecs commented Mar 4, 2026 •

edited

Loading

drealecs Mar 12, 2026 •

edited

Loading

iluuu1994 Mar 18, 2026 •

edited

Loading

drealecs Mar 18, 2026 •

edited

Loading

iluuu1994 Mar 19, 2026 •

edited

Loading

drealecs Mar 18, 2026 •

edited

Loading

drealecs Mar 19, 2026 •

edited

Loading

iluuu1994 Mar 19, 2026 •

edited

Loading

drealecs Mar 19, 2026 •

edited

Loading

TimWolla Mar 19, 2026 •

edited

Loading

drealecs Mar 19, 2026 •

edited

Loading