Implement JIT for ZEND_FETCH_STATIC_PROP_* and improve interpretation #16157
Conversation
Zend/zend_execute.c
Outdated
| } | ||
| if (EXPECTED(op1_type == IS_CONST) && EXPECTED(CACHED_PTR(cache_slot) == ce)) { | ||
| *retval = CACHED_PTR(cache_slot + sizeof(void *)); | ||
| if (EXPECTED(op1_type == IS_CONST) && EXPECTED(CACHED_PTR(cache_slot + sizeof(void *)) == ce)) { |
There was a problem hiding this comment.
It doesn't seem right that you add sizeof(void *) here. CACHE_POLYMORPHIC_PTR takes offset 0 to the cache_slot for ce. The result is stored in offset sizeof(void *) instead of ce.
There was a problem hiding this comment.
It doesn't seem right that you add
sizeof(void *)here.CACHE_POLYMORPHIC_PTRtakes offset 0 to thecache_slotfor ce. Theresultis stored in offsetsizeof(void *)instead ofce.
You are right! Thanks!
|
I ran a benchmark (with the instruction count metric enabled) for this branch against the current master: https://gist.github.com/kocsismate/30d5e043d412f19fbebf7bc8f3fa16b8 |
Thanks for benchmarking. FETCH_STATIC_PROP is not very often used in real-life apps, so the average speed-up is less than a percent. On a syntactic benchmark the speed-up is really good.6.7% I started to look into FETCH_STATIC_PROP speed now, because it is very useful in FFI code. |
|
@dstogov I don't think you meant to close this? |
I pressed to "close" by accident :) |
|
Can't blame you 😉 GH is not great at UI. |
Zend/zend_execute.c
Outdated
| return result; | ||
| } | ||
|
|
||
| ZEND_API zval* zend_fetch_static_property(zend_execute_data *ex, int fetch_type) { |
There was a problem hiding this comment.
Should this be marked with ZEND_FASTCALL since it is called from JIT?
There was a problem hiding this comment.
yeah. It makes sense to switch to FASTCALL. This will allow to pass parameters in register in 32-bit x86 build.
I don't care about 32-bit x86 a lot.
| 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); | ||
| if (ce && (ce->type == ZEND_INTERNAL_CLASS || ce->info.user.filename != op_array->filename)) { |
There was a problem hiding this comment.
Are we skipping internal classes only because of Windows? Not that it matters, we don't have many static properties in internal classes.
There was a problem hiding this comment.
Your question make me think about the preloaded user classes...
I'll think how to improve this condition.
There was a problem hiding this comment.
I decided to commit this without changes, because this is consistent with regular properties.
Both should be improved later to support preloaded classes and may be internal ones.
ext/opcache/jit/zend_jit_ir.c
Outdated
| fetch_type = BP_VAR_UNSET; | ||
| break; | ||
| default: | ||
| ZEND_UNREACHABLE(); |
There was a problem hiding this comment.
Nit: EMPTY_SWITCH_DEFAULT_CASE().
| ir_CONST_U32(_ZEND_TYPE_MASK))); | ||
| ir_IF_FALSE(if_typed); | ||
| ir_END_list(merge); | ||
| ir_IF_TRUE(if_typed); |
There was a problem hiding this comment.
Since this is the cold-path, should we move the loading of the property info to zend_jit_uninit_static_prop()? That would reduce code size a bit.
| goto done; | ||
| case ZEND_FETCH_STATIC_PROP_FUNC_ARG: | ||
| if (!JIT_G(current_frame) | ||
| || !JIT_G(current_frame)->call |
There was a problem hiding this comment.
Question: looking at similar code there is also a check for !JIT_G(current_frame)->call->func, is that missing here?
There was a problem hiding this comment.
These func check seem not necessary.
JIT/FFI patch is going to remove them, because it adds an ability to inline FFI calls implemented through trampolines (without known func).
Here I didn't add the check in the first place.
|
@nielsdos @iluuu1994 Thank you very much! Your feedback was very helpful. |
4 participants
Execution of ZEND_FETCH_STATIC_PROP_R_SPEC_HANDLER in interpreter now requires ~35% less CPU instructions (according to callgrind).
JIT eliminates ~80% of CPU instructions for each ZEND_FETCH_STATIC_PROP_R opcode.