-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Implement frameless static calls #16471
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.
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
Conversation
Zend/zend_compile.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I actually forgot that these operands may contain multiple literals... Optimally we could attempt to compile frameless calls before emitting any opcodes, but the loading of fbc currently uses lcname emitted by zend_set_class_name_op1, a bit annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It would be better to check for frame-less variant before emitting INIT_STATIC_METHOD_CALL
This may be done with the alternative following patch.
The only drawback is that zend_string_tolower() called twice times more.
diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 7e5351ddfaf..c479d97b27a 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -5329,6 +5329,47 @@ static void zend_compile_static_call(znode *result, zend_ast *ast, uint32_t type } } + /* Check if we already know which method we're calling */ + if (method_node.op_type == IS_CONST) { + zend_class_entry *ce = NULL; + if (class_node.op_type == IS_CONST) { + zend_string *lcname = zend_string_tolower(Z_STR(class_node.u.constant)); + ce = zend_hash_find_ptr(CG(class_table), lcname); + if (ce) { + if (zend_compile_ignore_class(ce, CG(active_op_array)->filename)) { + ce = NULL; + } + } else if (CG(active_class_entry) + && zend_string_equals_ci(CG(active_class_entry)->name, lcname)) { + ce = CG(active_class_entry); + } + zend_string_release(lcname); + } else if (class_node.op_type == IS_UNUSED + && (class_node.u.op.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF + && zend_is_scope_known()) { + ce = CG(active_class_entry); + } + if (ce) { + zend_string *lcname = zend_string_tolower(Z_STR(method_node.u.constant)); + fbc = zend_get_compatible_func_or_null(ce, lcname); + zend_string_release(lcname); + + if (fbc + && !(CG(compiler_options) & ZEND_COMPILE_NO_BUILTINS) + && (fbc->type == ZEND_INTERNAL_FUNCTION) + && zend_ast_is_list(args_ast) + && !zend_args_contain_unpack_or_named(zend_ast_get_list(args_ast))) { + if (zend_compile_frameless_icall(result, zend_ast_get_list(args_ast), fbc, type) != (uint32_t)-1) { + zval_ptr_dtor(&method_node.u.constant); + if (class_node.op_type == IS_CONST) { + zval_ptr_dtor(&class_node.u.constant); + } + return; + } + } + } + } + opline = get_next_op(); opline->opcode = ZEND_INIT_STATIC_METHOD_CALL; @@ -5346,31 +5387,6 @@ static void zend_compile_static_call(znode *result, zend_ast *ast, uint32_t type SET_NODE(opline->op2, &method_node); } - /* Check if we already know which method we're calling */ - if (opline->op2_type == IS_CONST) { - zend_class_entry *ce = NULL; - if (opline->op1_type == IS_CONST) { - zend_string *lcname = Z_STR_P(CT_CONSTANT(opline->op1) + 1); - ce = zend_hash_find_ptr(CG(class_table), lcname); - if (ce) { - if (zend_compile_ignore_class(ce, CG(active_op_array)->filename)) { - ce = NULL; - } - } else if (CG(active_class_entry) - && zend_string_equals_ci(CG(active_class_entry)->name, lcname)) { - ce = CG(active_class_entry); - } - } else if (opline->op1_type == IS_UNUSED - && (opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF - && zend_is_scope_known()) { - ce = CG(active_class_entry); - } - if (ce) { - zend_string *lcname = Z_STR_P(CT_CONSTANT(opline->op2) + 1); - fbc = zend_get_compatible_func_or_null(ce, lcname); - } - } - zend_compile_call_common(result, args_ast, fbc, zend_ast_get_lineno(method_ast)); } /* }}} */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! The code looks good to me and I pushed it. It makes reviewing slightly harder.
afa4e02 to
640c7e0
Compare
@dstogov Have you ever had time to check whether this makes a meaningful difference for your FFI case? In the synthetic benchmark, it shows a 29% improvement compared to normal static method calls.
Sorry, I missed this. I'll take a careful look and do benchmarks later this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the implementation.
Think about the alternative zend_compile.c patch.
I'm not sure what version is better.
May be lc_name(s) may be cached to avoid the double work or may be it's better to keep the sources clean (speed of compiler is not very important).
Zend/zend_compile.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It would be better to check for frame-less variant before emitting INIT_STATIC_METHOD_CALL
This may be done with the alternative following patch.
The only drawback is that zend_string_tolower() called twice times more.
diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 7e5351ddfaf..c479d97b27a 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -5329,6 +5329,47 @@ static void zend_compile_static_call(znode *result, zend_ast *ast, uint32_t type } } + /* Check if we already know which method we're calling */ + if (method_node.op_type == IS_CONST) { + zend_class_entry *ce = NULL; + if (class_node.op_type == IS_CONST) { + zend_string *lcname = zend_string_tolower(Z_STR(class_node.u.constant)); + ce = zend_hash_find_ptr(CG(class_table), lcname); + if (ce) { + if (zend_compile_ignore_class(ce, CG(active_op_array)->filename)) { + ce = NULL; + } + } else if (CG(active_class_entry) + && zend_string_equals_ci(CG(active_class_entry)->name, lcname)) { + ce = CG(active_class_entry); + } + zend_string_release(lcname); + } else if (class_node.op_type == IS_UNUSED + && (class_node.u.op.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF + && zend_is_scope_known()) { + ce = CG(active_class_entry); + } + if (ce) { + zend_string *lcname = zend_string_tolower(Z_STR(method_node.u.constant)); + fbc = zend_get_compatible_func_or_null(ce, lcname); + zend_string_release(lcname); + + if (fbc + && !(CG(compiler_options) & ZEND_COMPILE_NO_BUILTINS) + && (fbc->type == ZEND_INTERNAL_FUNCTION) + && zend_ast_is_list(args_ast) + && !zend_args_contain_unpack_or_named(zend_ast_get_list(args_ast))) { + if (zend_compile_frameless_icall(result, zend_ast_get_list(args_ast), fbc, type) != (uint32_t)-1) { + zval_ptr_dtor(&method_node.u.constant); + if (class_node.op_type == IS_CONST) { + zval_ptr_dtor(&class_node.u.constant); + } + return; + } + } + } + } + opline = get_next_op(); opline->opcode = ZEND_INIT_STATIC_METHOD_CALL; @@ -5346,31 +5387,6 @@ static void zend_compile_static_call(znode *result, zend_ast *ast, uint32_t type SET_NODE(opline->op2, &method_node); } - /* Check if we already know which method we're calling */ - if (opline->op2_type == IS_CONST) { - zend_class_entry *ce = NULL; - if (opline->op1_type == IS_CONST) { - zend_string *lcname = Z_STR_P(CT_CONSTANT(opline->op1) + 1); - ce = zend_hash_find_ptr(CG(class_table), lcname); - if (ce) { - if (zend_compile_ignore_class(ce, CG(active_op_array)->filename)) { - ce = NULL; - } - } else if (CG(active_class_entry) - && zend_string_equals_ci(CG(active_class_entry)->name, lcname)) { - ce = CG(active_class_entry); - } - } else if (opline->op1_type == IS_UNUSED - && (opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF - && zend_is_scope_known()) { - ce = CG(active_class_entry); - } - if (ce) { - zend_string *lcname = Z_STR_P(CT_CONSTANT(opline->op2) + 1); - fbc = zend_get_compatible_func_or_null(ce, lcname); - } - } - zend_compile_call_common(result, args_ast, fbc, zend_ast_get_lineno(method_ast)); } /* }}} */
640c7e0 to
466e7a4
Compare
There is an issue on ARM (or possibly the configuration we're using for that build). I'll have a look at that.
Co-authored-by: Dmitry Stogov <dmitry@zend.com>
466e7a4 to
ba9092a
Compare
Uh oh!
There was an error while loading. Please reload this page.
The very synthetic
_ZendTestClass::framelessStaticMethod()shows a significant improvement, iterated 100 000 000 times.Obviously, this difference will be more noticeable if the implementation is simple, as in this case. The difference for useful methods will be smaller.