Skip to content

Conversation

@nielsdos
Copy link
Member

@nielsdos nielsdos commented Mar 26, 2025

The refcounting and destruction is not necessary because zend_call_function will make a copy anyway. And zend_call_function only returns FAILURE if EG(active) is false in which case array_map shouldn't have been called in the first place.

This also adds a fast path for packed arrays (even with holes). By separating the paths of packed/hashed arrays the hashed array iteration is very very slightly faster as well but not by much.

For this script:

$array = range(1, 10000);

for ($i = 0; $i < 800; $i++) {
    array_map(static function ($item) {
            return $item * 2;
    }, $array);
}

On an intel i7-1185G7:

Benchmark 1: ./sapi/cli/php map.php
  Time (mean ± σ):     127.5 ms ±   3.2 ms    [User: 124.1 ms, System: 3.2 ms]
  Range (min … max):   125.1 ms … 138.1 ms    23 runs
  
Benchmark 2: ./sapi/cli/php_old map.php
  Time (mean ± σ):     148.6 ms ±   3.3 ms    [User: 144.9 ms, System: 3.5 ms]
  Range (min … max):   145.6 ms … 160.6 ms    20 runs
  
Summary
  ./sapi/cli/php map.php ran
    1.17 ± 0.04 times faster than ./sapi/cli/php_old map.php

@nielsdos nielsdos force-pushed the array-perf-2 branch 2 times, most recently from 878649a to 07a4216 Compare March 27, 2025 21:39
@nielsdos nielsdos requested review from TimWolla and iluuu1994 March 27, 2025 22:46
@nielsdos nielsdos marked this pull request as ready for review March 27, 2025 22:47
@nielsdos nielsdos requested a review from bukka as a code owner March 27, 2025 22:47
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks correct to me.

}

fci.retval = &result;
fci.param_count = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this isn't moved up so that the two HT_IS_PACKED(input) branches can be joined?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason, I'll merge them.

if (HT_IS_PACKED(input)) {
uint32_t undefs = 0;
ZEND_HASH_FILL_PACKED(output) {
for (zval *cur = input->arPacked, *end = input->arPacked + input->nNumUsed; cur != end; cur++) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this not using ZEND_HASH_PACKED_FOREACH_VAL() to avoid skipping undef values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Member

Choose a reason for hiding this comment

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

I would add a comment, and possibly a test even to make sure this doesn't get changed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a comment now. Adding a test was not necessary as 3 tests already test this.

The refcounting and destruction is not necessary because zend_call_function
will make a copy anyway. And zend_call_function only returns FAILURE if
EG(active) is false in which case array_map shouldn't have been called
in the first place.
@nielsdos nielsdos force-pushed the array-perf-2 branch 2 times, most recently from b1b6963 to 846900b Compare March 28, 2025 18:09
@nielsdos nielsdos merged commit b6becad into php:master Mar 28, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants