Skip to content

Test array argument in-place optimization #11167

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

Closed
wants to merge 1 commit into from
Closed

Test array argument in-place optimization #11167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
344 changes: 344 additions & 0 deletions ext/standard/tests/array/array_modify_arg_in_place_optimization.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,344 @@
--TEST--
Test array argument in-place optimization
--EXTENSIONS--
zend_test
--FILE--
<?php

enum EN
{
case Y;
case N;
}

$listInt = [];
$listStr = [];
$mapInt = [];
$mapObj = [];
if (time() > 10) { // always true, but not const expr as time() is not CTE function
$listInt[] = 1;
$listInt[] = 2;
$listStr[] = 'a';
$mapInt[1] = 0;
$mapObj['e'] = EN::Y;
$mapObj['f'] = EN::N;
}

$anotherList = [8, 9];

// the arrays now have GC_IMMUTABLE and GC_PERSISTENT flags cleared

echo "*** array_merge ***\n";

// const empty 2nd array
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_merge($listInt, []);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($listStr);
$listStr = array_merge($listStr, []);
$ptrAfter = zend_get_array_ptr($listStr);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapInt);
$mapInt = array_merge($mapInt, []);
$ptrAfter = zend_get_array_ptr($mapInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_merge($mapObj, []);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

// const non-empty 2nd array
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_merge($listInt, [4]);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($listStr);
$listStr = array_merge($listStr, [4]);
$ptrAfter = zend_get_array_ptr($listStr);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapInt);
$mapInt = array_merge($mapInt, [4]);
$ptrAfter = zend_get_array_ptr($mapInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_merge($mapObj, [4]);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

// non-const non-empty 2nd array
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_merge($listInt, $anotherList);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($listStr);
$listStr = array_merge($listStr, $anotherList);
$ptrAfter = zend_get_array_ptr($listStr);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapInt);
$mapInt = array_merge($mapInt, $anotherList);
$ptrAfter = zend_get_array_ptr($mapInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_merge($mapObj, $anotherList);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

// non-1st argument as a result
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_merge($anotherList, $listInt);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($listStr);
$listStr = array_merge($anotherList, $listStr);
$ptrAfter = zend_get_array_ptr($listStr);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapInt);
$mapInt = array_merge($anotherList, $mapInt);
$ptrAfter = zend_get_array_ptr($mapInt);
var_dump($ptrAfter === $ptrBefore);

$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_merge($anotherList, $mapObj);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

echo "---\n";
foreach ($listInt as $v) {
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_merge($listInt, [$v]); // 2nd and 3rd iteration must not copy the array
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

if ($v === 1) { // 3rd iteration
break;
}
}
foreach (array_keys($listInt) as $k) {
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_merge($listInt, [$listInt[$k]]); // array must never be copied
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

if ($listInt[$k] === 1) { // 3rd iteration
break;
}
}

// TODO array_merge should be optimized to always return the 1st array zval if there is no element to be added

print_r($anotherList); // must not be modified
print_r($listInt);
print_r($listStr);
print_r($mapInt);
print_r($mapObj);

$listInt = array_slice($listInt, 0, 2, true);
$listStr = array_slice($listStr, 0, 1, true);
$mapInt = array_slice($mapInt, 0, 1, true);
$mapObj = array_slice($mapObj, 0, 2, true);


echo "*** array merge with unpacking ***\n";
// TODO

echo "*** array_diff family ***\n";
// TODO array_diff family should be optimized - https://github.com/php/php-src/pull/11060#discussion_r1175022196

echo "*** array_intersect family ***\n";
$oneListInt = array_slice($listInt, 0, 1, true);
$oneMapObj = array_slice($mapObj, 0, 1, true);

$listInt[] = -1;
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_intersect($listInt, $oneListInt);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$mapObj[] = -1;
$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_intersect($mapObj, $oneMapObj);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

$listInt[] = -1;
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_intersect_assoc($listInt, $oneListInt);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$mapObj[] = -1;
$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_intersect_assoc($mapObj, $oneMapObj);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

$listInt[] = -1;
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_intersect_key($listInt, $oneListInt);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$listInt[] = -1;
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_intersect_ukey($listInt, $oneListInt, fn ($k1, $k2) => $k1 <=> $k2);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$mapObj[] = -1;
$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_intersect_ukey($mapObj, $oneListInt, fn ($k1, $k2) => $k1 <=> $k2);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

echo "*** array_unique ***\n";

$listInt[] = end($listInt);
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_unique($listInt);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$mapObj[] = end($mapObj);
$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_unique($mapObj, SORT_REGULAR);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

echo "*** array_replace ***\n";

$listInt[] = end($listInt);
$ptrBefore = zend_get_array_ptr($listInt);
$listInt = array_replace($listInt, ['*']);
$ptrAfter = zend_get_array_ptr($listInt);
var_dump($ptrAfter === $ptrBefore);

$mapObj[] = end($mapObj);
$ptrBefore = zend_get_array_ptr($mapObj);
$mapObj = array_replace($mapObj, ['*']);
$ptrAfter = zend_get_array_ptr($mapObj);
var_dump($ptrAfter === $ptrBefore);

print_r($listInt);
print_r($mapObj);

?>
--EXPECT--
*** array_merge ***
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is zend_get_zval_ptr implemented correctly? It seems $ptrAfter === $ptrBefore is always true, which is not expected here.

Copy link
Member

Choose a reason for hiding this comment

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

You need to be aware of how zvals and arrays work behind the scenes.
A zval contains some data and some type information. For arrays this type is IS_ARRAY, and its data is a pointer to the array. When I do $a = $b, a and b are two different zvals, but if b is an array, then a will point to the same array because the pointer is copied (and the array refcount is increased by one).
Similarly if you have:

$a = [];
$a = [1];
$a = [1, 2];

then the zend_get_zval_ptr for $a will always return the same pointer for all three lines, because you're always asking the pointer for the container variable a. What you actually want to ask the engine is the pointer to the array (e.g. Z_ARRVAL_P(zval_pointer_for_a_here)`. Then you'll see that each line will have a different array pointer, while having the same zval pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that #11060 only introduces the optimisation for RC1 temporaries, not CVs. #11166 enables the optimisation for CVs.

@nielsdos $listInt in the test should have RC = 1 (excl. in the foreach test), but still the results are very different vs. what I would expect, can you please give me an example of php code where variable is "RC1 temporaries" and where "CV"?

Copy link
Member

Choose a reason for hiding this comment

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

@mvorisek CVs are never RC1 when passed to a function. #11166 will change this but only work with opcache (and optimizations) enabled. TMP/VAR will be RC1 if allocated within a function and then returned, of when a new value is generated from a temporary expression (e.g. [0 => 1] + [1 => 2]). range is another way to generate a temporary array with RC1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So array_merge function in $listInt = array_merge($listInt, []); expr, when $listInt had RC1 originally, will not be optimized, as array_merge will receive RC2? If yes, then can the testing using the before/after PTR be done before #11166 is landed?

Copy link
Member

Choose a reason for hiding this comment

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

So array_merge function in $listInt = array_merge($listInt, []); expr, when $listInt had RC1 originally, will not be optimized, as array_merge will receive RC2?

Right

If yes, then can the testing using the before/after PTR be done before #11166 is landed?

Sure, but that will only work in a limited context. E.g. pseudo main (root of the file) won't work, so you'll need to wrap it in a separate function. And as mentioned above, it will only work when opcache is enabled, as the optimizer otherwise doesn't run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nielsdos would you be ok to add this test to your #11166 directly?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait until that PR is merged and keep this one separate. Otherwise it will be a back and forth between us two which is a lot of overhead.

bool(false)
bool(false)
bool(false)
---
bool(false)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
Array
(
[0] => 8
[1] => 9
)
Array
(
[0] => 8
[1] => 9
[2] => 1
[3] => 2
[4] => 4
[5] => 8
[6] => 9
[7] => 8
[8] => 9
[9] => 1
[10] => 8
[11] => 9
[12] => 1
)
Array
(
[0] => 8
[1] => 9
[2] => a
[3] => 4
[4] => 8
[5] => 9
)
Array
(
[0] => 8
[1] => 9
[2] => 0
[3] => 4
[4] => 8
[5] => 9
)
Array
(
[0] => 8
[1] => 9
[e] => EN Enum
(
[name] => Y
)

[f] => EN Enum
(
[name] => N
)

[2] => 4
[3] => 8
[4] => 9
)
*** array merge with unpacking ***
*** array_diff family ***
*** array_intersect family ***
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
bool(true)
*** array_unique ***
bool(true)
bool(true)
*** array_replace ***
bool(true)
bool(true)
Array
(
[0] => *
[1] => 8
)
Array
(
[0] => *
[3] => 8
)
11 changes: 11 additions & 0 deletions ext/zend_test/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,17 @@ static ZEND_FUNCTION(zend_test_is_string_marked_as_valid_utf8)
RETURN_BOOL(ZSTR_IS_VALID_UTF8(str));
}

static ZEND_FUNCTION(zend_get_array_ptr)
{
zval *v;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_ARRAY(v)
ZEND_PARSE_PARAMETERS_END();

RETURN_LONG((zend_long) Z_ARR_P(v));
}

static ZEND_FUNCTION(ZendTestNS2_namespaced_func)
{
ZEND_PARSE_PARAMETERS_NONE();
Expand Down
2 changes: 2 additions & 0 deletions ext/zend_test/test.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ function zend_get_map_ptr_last(): int {}
function zend_test_crash(?string $message = null): void {}

function zend_test_fill_packed_array(array &$array): void {}

function zend_get_array_ptr(array $array): int {}
}

namespace ZendTestNS {
Expand Down
Loading