Add consumed_args callback optimization and apply to array_reduce() for carry to improve performance

[drealecs]

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]

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

[drealecs]

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.

[drealecs]

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.

[drealecs]

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:

1. engine-only consumed_args support in zend_call_function() + tests.
2. 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!

[iluuu1994]

LGTM otherwise

[iluuu1994]

Assuming tests pass. Thanks for the changes @drealecs!

[TimWolla]

I'm not seeing any more obvious issues, but I didn't check this in detail beyond "reading the diff".

[Girgias]

I'm not sure this is very reliable :/ Maybe the better option is to create a new "debug" function in zend_test that takes part of the debug_zval_dump() code and just returns an integer for the refcount number.

if (Z_REFCOUNTED_P(struc)) {
	return Z_REFCOUNT_P(struc);
} else {
	/* interned */
	return -1:
}

But I totally get that you're fed up with the wild goose chase, so also happy to just drop the test. :)

[drealecs]

I pushed now the last commit with testing ob_start using a new refcounting test function, zend_test_refcount().

[drealecs]

But I totally get that you're fed up with the wild goose chase, so also happy to just drop the test. :)

😄 No worries, I considered from the start that it might take some time.

Also, since things are (almost) ready, and as you were the one mentioning it, to split the PR: I still kept it as it is, the PR is made of two commits, taking care to keep them well composed and focused on what each represents.
So, just to be sure... does this sound good? And also wondering if the merge can be done without using squash, so they are preserved?