Skip to content

Improve performance of ZEND_IS_IDENTICAL #4900

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 5 commits into from
Closed

Improve performance of ZEND_IS_IDENTICAL #4900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58d9397
Improve performance of ZEND_IS_IDENTICAL
TysonAndre Nov 10, 2019
72ec3a6
Address review feedback
TysonAndre Nov 11, 2019
2f96eba
Optimize for not being indirect/undef/references
TysonAndre Nov 11, 2019
aec0a54
Fix MacOS build error
TysonAndre Nov 13, 2019
18a672b
Fix undefined zval check
TysonAndre Nov 13, 2019
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
10 changes: 10 additions & 0 deletions Zend/tests/bug70785.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ try {
undefined_function();
} catch (Exception $e) {
}
try {
$d === $f; // ZEND_VM_NEXT_OPCODE
undefined_function();
} catch (Exception $e) {
}
try {
$h !== $g; // ZEND_VM_NEXT_OPCODE
undefined_function();
} catch (Exception $e) {
}
?>
okey
--EXPECT--
Expand Down
6 changes: 6 additions & 0 deletions Zend/zend_operators.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2118,6 +2118,12 @@ ZEND_API zend_bool ZEND_FASTCALL zend_is_identical(zval *op1, zval *op2) /* {{{
}
/* }}} */

ZEND_API zend_bool ZEND_FASTCALL zend_is_identical_array(HashTable *h1, HashTable *h2) /* {{{ */
{
return (h1 == h2 ||
zend_hash_compare(h1, h2, (compare_func_t) hash_zval_identical_function, 1) == 0);
}

ZEND_API int ZEND_FASTCALL is_identical_function(zval *result, zval *op1, zval *op2) /* {{{ */
{
ZVAL_BOOL(result, zend_is_identical(op1, op2));
Expand Down
1 change: 1 addition & 0 deletions Zend/zend_operators.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ ZEND_API int ZEND_FASTCALL shift_right_function(zval *result, zval *op1, zval *o
ZEND_API int ZEND_FASTCALL concat_function(zval *result, zval *op1, zval *op2);

ZEND_API zend_bool ZEND_FASTCALL zend_is_identical(zval *op1, zval *op2);
ZEND_API zend_bool ZEND_FASTCALL zend_is_identical_array(HashTable *h1, HashTable *h2);

ZEND_API int ZEND_FASTCALL is_equal_function(zval *result, zval *op1, zval *op2);
ZEND_API int ZEND_FASTCALL is_identical_function(zval *result, zval *op1, zval *op2);
Expand Down
198 changes: 185 additions & 13 deletions Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,9 +449,95 @@ ZEND_VM_COLD_CONSTCONST_HANDLER(16, ZEND_IS_IDENTICAL, CONST|TMP|VAR|CV, CONST|T
zend_bool result;

SAVE_OPLINE();
op1 = GET_OP1_ZVAL_PTR_DEREF(BP_VAR_R);
op2 = GET_OP2_ZVAL_PTR_DEREF(BP_VAR_R);
result = fast_is_identical_function(op1, op2);
op1 = GET_OP1_ZVAL_PTR_UNDEF(BP_VAR_R);
op2 = GET_OP2_ZVAL_PTR_UNDEF(BP_VAR_R);
ZEND_VM_C_LABEL(compare_values_any_type):

if (Z_TYPE_P(op1) != Z_TYPE_P(op2)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to fetch op1 as UNDEF and handle the REF/UNDEF cases explicitly, so we get a fast path if they do not apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually working on that locally - I didn't see much of a performance boost locally (I assume it's a compare and conditional jump, for 2 instructions)

  • Maybe moving an is_identical_function implementation into the

I was also thinking of skipping the IS_UNDEF check for const/tmp/var, and IS_INDIRECT for everything except var

Copy link
Contributor Author

@TysonAndre TysonAndre Nov 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/TysonAndre/php-src/pull/new/ZEND_IS_IDENTICAL-wip-broken - will fix this later today or tomorrow.

I forgot to check for IS_INDIRECT in the case for if (Z_TYPE_P(op1) != Z_TYPE_P(op2)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue I was having was caused by opcache being enabled in the php.ini that would have been used by the installation. I think IS_INDIRECT still needed to be handled

/* Only VAR can be indirect */
if (UNEXPECTED(((OP1_TYPE & IS_VAR) && Z_TYPE_P(op1) == IS_INDIRECT) || ((OP2_TYPE & IS_VAR) && Z_TYPE_P(op2) == IS_INDIRECT))) {
ZVAL_DEINDIRECT(op1);
ZVAL_DEINDIRECT(op2);
ZEND_VM_C_GOTO(compare_values_any_type);
}
/* Only VAR and CV can be references */
if (UNEXPECTED(((OP1_TYPE & (IS_VAR|IS_CV)) && Z_ISREF_P(op1)) || ((OP2_TYPE & (IS_VAR|IS_CV)) && Z_ISREF_P(op2)))) {
ZVAL_DEREF(op1);
ZVAL_DEREF(op2);
if (Z_TYPE_P(op1) == Z_TYPE_P(op2)) {
ZEND_VM_C_GOTO(compare_values);
}
}
/* Only CV can be undef */
if (UNEXPECTED(((OP1_TYPE & IS_CV) && Z_ISUNDEF_P(op1)) || ((OP2_TYPE & IS_CV) && Z_ISUNDEF_P(op2)) )) {
/* Convert undef to null, check if they're identical */
if (Z_TYPE_P(op1) == IS_UNDEF) {
op1 = ZVAL_UNDEFINED_OP1();
}
if (Z_TYPE_P(op2) == IS_UNDEF) {
op2 = ZVAL_UNDEFINED_OP2();
}
result = Z_TYPE_P(op1) == Z_TYPE_P(op2);
ZEND_VM_C_GOTO(return_result_maythrow);
}
/* They are not identical, return false. This has to check for __destruct errors */
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(0, 1);
}
ZEND_VM_C_LABEL(compare_values):
if (Z_TYPE_P(op1) <= IS_TRUE) {
if (((OP1_TYPE & IS_CV) && UNEXPECTED(Z_TYPE_P(op1) == IS_UNDEF)) || ((OP2_TYPE & IS_CV) && UNEXPECTED(Z_TYPE_P(op2) == IS_UNDEF))) {
/* They are both undefined - fetch them to emit the undefined variable warnings. */
op1 = ZVAL_UNDEFINED_OP1();
op2 = ZVAL_UNDEFINED_OP2();
ZEND_VM_SMART_BRANCH(1, 1);
}
/* They are identical, return true */
/* This has to check for undefined variable errors when IS_UNDEF is possible. (only warns for IS_CV) */
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(1, 0);
}
switch (Z_TYPE_P(op1)) {
case IS_LONG:
result = (Z_LVAL_P(op1) == Z_LVAL_P(op2));
ZEND_VM_C_LABEL(free_nothrow):
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(result, 0);
break;
case IS_RESOURCE:
result = (Z_RES_P(op1) == Z_RES_P(op2));
break;
case IS_DOUBLE:
result = (Z_DVAL_P(op1) == Z_DVAL_P(op2));
ZEND_VM_C_GOTO(free_nothrow);
case IS_STRING:
result = zend_string_equals(Z_STR_P(op1), Z_STR_P(op2));
ZEND_VM_C_GOTO(free_nothrow);
case IS_ARRAY:
/* This may cause EG(exception) due to infinite nesting, but other zval types won't? */
/* XXX hash_zval_identical_function is not static */
result = zend_is_identical_array(Z_ARRVAL_P(op1), Z_ARRVAL_P(op2));
break;
case IS_OBJECT:
result = (Z_OBJ_P(op1) == Z_OBJ_P(op2));
break;
case IS_REFERENCE:
/* Both are references */
op1 = Z_REFVAL_P(op1);
op2 = Z_REFVAL_P(op2);
ZEND_VM_C_GOTO(compare_values_any_type);
case IS_INDIRECT:
op1 = Z_INDIRECT_P(op1);
op2 = Z_INDIRECT_P(op2);
ZEND_VM_C_GOTO(compare_values_any_type);
default:
result = 1;
}
ZEND_VM_C_LABEL(return_result_maythrow):
/* Check if freeing the operands (e.g. __destruct(), freeing resources (not sure about that), etc threw an exception before setting the result or branching */
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(result, 1);
Expand All @@ -464,9 +550,95 @@ ZEND_VM_COLD_CONSTCONST_HANDLER(17, ZEND_IS_NOT_IDENTICAL, CONST|TMP|VAR|CV, CON
zend_bool result;

SAVE_OPLINE();
op1 = GET_OP1_ZVAL_PTR_DEREF(BP_VAR_R);
op2 = GET_OP2_ZVAL_PTR_DEREF(BP_VAR_R);
result = fast_is_not_identical_function(op1, op2);
op1 = GET_OP1_ZVAL_PTR_UNDEF(BP_VAR_R);
op2 = GET_OP2_ZVAL_PTR_UNDEF(BP_VAR_R);
ZEND_VM_C_LABEL(compare_values_any_type):

if (Z_TYPE_P(op1) != Z_TYPE_P(op2)) {
/* Only VAR can be indirect */
if (UNEXPECTED(((OP1_TYPE & IS_VAR) && Z_TYPE_P(op1) == IS_INDIRECT) || ((OP2_TYPE & IS_VAR) && Z_TYPE_P(op2) == IS_INDIRECT))) {
ZVAL_DEINDIRECT(op1);
ZVAL_DEINDIRECT(op2);
ZEND_VM_C_GOTO(compare_values_any_type);
}
/* Only VAR and CV can be references */
if (UNEXPECTED(((OP1_TYPE & (IS_VAR|IS_CV)) && Z_ISREF_P(op1)) || ((OP2_TYPE & (IS_VAR|IS_CV)) && Z_ISREF_P(op2)))) {
ZVAL_DEREF(op1);
ZVAL_DEREF(op2);
if (Z_TYPE_P(op1) == Z_TYPE_P(op2)) {
ZEND_VM_C_GOTO(compare_values);
}
}
/* Only CV can be undef */
if (UNEXPECTED(((OP1_TYPE & IS_CV) && Z_ISUNDEF_P(op1)) || ((OP2_TYPE & IS_CV) && Z_ISUNDEF_P(op2)) )) {
/* Convert undef to null, check if they're not identical */
if (Z_TYPE_P(op1) == IS_UNDEF) {
op1 = ZVAL_UNDEFINED_OP1();
}
if (Z_TYPE_P(op2) == IS_UNDEF) {
op2 = ZVAL_UNDEFINED_OP2();
}
result = Z_TYPE_P(op1) != Z_TYPE_P(op2);
ZEND_VM_C_GOTO(return_result_maythrow);
}
/* They are not identical, return true. This has to check for __destruct errors (and undefined variable errors when IS_NULL is possible) */
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(1, 1);
}
ZEND_VM_C_LABEL(compare_values):
if (Z_TYPE_P(op1) <= IS_TRUE) {
if (((OP1_TYPE & IS_CV) && UNEXPECTED(Z_TYPE_P(op1) == IS_UNDEF)) || ((OP2_TYPE & IS_CV) && UNEXPECTED(Z_TYPE_P(op2) == IS_UNDEF))) {
/* They are both undefined - fetch them to emit the undefined variable warnings. */
op1 = ZVAL_UNDEFINED_OP1();
op2 = ZVAL_UNDEFINED_OP2();
ZEND_VM_SMART_BRANCH(0, 1);
}
/* They are identical, return false. */
/* This has to check for undefined variable errors when IS_UNDEF is possible. (only warns for IS_CV) */
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(0, 0);
}
switch (Z_TYPE_P(op1)) {
case IS_LONG:
result = (Z_LVAL_P(op1) != Z_LVAL_P(op2));
ZEND_VM_C_LABEL(free_nothrow):
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(result, 0);
break;
case IS_RESOURCE:
result = (Z_RES_P(op1) != Z_RES_P(op2));
break;
case IS_DOUBLE:
result = (Z_DVAL_P(op1) != Z_DVAL_P(op2));
ZEND_VM_C_GOTO(free_nothrow);
case IS_STRING:
result = !zend_string_equals(Z_STR_P(op1), Z_STR_P(op2));
ZEND_VM_C_GOTO(free_nothrow);
case IS_ARRAY:
/* This may cause EG(exception) due to infinite nesting, but other zval types won't? */
/* XXX hash_zval_identical_function is not static */
result = !zend_is_identical_array(Z_ARRVAL_P(op1), Z_ARRVAL_P(op2));
break;
case IS_OBJECT:
result = (Z_OBJ_P(op1) != Z_OBJ_P(op2));
break;
case IS_REFERENCE:
/* Both are references */
op1 = Z_REFVAL_P(op1);
op2 = Z_REFVAL_P(op2);
ZEND_VM_C_GOTO(compare_values_any_type);
case IS_INDIRECT:
op1 = Z_INDIRECT_P(op1);
op2 = Z_INDIRECT_P(op2);
ZEND_VM_C_GOTO(compare_values_any_type);
default:
result = 1;
}
ZEND_VM_C_LABEL(return_result_maythrow):
/* Check if freeing the operands (e.g. __destruct(), freeing resources (not sure about that), etc threw an exception before setting the result or branching */
FREE_OP1();
FREE_OP2();
ZEND_VM_SMART_BRANCH(result, 1);
Expand Down Expand Up @@ -1496,7 +1668,7 @@ ZEND_VM_HELPER(zend_pre_dec_helper, VAR|CV, ANY)

SAVE_OPLINE();
if (OP1_TYPE == IS_CV && UNEXPECTED(Z_TYPE_P(var_ptr) == IS_UNDEF)) {
ZVAL_NULL(var_ptr);
ZVAL_NULL(var_ptr);
ZVAL_UNDEFINED_OP1();
}

Expand Down Expand Up @@ -2234,7 +2406,7 @@ ZEND_VM_C_LABEL(fetch_obj_is_fast_copy):
}

retval = zobj->handlers->read_property(zobj, name, BP_VAR_IS, cache_slot, EX_VAR(opline->result.var));

if (OP2_TYPE != IS_CONST) {
zend_tmp_string_release(tmp_name);
}
Expand Down Expand Up @@ -5528,10 +5700,10 @@ ZEND_VM_HANDLER(147, ZEND_ADD_ARRAY_UNPACK, ANY, ANY)
{
USE_OPLINE
zval *op1;

SAVE_OPLINE();
op1 = GET_OP1_ZVAL_PTR(BP_VAR_R);

ZEND_VM_C_LABEL(add_unpack_again):
if (EXPECTED(Z_TYPE_P(op1) == IS_ARRAY)) {
HashTable *ht = Z_ARRVAL_P(op1);
Expand Down Expand Up @@ -5572,11 +5744,11 @@ ZEND_VM_C_LABEL(add_unpack_again):
}
HANDLE_EXCEPTION();
}

if (iter->funcs->rewind) {
iter->funcs->rewind(iter);
}

for (; iter->funcs->valid(iter) == SUCCESS; ) {
zval *val;

Expand Down Expand Up @@ -5625,7 +5797,7 @@ ZEND_VM_C_LABEL(add_unpack_again):
} else {
zend_throw_error(NULL, "Only arrays and Traversables can be unpacked");
}

FREE_OP1();
ZEND_VM_NEXT_OPCODE_CHECK_EXCEPTION();
}
Expand Down
Loading