-
Notifications
You must be signed in to change notification settings - Fork 7.9k
JIT for INIT_STATIC_METHOD_CALL #16206
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
This PR brings around 0.88% perf improvement for Laravel. The rest of the tests are very similar as before. |
ext/opcache/jit/zend_jit_helpers.c
Outdated
return fbc; | ||
} | ||
|
||
static zend_execute_data* ZEND_FASTCALL zend_jit_push_this_metod_call_frame(zend_class_entry *scope, zend_function *fbc, uint32_t num_args) |
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.
Typo: metod -> method
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.
Thanks.
ext/opcache/jit/zend_jit_ir.c
Outdated
|
||
ZEND_ASSERT(Z_TYPE_P(zv) == IS_STRING); | ||
class_name = Z_STR_P(zv); | ||
ce = zend_lookup_class_ex(class_name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD); |
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.
Would it be beneficial to use the runtime cache to save some compilation time? In the tracing JIT the cache is probably primed. We could also use it as profiling data in some cases.
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.
This may be a good idea, but I would prefer to land this patch as is and than think about data-profiling improvement.
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.
Of course! This was just a thought I had while looking the change, but I agree it's not within the scope of this PR
ext/opcache/jit/zend_jit_ir.c
Outdated
if (op_array->fn_flags & ZEND_ACC_STATIC) { | ||
scope_ref = ir_LOAD_A(jit_EX(This.value.ref)); | ||
} else { | ||
scope_ref = ir_LOAD_A(ir_ADD_OFFSET(jit_EX(This.value.ref), offsetof(zend_object, ce))); |
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.
scope_ref = ir_LOAD_A(ir_ADD_OFFSET(jit_EX(This.value.ref), offsetof(zend_object, ce))); | |
scope_ref = ir_LOAD_A(ir_ADD_OFFSET(ir_LOAD_A(jit_EX(This.value.ref)), offsetof(zend_object, ce))); |
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.
Thanks! You are right.
ext/opcache/jit/zend_jit_ir.c
Outdated
} | ||
} | ||
|
||
if (opline->op1_type == IS_CONST) { |
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.
This check with the "then" and "else" branch are almost identically the same to what happens in zend_jit_fetch_static_prop
, except for the op number. Perhaps this can be factored out to a separate method?
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.
Thanks! Done!
No description provided.