Hi,
Recently we've found a huge problem in PHP traits implementation.
It performs copying of each op_array (with all opcodes!) for each method
used from trait. This not only makes traits extremely slow and reduce
effect of opcode caches, but also prohibits extensions from modifying
op_array in some way, e.g. extending op_arrays with additional
information in op_array.reserved* fields. As result some extensions
(e.g. xdebug and some Zend extensions) will just crash on traits usage.
As I understood the copying was done only for proper handling of
__CLASS__ constant in trait methods. I think it's too radical solution.
I've introduced ZEND_CLASS_NAME instruction instead and made op_arrays
to share their opcodes (in the same way as inherited methods). The only
difference that methods from traits may be renamed.
The patch is attached (it requires executor/scanner/parser regeneration)
I would like to commit it into 5.4 and trunk. Note that the patch makes
binary compatibility break and can't be applied to 5.4.* after 5.4.0
release.
I know that applying it may delay the PHP 5.4.0 release, but it's better
to fix the problem now.
Thanks. Dmitry.
Index: Zend/zend_compile.c
===================================================================
--- Zend/zend_compile.c (revision 322174)
+++ Zend/zend_compile.c (working copy)
@@ -2226,6 +2226,19 @@
}
/* }}} */
+void zend_do_class_name(znode *result TSRMLS_DC) /* {{{ */
+{
+ zend_op *opline = get_next_op(CG(active_op_array) TSRMLS_CC);
+
+ opline->opcode = ZEND_CLASS_NAME;
+ SET_UNUSED(opline->op1);
+ SET_UNUSED(opline->op2);
+ opline->result_type = IS_TMP_VAR;
+ opline->result.var = get_temporary_variable(CG(active_op_array));
+ GET_NODE(result, opline->result);
+}
+/* }}} */
+
void zend_do_label(znode *label TSRMLS_DC) /* {{{ */
{
zend_label dest;
@@ -3670,142 +3683,6 @@
}
/* }}} */
-/* {{{ Originates from php_runkit_function_copy_ctor
- Duplicate structures in an op_array where necessary to make an outright
duplicate */
-static void zend_traits_duplicate_function(zend_function *fe, zend_class_entry
*target_ce, char *newname TSRMLS_DC)
-{
- zend_literal *literals_copy;
- zend_compiled_variable *dupvars;
- zend_op *opcode_copy;
- zval class_name_zv;
- int class_name_literal;
- int i;
- int number_of_literals;
-
- if (fe->op_array.static_variables) {
- HashTable *tmpHash;
-
- ALLOC_HASHTABLE(tmpHash);
- zend_hash_init(tmpHash,
zend_hash_num_elements(fe->op_array.static_variables), NULL, ZVAL_PTR_DTOR, 0);
- zend_hash_apply_with_arguments(fe->op_array.static_variables
TSRMLS_CC, (apply_func_args_t)zval_copy_static_var, 1, tmpHash);
-
- fe->op_array.static_variables = tmpHash;
- }
-
- number_of_literals = fe->op_array.last_literal;
- literals_copy = (zend_literal*)emalloc(number_of_literals *
sizeof(zend_literal));
-
- for (i = 0; i < number_of_literals; i++) {
- literals_copy[i] = fe->op_array.literals[i];
- zval_copy_ctor(&literals_copy[i].constant);
- }
- fe->op_array.literals = literals_copy;
-
-
- fe->op_array.refcount = emalloc(sizeof(zend_uint));
- *(fe->op_array.refcount) = 1;
-
- if (fe->op_array.vars) {
- i = fe->op_array.last_var;
- dupvars = safe_emalloc(fe->op_array.last_var,
sizeof(zend_compiled_variable), 0);
- while (i > 0) {
- i--;
- dupvars[i].name = estrndup(fe->op_array.vars[i].name,
fe->op_array.vars[i].name_len);
- dupvars[i].name_len = fe->op_array.vars[i].name_len;
- dupvars[i].hash_value = fe->op_array.vars[i].hash_value;
- }
- fe->op_array.vars = dupvars;
- } else {
- fe->op_array.vars = NULL;
- }
-
- opcode_copy = safe_emalloc(sizeof(zend_op), fe->op_array.last, 0);
- for(i = 0; i < fe->op_array.last; i++) {
- opcode_copy[i] = fe->op_array.opcodes[i];
- if (opcode_copy[i].op1_type != IS_CONST) {
- switch (opcode_copy[i].opcode) {
- case ZEND_GOTO:
- case ZEND_JMP:
- if (opcode_copy[i].op1.jmp_addr &&
opcode_copy[i].op1.jmp_addr >= fe->op_array.opcodes &&
- opcode_copy[i].op1.jmp_addr <
fe->op_array.opcodes + fe->op_array.last) {
- opcode_copy[i].op1.jmp_addr =
opcode_copy + (fe->op_array.opcodes[i].op1.jmp_addr - fe->op_array.opcodes);
- }
- break;
- }
- } else {
- /* if __CLASS__ i.e. T_CLASS_C was used, we need to fix
it up here */
- if (target_ce
- /* REM: used a IS_NULL place holder with a
special marker LVAL */
- && Z_TYPE_P(opcode_copy[i].op1.zv) == IS_NULL
- && Z_LVAL_P(opcode_copy[i].op1.zv) ==
ZEND_ACC_TRAIT
- /* Only on merge into an actual class */
- && (ZEND_ACC_TRAIT != (target_ce->ce_flags &
ZEND_ACC_TRAIT)))
- {
- INIT_ZVAL(class_name_zv);
- ZVAL_STRINGL(&class_name_zv, target_ce->name,
target_ce->name_length, 1);
- class_name_literal =
zend_append_individual_literal(&fe->op_array, &class_name_zv TSRMLS_CC);
- opcode_copy[i].op1.zv =
&fe->op_array.literals[class_name_literal].constant;
- }
- }
-
- if (opcode_copy[i].op2_type != IS_CONST) {
- switch (opcode_copy[i].opcode) {
- case ZEND_JMPZ:
- case ZEND_JMPNZ:
- case ZEND_JMPZ_EX:
- case ZEND_JMPNZ_EX:
- case ZEND_JMP_SET:
- case ZEND_JMP_SET_VAR:
- if (opcode_copy[i].op2.jmp_addr &&
opcode_copy[i].op2.jmp_addr >= fe->op_array.opcodes &&
- opcode_copy[i].op2.jmp_addr <
fe->op_array.opcodes + fe->op_array.last) {
- opcode_copy[i].op2.jmp_addr =
opcode_copy + (fe->op_array.opcodes[i].op2.jmp_addr - fe->op_array.opcodes);
- }
- break;
- }
- } else {
- /* if __CLASS__ i.e. T_CLASS_C was used, we need to fix
it up here */
- if (target_ce
- /* REM: used a IS_NULL place holder with a
special marker LVAL */
- && Z_TYPE_P(opcode_copy[i].op2.zv) == IS_NULL
- && Z_LVAL_P(opcode_copy[i].op2.zv) ==
ZEND_ACC_TRAIT
- /* Only on merge into an actual class */
- && (ZEND_ACC_TRAIT != (target_ce->ce_flags &
ZEND_ACC_TRAIT)))
- {
- INIT_ZVAL(class_name_zv);
- ZVAL_STRINGL(&class_name_zv, target_ce->name,
target_ce->name_length, 1);
- class_name_literal =
zend_append_individual_literal(&fe->op_array, &class_name_zv TSRMLS_CC);
- opcode_copy[i].op2.zv =
&fe->op_array.literals[class_name_literal].constant;
- }
- }
- }
- fe->op_array.opcodes = opcode_copy;
- fe->op_array.function_name = newname;
-
- /* was setting it to fe which does not work since fe is stack allocated
and not a stable address */
- /* fe->op_array.prototype = fe->op_array.prototype; */
- if (fe->op_array.arg_info) {
- zend_arg_info *tmpArginfo;
-
- tmpArginfo = safe_emalloc(sizeof(zend_arg_info),
fe->op_array.num_args, 0);
- for(i = 0; i < fe->op_array.num_args; i++) {
- tmpArginfo[i] = fe->op_array.arg_info[i];
-
- tmpArginfo[i].name = estrndup(tmpArginfo[i].name,
tmpArginfo[i].name_len);
- if (tmpArginfo[i].class_name) {
- tmpArginfo[i].class_name =
estrndup(tmpArginfo[i].class_name, tmpArginfo[i].class_name_len);
- }
- }
- fe->op_array.arg_info = tmpArginfo;
- }
-
- fe->op_array.doc_comment = estrndup(fe->op_array.doc_comment,
fe->op_array.doc_comment_len);
- fe->op_array.try_catch_array =
(zend_try_catch_element*)estrndup((char*)fe->op_array.try_catch_array,
sizeof(zend_try_catch_element) * fe->op_array.last_try_catch);
-
- fe->op_array.brk_cont_array =
(zend_brk_cont_element*)estrndup((char*)fe->op_array.brk_cont_array,
sizeof(zend_brk_cont_element) * fe->op_array.last_brk_cont);
-
-}
-/* }}} */
-
static void zend_add_magic_methods(zend_class_entry* ce, const char* mname,
uint mname_len, zend_function* fe TSRMLS_DC) /* {{{ */
{
if (!strncmp(mname, ZEND_CLONE_FUNC_NAME, mname_len)) {
@@ -3916,7 +3793,7 @@
ce->ce_flags |= ZEND_HAS_STATIC_IN_METHODS;
}
fn_copy = *fn;
- zend_traits_duplicate_function(&fn_copy, ce,
estrdup(fn->common.function_name) TSRMLS_CC);
+ function_add_ref(&fn_copy);
if (zend_hash_quick_update(&ce->function_table,
hash_key->arKey, hash_key->nKeyLength, hash_key->h, &fn_copy,
sizeof(zend_function), (void**)&fn_copy_p)==FAILURE) {
zend_error(E_COMPILE_ERROR, "Trait method %s has not
been applied, because failure occured during updating class method table",
hash_key->arKey);
@@ -3961,7 +3838,11 @@
&& aliases[i]->trait_method->mname_len ==
fnname_len
&&
(zend_binary_strcasecmp(aliases[i]->trait_method->method_name,
aliases[i]->trait_method->mname_len, fn->common.function_name, fnname_len) ==
0)) {
fn_copy = *fn;
- zend_traits_duplicate_function(&fn_copy, NULL,
estrndup(aliases[i]->alias, aliases[i]->alias_len) TSRMLS_CC);
+ function_add_ref(&fn_copy);
+ /* this function_name is never destroied,
because its refcount
+ greater than 1, classes are always destoyed
in reverse order
+ and trait is declared early than this class
*/
+ fn_copy.common.function_name =
aliases[i]->alias;
/* if it is 0, no modifieres has been changed */
if (aliases[i]->modifiers) {
@@ -3993,7 +3874,7 @@
if (exclude_table == NULL || zend_hash_find(exclude_table, lcname,
fnname_len, &dummy) == FAILURE) {
/* is not in hashtable, thus, function is not to be excluded */
fn_copy = *fn;
- zend_traits_duplicate_function(&fn_copy, NULL,
estrndup(fn->common.function_name, fnname_len) TSRMLS_CC);
+ function_add_ref(&fn_copy);
/* apply aliases which are not qualified by a class name, or
which have not
* alias name, just setting visibility */
Index: Zend/zend_compile.h
===================================================================
--- Zend/zend_compile.h (revision 322174)
+++ Zend/zend_compile.h (working copy)
@@ -485,6 +485,7 @@
void zend_do_clone(znode *result, const znode *expr TSRMLS_DC);
void zend_do_begin_dynamic_function_call(znode *function_name, int prefix_len
TSRMLS_DC);
void zend_do_fetch_class(znode *result, znode *class_name TSRMLS_DC);
+void zend_do_class_name(znode *result TSRMLS_DC);
void zend_do_build_full_name(znode *result, znode *prefix, znode *name, int
is_class_member TSRMLS_DC);
int zend_do_begin_class_member_function_call(znode *class_name, znode
*method_name TSRMLS_DC);
void zend_do_end_function_call(znode *function_name, znode *result, const
znode *argument_list, int is_method, int is_dynamic_fcall TSRMLS_DC);
Index: Zend/zend_language_parser.y
===================================================================
--- Zend/zend_language_parser.y (revision 322174)
+++ Zend/zend_language_parser.y (working copy)
@@ -907,7 +907,7 @@
| T_LINE { $$ = $1; }
| T_FILE { $$ = $1; }
| T_DIR { $$ = $1; }
- | T_CLASS_C { $$ = $1; }
+ | T_CLASS_C { if
(Z_TYPE($1.u.constant) == IS_NULL) {zend_do_class_name(&$$ TSRMLS_CC);} else
{$$ = $1;} }
| T_TRAIT_C { $$ = $1; }
| T_METHOD_C { $$ = $1; }
| T_FUNC_C { $$ = $1; }
Index: Zend/zend_vm_def.h
===================================================================
--- Zend/zend_vm_def.h (revision 322174)
+++ Zend/zend_vm_def.h (working copy)
@@ -5156,4 +5156,18 @@
ZEND_VM_NEXT_OPCODE();
}
+ZEND_VM_HANDLER(159, ZEND_CLASS_NAME, ANY, ANY)
+{
+ USE_OPLINE
+ zval *tmp = &EX_T(opline->result.var).tmp_var;
+
+ SAVE_OPLINE();
+ Z_STRVAL_P(tmp) = IS_INTERNED(EG(scope)->name) ? EG(scope)->name :
estrndup(EG(scope)->name, EG(scope)->name_length);
+ Z_STRLEN_P(tmp) = EG(scope)->name_length;
+ Z_TYPE_P(tmp) = IS_STRING;
+ Z_SET_REFCOUNT_P(tmp, 1);
+ Z_UNSET_ISREF_P(tmp);
+ ZEND_VM_NEXT_OPCODE();
+}
+
ZEND_VM_EXPORT_HELPER(zend_do_fcall, zend_do_fcall_common_helper)
Index: Zend/zend_language_scanner.l
===================================================================
--- Zend/zend_language_scanner.l (revision 322174)
+++ Zend/zend_language_scanner.l (working copy)
@@ -1564,9 +1564,9 @@
if (CG(active_class_entry)
&& (ZEND_ACC_TRAIT ==
(CG(active_class_entry)->ce_flags & ZEND_ACC_TRAIT))) {
- // This is a hack, we abuse IS_NULL to indicate an invalid value
- // if __CLASS__ is encountered in a trait, however, we also not
that we
- // should fix it up when we copy the method into an actual class
+ /* This is a hack, we abuse IS_NULL to indicate an invalid value
+ if __CLASS__ is encountered in a trait, however, we
+ should get the actual class name using ZEND_CLASS_NAME
instruction */
zendlval->value.lval = ZEND_ACC_TRAIT;
zendlval->type = IS_NULL;
} else {
@@ -1579,7 +1579,7 @@
}
zendlval->value.str.len = strlen(class_name);
- zendlval->value.str.val = estrndup(class_name,
zendlval->value.str.len);
+ zendlval->value.str.val = IS_INTERNED(class_name) ? class_name
: estrndup(class_name, zendlval->value.str.len);
zendlval->type = IS_STRING;
}
return T_CLASS_C;
@@ -1599,7 +1599,7 @@
}
zendlval->value.str.len = strlen(trait_name);
- zendlval->value.str.val = estrndup(trait_name, zendlval->value.str.len);
+ zendlval->value.str.val = IS_INTERNED(trait_name) ? trait_name :
estrndup(trait_name, zendlval->value.str.len);
zendlval->type = IS_STRING;
return T_TRAIT_C;
@@ -1616,7 +1616,7 @@
func_name = "";
}
zendlval->value.str.len = strlen(func_name);
- zendlval->value.str.val = estrndup(func_name, zendlval->value.str.len);
+ zendlval->value.str.val = IS_INTERNED(func_name) ? func_name :
estrndup(func_name, zendlval->value.str.len);
zendlval->type = IS_STRING;
return T_FUNC_C;
}
@@ -1655,7 +1655,7 @@
filename = "";
}
zendlval->value.str.len = strlen(filename);
- zendlval->value.str.val = estrndup(filename, zendlval->value.str.len);
+ zendlval->value.str.val = IS_INTERNED(filename) ? filename :
estrndup(filename, zendlval->value.str.len);
zendlval->type = IS_STRING;
return T_FILE;
}
Index: Zend/zend_execute_API.c
===================================================================
--- Zend/zend_execute_API.c (revision 322174)
+++ Zend/zend_execute_API.c (working copy)
@@ -110,7 +110,7 @@
static int clean_non_persistent_function_full(zend_function *function
TSRMLS_DC) /* {{{ */
{
- return (function->type != ZEND_INTERNAL_FUNCTION);
+ return (function->type == ZEND_INTERNAL_FUNCTION) ?
ZEND_HASH_APPLY_KEEP : ZEND_HASH_APPLY_REMOVE;
}
/* }}} */
@@ -122,7 +122,7 @@
static int clean_non_persistent_class_full(zend_class_entry **ce TSRMLS_DC) /*
{{{ */
{
- return ((*ce)->type != ZEND_INTERNAL_CLASS);
+ return ((*ce)->type == ZEND_INTERNAL_CLASS) ? ZEND_HASH_APPLY_KEEP :
ZEND_HASH_APPLY_REMOVE;
}
/* }}} */
@@ -298,8 +298,8 @@
/* Destroy all op arrays */
if (EG(full_tables_cleanup)) {
- zend_hash_apply(EG(function_table), (apply_func_t)
clean_non_persistent_function_full TSRMLS_CC);
- zend_hash_apply(EG(class_table), (apply_func_t)
clean_non_persistent_class_full TSRMLS_CC);
+ zend_hash_reverse_apply(EG(function_table),
(apply_func_t) clean_non_persistent_function_full TSRMLS_CC);
+ zend_hash_reverse_apply(EG(class_table), (apply_func_t)
clean_non_persistent_class_full TSRMLS_CC);
} else {
zend_hash_reverse_apply(EG(function_table),
(apply_func_t) clean_non_persistent_function TSRMLS_CC);
zend_hash_reverse_apply(EG(class_table), (apply_func_t)
clean_non_persistent_class TSRMLS_CC);
Index: ext/reflection/tests/traits001.phpt
===================================================================
--- ext/reflection/tests/traits001.phpt (revision 322174)
+++ ext/reflection/tests/traits001.phpt (working copy)
@@ -63,7 +63,6 @@
@@ %straits001.php 9 - 9
}
-
Method [ <user> public method someMethod ] {
@@ %straits001.php 3 - 3
}
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
