From 8f15c2749f3f2e878b9d3c20ce475a3daf7eea24 Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sat, 25 Sep 2021 09:53:09 -0400 Subject: [PATCH 1/3] Performance testing: Bind static variables as non-references. The goal is to see if this is worth doing in opcache. This creates a static variable that is a value instead of a reference, subsequent assignments such as `$x = null;` do not modify the original declaration. (If $x was previously a reference before the `static` var statement, it continues to be unset before the expression on the right hand side is assigned to $x) (e.g. `static $x : = [new ArrayObject(['foo' => 'bar'])]` 1. Locally benchmark **until implementing changes so that opcache can infer when it is safe to bind a `static $var` with $var as a non-reference** (e.g. only used by value, no foreach by ref, no $$ variable access or functions such as extract, no `require*/include*` statements, not in the top-level file scope, etc.). For example, `function (): int { static $var = 2; return $var; }` is likely much less efficient than a local variable but opcache doesn't attempt to optimize it. 2. Work out how to make the opcache changes correctly and safely --- Zend/Optimizer/zend_inference.c | 16 +++++++++----- Zend/Optimizer/zend_ssa.h | 1 + .../static_variable_initialize_non_ref.phpt | 21 +++++++++++++++++++ Zend/zend_compile.c | 2 +- Zend/zend_compile.h | 3 +++ Zend/zend_language_parser.y | 5 +++-- Zend/zend_vm_def.h | 11 ++++++++-- Zend/zend_vm_execute.h | 11 ++++++++-- ext/reflection/php_reflection.c | 2 +- 9 files changed, 59 insertions(+), 13 deletions(-) create mode 100644 Zend/tests/static_variable_initialize_non_ref.phpt diff --git a/Zend/Optimizer/zend_inference.c b/Zend/Optimizer/zend_inference.c index 05b3700ca195..ccc40a7b68dd 100644 --- a/Zend/Optimizer/zend_inference.c +++ b/Zend/Optimizer/zend_inference.c @@ -3012,8 +3012,13 @@ static zend_always_inline zend_result _zend_update_type_info( UPDATE_SSA_TYPE(tmp, ssa_op->op1_def); break; case ZEND_BIND_STATIC: + /** + * NOTE: External PECL functions such as uopz_set_static allow modifying static variables, + * so they really can have any type. + * This is used in some unit testing frameworks to restore state after a unit test runs. + */ tmp = MAY_BE_ANY | MAY_BE_ARRAY_KEY_ANY | MAY_BE_ARRAY_OF_ANY | MAY_BE_ARRAY_OF_REF - | ((opline->extended_value & ZEND_BIND_REF) ? MAY_BE_REF : (MAY_BE_RC1 | MAY_BE_RCN)); + | ((((opline->extended_value & (ZEND_BIND_REF|ZEND_BIND_ACTUALLY_NON_REF)) == ZEND_BIND_REF)) ? MAY_BE_REF : (MAY_BE_RC1 | MAY_BE_RCN)); if (opline->extended_value & ZEND_BIND_IMPLICIT) { tmp |= MAY_BE_UNDEF; } @@ -4409,7 +4414,8 @@ static void zend_mark_cv_references(const zend_op_array *op_array, const zend_sc } break; case ZEND_BIND_STATIC: - if (!(opline->extended_value & ZEND_BIND_REF)) { + if ((opline->extended_value & (ZEND_BIND_REF|ZEND_BIND_ACTUALLY_NON_REF)) != ZEND_BIND_REF) { + /** Anything other than `static $x [= $val];` or `use(&$x) will not create references */ continue; } break; @@ -4795,11 +4801,11 @@ ZEND_API bool zend_may_throw_ex(const zend_op *opline, const zend_ssa_op *ssa_op return (t1 & (MAY_BE_OBJECT|MAY_BE_RESOURCE|MAY_BE_ARRAY_OF_OBJECT|MAY_BE_ARRAY_OF_RESOURCE|MAY_BE_ARRAY_OF_ARRAY)); case ZEND_BIND_STATIC: if (t1 & (MAY_BE_OBJECT|MAY_BE_RESOURCE|MAY_BE_ARRAY_OF_OBJECT|MAY_BE_ARRAY_OF_RESOURCE|MAY_BE_ARRAY_OF_ARRAY)) { - /* Destructor may throw. */ + /* Destructor may throw when the new value replaces the old value. */ return 1; } else { - zval *value = (zval*)((char*)op_array->static_variables->arData + (opline->extended_value & ~(ZEND_BIND_REF|ZEND_BIND_IMPLICIT|ZEND_BIND_EXPLICIT))); - /* May throw if initializer is CONSTANT_AST. */ + zval *value = (zval*)((char*)op_array->static_variables->arData + (opline->extended_value & ~ZEND_BIND_BITFLAG_COMBINATION)); + /* May throw if initializer is CONSTANT_AST (i.e. an AST instead of a value, because it wasn't precomputed at compile time). */ return Z_TYPE_P(value) == IS_CONSTANT_AST; } case ZEND_ASSIGN_DIM: diff --git a/Zend/Optimizer/zend_ssa.h b/Zend/Optimizer/zend_ssa.h index 93e6e8b26be5..2698270749f2 100644 --- a/Zend/Optimizer/zend_ssa.h +++ b/Zend/Optimizer/zend_ssa.h @@ -216,6 +216,7 @@ static zend_always_inline zend_ssa_phi* zend_ssa_next_use_phi(const zend_ssa *ss return NULL; } +/* Returns true if this is not a use of a previous assignment to var in this operation in the SSA graph. */ static zend_always_inline bool zend_ssa_is_no_val_use(const zend_op *opline, const zend_ssa_op *ssa_op, int var) { if (opline->opcode == ZEND_ASSIGN diff --git a/Zend/tests/static_variable_initialize_non_ref.phpt b/Zend/tests/static_variable_initialize_non_ref.phpt new file mode 100644 index 000000000000..af25cec2a988 --- /dev/null +++ b/Zend/tests/static_variable_initialize_non_ref.phpt @@ -0,0 +1,21 @@ +--TEST-- +Static variable initialize by non-reference (:=) +--FILE-- +append($value); + $values = $singleton->getArrayCopy(); + + $singleton = 'object overwritten by value (not by ref)'; + return [$values, $singleton]; +} +for ($i = 0; $i < 3; $i++) { + echo json_encode(test_append("x$i")), "\n"; +} + +?> +--EXPECT-- +[["x0"],"object overwritten by value (not by ref)"] +[["x0","x1"],"object overwritten by value (not by ref)"] +[["x0","x1","x2"],"object overwritten by value (not by ref)"] diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 892d51bc1820..ec79e150de3f 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -4746,7 +4746,7 @@ static void zend_compile_static_var(zend_ast *ast) /* {{{ */ ZVAL_NULL(&value_zv); } - zend_compile_static_var_common(zend_ast_get_str(var_ast), &value_zv, ZEND_BIND_REF); + zend_compile_static_var_common(zend_ast_get_str(var_ast), &value_zv, ZEND_BIND_REF | ast->attr); } /* }}} */ diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h index 90ce5504499b..67baec3cf76f 100644 --- a/Zend/zend_compile.h +++ b/Zend/zend_compile.h @@ -1043,10 +1043,13 @@ static zend_always_inline bool zend_check_arg_send_type(const zend_function *zf, #define ZEND_RETURN_VAL 0 #define ZEND_RETURN_REF 1 +/* These bit flags currently must be less than the zval size of 16 bytes, unless ZEND_VM_HANDLER for ZEND_BIND_STATIC is adjusted. */ #define ZEND_BIND_VAL 0 #define ZEND_BIND_REF 1 #define ZEND_BIND_IMPLICIT 2 #define ZEND_BIND_EXPLICIT 4 +#define ZEND_BIND_ACTUALLY_NON_REF 8 +#define ZEND_BIND_BITFLAG_COMBINATION (ZEND_BIND_REF|ZEND_BIND_IMPLICIT|ZEND_BIND_EXPLICIT|ZEND_BIND_ACTUALLY_NON_REF) #define ZEND_RETURNS_FUNCTION (1<<0) #define ZEND_RETURNS_VALUE (1<<1) diff --git a/Zend/zend_language_parser.y b/Zend/zend_language_parser.y index a8bddfae50a3..caa8e56268aa 100644 --- a/Zend/zend_language_parser.y +++ b/Zend/zend_language_parser.y @@ -894,8 +894,9 @@ static_var_list: ; static_var: - T_VARIABLE { $$ = zend_ast_create(ZEND_AST_STATIC, $1, NULL); } - | T_VARIABLE '=' expr { $$ = zend_ast_create(ZEND_AST_STATIC, $1, $3); } + T_VARIABLE { $$ = zend_ast_create(ZEND_AST_STATIC, $1, NULL); } + | T_VARIABLE '=' expr { $$ = zend_ast_create(ZEND_AST_STATIC, $1, $3); } + | T_VARIABLE ':' '=' expr { $$ = zend_ast_create(ZEND_AST_STATIC, $1, $4); $$->attr = ZEND_BIND_ACTUALLY_NON_REF; } ; class_statement_list: diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index 6a55dd9d87a5..882496f19438 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -8715,17 +8715,23 @@ ZEND_VM_HANDLER(183, ZEND_BIND_STATIC, CV, UNUSED, REF) } ZEND_ASSERT(GC_REFCOUNT(ht) == 1); - value = (zval*)((char*)ht->arData + (opline->extended_value & ~(ZEND_BIND_REF|ZEND_BIND_IMPLICIT|ZEND_BIND_EXPLICIT))); + value = (zval*)((char*)ht->arData + (opline->extended_value & ~ZEND_BIND_BITFLAG_COMBINATION)); - if (opline->extended_value & ZEND_BIND_REF) { + const uint32_t extended_value = opline->extended_value; + if (extended_value & ZEND_BIND_REF) { if (Z_TYPE_P(value) == IS_CONSTANT_AST) { SAVE_OPLINE(); if (UNEXPECTED(zval_update_constant_ex(value, EX(func)->op_array.scope) != SUCCESS)) { HANDLE_EXCEPTION(); } } + if (UNEXPECTED(extended_value & ZEND_BIND_ACTUALLY_NON_REF)) { + ZVAL_DEREF(value); + ZEND_VM_C_GOTO(copy_raw_value); + } i_zval_ptr_dtor(variable_ptr); + if (UNEXPECTED(!Z_ISREF_P(value))) { zend_reference *ref = (zend_reference*)emalloc(sizeof(zend_reference)); GC_SET_REFCOUNT(ref, 2); @@ -8740,6 +8746,7 @@ ZEND_VM_HANDLER(183, ZEND_BIND_STATIC, CV, UNUSED, REF) ZVAL_REF(variable_ptr, Z_REF_P(value)); } } else { +ZEND_VM_C_LABEL(copy_raw_value): i_zval_ptr_dtor(variable_ptr); ZVAL_COPY(variable_ptr, value); } diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index f2eb205d53c2..436a8afc133b 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -47400,17 +47400,23 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_BIND_STATIC_SPEC_CV_UNUSED_HAN } ZEND_ASSERT(GC_REFCOUNT(ht) == 1); - value = (zval*)((char*)ht->arData + (opline->extended_value & ~(ZEND_BIND_REF|ZEND_BIND_IMPLICIT|ZEND_BIND_EXPLICIT))); + value = (zval*)((char*)ht->arData + (opline->extended_value & ~ZEND_BIND_BITFLAG_COMBINATION)); - if (opline->extended_value & ZEND_BIND_REF) { + const uint32_t extended_value = opline->extended_value; + if (extended_value & ZEND_BIND_REF) { if (Z_TYPE_P(value) == IS_CONSTANT_AST) { SAVE_OPLINE(); if (UNEXPECTED(zval_update_constant_ex(value, EX(func)->op_array.scope) != SUCCESS)) { HANDLE_EXCEPTION(); } } + if (UNEXPECTED(extended_value & ZEND_BIND_ACTUALLY_NON_REF)) { + ZVAL_DEREF(value); + goto copy_raw_value; + } i_zval_ptr_dtor(variable_ptr); + if (UNEXPECTED(!Z_ISREF_P(value))) { zend_reference *ref = (zend_reference*)emalloc(sizeof(zend_reference)); GC_SET_REFCOUNT(ref, 2); @@ -47425,6 +47431,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_BIND_STATIC_SPEC_CV_UNUSED_HAN ZVAL_REF(variable_ptr, Z_REF_P(value)); } } else { +copy_raw_value: i_zval_ptr_dtor(variable_ptr); ZVAL_COPY(variable_ptr, value); } diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c index 9fa69428c51e..8eda789fb27e 100644 --- a/ext/reflection/php_reflection.c +++ b/ext/reflection/php_reflection.c @@ -1706,7 +1706,7 @@ ZEND_METHOD(ReflectionFunctionAbstract, getClosureUsedVariables) Bucket *bucket = (Bucket*) (((char*)static_variables->arData) + - (opline->extended_value & ~(ZEND_BIND_REF|ZEND_BIND_IMPLICIT|ZEND_BIND_EXPLICIT))); + (opline->extended_value & ~ZEND_BIND_BITFLAG_COMBINATION)); if (Z_ISUNDEF(bucket->val)) { continue; From 12c4cad9dd986948356a81f3921d2a066892ccb6 Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sat, 25 Sep 2021 10:54:13 -0400 Subject: [PATCH 2/3] Try something else --- Zend/zend_vm_def.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index 882496f19438..b546902ab71f 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -8725,7 +8725,7 @@ ZEND_VM_HANDLER(183, ZEND_BIND_STATIC, CV, UNUSED, REF) HANDLE_EXCEPTION(); } } - if (UNEXPECTED(extended_value & ZEND_BIND_ACTUALLY_NON_REF)) { + if (extended_value & ZEND_BIND_ACTUALLY_NON_REF) { ZVAL_DEREF(value); ZEND_VM_C_GOTO(copy_raw_value); } From c25f0173cefcd6fe20da9fcb4e5109c6f105199c Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Sun, 26 Sep 2021 11:24:03 -0400 Subject: [PATCH 3/3] Other changes, goto had worse performance --- Zend/zend_vm_def.h | 10 +++------- Zend/zend_vm_execute.h | 9 +++------ 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index b546902ab71f..044ca7c98b50 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -8725,14 +8725,11 @@ ZEND_VM_HANDLER(183, ZEND_BIND_STATIC, CV, UNUSED, REF) HANDLE_EXCEPTION(); } } - if (extended_value & ZEND_BIND_ACTUALLY_NON_REF) { - ZVAL_DEREF(value); - ZEND_VM_C_GOTO(copy_raw_value); - } - i_zval_ptr_dtor(variable_ptr); - if (UNEXPECTED(!Z_ISREF_P(value))) { + if (extended_value & ZEND_BIND_ACTUALLY_NON_REF) { + ZVAL_COPY_DEREF(variable_ptr, value); + } else if (UNEXPECTED(!Z_ISREF_P(value))) { zend_reference *ref = (zend_reference*)emalloc(sizeof(zend_reference)); GC_SET_REFCOUNT(ref, 2); GC_TYPE_INFO(ref) = GC_REFERENCE; @@ -8746,7 +8743,6 @@ ZEND_VM_HANDLER(183, ZEND_BIND_STATIC, CV, UNUSED, REF) ZVAL_REF(variable_ptr, Z_REF_P(value)); } } else { -ZEND_VM_C_LABEL(copy_raw_value): i_zval_ptr_dtor(variable_ptr); ZVAL_COPY(variable_ptr, value); } diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index 436a8afc133b..3f890d0d5013 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -47410,14 +47410,11 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_BIND_STATIC_SPEC_CV_UNUSED_HAN HANDLE_EXCEPTION(); } } - if (UNEXPECTED(extended_value & ZEND_BIND_ACTUALLY_NON_REF)) { - ZVAL_DEREF(value); - goto copy_raw_value; - } - i_zval_ptr_dtor(variable_ptr); - if (UNEXPECTED(!Z_ISREF_P(value))) { + if (extended_value & ZEND_BIND_ACTUALLY_NON_REF) { + ZVAL_COPY_DEREF(variable_ptr, value); + } else if (UNEXPECTED(!Z_ISREF_P(value))) { zend_reference *ref = (zend_reference*)emalloc(sizeof(zend_reference)); GC_SET_REFCOUNT(ref, 2); GC_TYPE_INFO(ref) = GC_REFERENCE;