Add JIT guards for INIT_METHOD_CALL when the method may be modified #8600
Conversation
| } | ||
|
|
||
| if (!func | ||
| if ((!func || zend_jit_may_be_modified(func, op_array)) |
There was a problem hiding this comment.
func here comes from static analysis / zend optimizer. In my understanding a non-null func here is an indication that the call is non polymorphic, since the function could be resolved statically. A guard was not added in this case, with the assumption that func could not change. This assumption doesn't hold true across multiple requests.
| const void *exit_addr; | ||
|
|
||
| exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_METHOD_CALL); | ||
| exit_point = zend_jit_trace_get_exit_point(opline, func ? ZEND_JIT_EXIT_INVALIDATE : ZEND_JIT_EXIT_METHOD_CALL); |
There was a problem hiding this comment.
In my understanding, ZEND_JIT_EXIT_METHOD_CALL is designed for polymorphic calls. It fallbacks to the VM, but the next loop iterations will execute this trace again for a while before the jit engine decides to generate a new/side trace.
ZEND_JIT_EXIT_INVALIDATE invalidates the trace immediately.
func being non-null here indicates that this is not a polymorphic call
There was a problem hiding this comment.
For non-polimorphic calls function guard might be checked once per request, so it would be better to do this right after zend_jit_find_method_helper(). I may optimize this on top of the PR.
dstogov
left a comment
There was a problem hiding this comment.
Looks good. An optimization of the guard is possible, but it may be done separately.
BTW I afraid, the problem may be more complex, because __construct() is also modified on second request.
| const void *exit_addr; | ||
|
|
||
| exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_METHOD_CALL); | ||
| exit_point = zend_jit_trace_get_exit_point(opline, func ? ZEND_JIT_EXIT_INVALIDATE : ZEND_JIT_EXIT_METHOD_CALL); |
There was a problem hiding this comment.
For non-polimorphic calls function guard might be checked once per request, so it would be better to do this right after zend_jit_find_method_helper(). I may optimize this on top of the PR.
|
@dstogov Thank you for the review
I will look at this on friday or during the weekend. But I don't mind if you merge the PR before that, if you plan to work on this or the optimization. |
Non-polymorphic methods can be modified from one request to an other due to recompilation or conditional declaration. Co-authored-by: Oleg Stepanischev <[email protected]>
|
The __construct issue is really a separate one, I will handle it separately (#8642). I'm merging this now so that you can handle the optimization. |
* PHP-8.1: [ci skip] NEWS Add JIT guards for INIT_METHOD_CALL when the method may be modified (#8600)
| @@ -0,0 +1,41 @@ | |||
| --TEST-- | |||
| Bug GH-8591 001 (JIT does not account for class re-compile) | |||
There was a problem hiding this comment.
looks like a typo in test name
There was a problem hiding this comment.
Oh, thank you. I will update it.
Non-polymorphic methods can be modified from one request to an other due to recompilation or conditional declaration.
Fixes GH-8591