-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Provide script to TSSA build in tracing JIT #18353
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
base: master
Are you sure you want to change the base?
Conversation
9b8e160
to
ee732d3
Compare
For the following script: ```php final class Foo { public $prop = 0; } function test(Foo $obj) { $obj->prop=1; } $foo = new Foo; for ($i=0;$i<3;$i++) { test($foo); } ``` When comparing the TSSA (via opcache.jit_debug) vs the opcache SSA (via opcache.opt_debug_level=0x400000) we note that in TSSA, the RECV op for `test` does not infer the type of the argument to be class `Foo`. This is because the optimizer uses the `script` pointer to figure out known classes but TSSA always sets `script` to NULL. This in turn generates suboptimal assembly because `zend_may_throw` returns 1 due to the unknown CE in the TSSA, resulting in an extra exception check in the assembly code.
@@ -4117,6 +4117,14 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par | |||
|
|||
checkpoint = zend_arena_checkpoint(CG(arena)); | |||
|
|||
zend_accel_hash_entry *accel_h_entry = zend_accel_hash_find_entry(&ZCSG(hash), trace_buffer->op_array->filename); |
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.
Possibly this can fetch a different version of the script (e.g. if the file was discarded and recompiled).
An alternative would be to store the script into zend_jit_op_array_trace_extension
. (After op_array
, as I believe the func_info
and op_array
fields must be at the same offset in all zend_jit_*_extension
structs.)
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.
So simple? I wonder, why I didn't use SHM stored script
before. (I can't remember any related problems).
This also relates to the fact that for tacing JIT we use opcache.jit=1254
instead of 1255
, but in case we can use the SHM stored version of the script , we may consider switching to 1255
by default. @nielsdos Please double check this.
@arnaud-lb script caching and trace compilations are serialized through the same mutex. The only possible race condition I see, is script modification, between start of tracing start and start of trace compilation. That mean the new compiled trace is going to be already outdated. I think this should be fixed, by adding some check right after zend_shared_alloc_lock
in zend_jit_compile_root/side_trace()
. Then this solution should be fine. Do you see other problems?
@dstogov an other possible race condition is script modification while it's being executed (start executing script, re-compile script, start tracing) : // main.php
require 'inc.php';
opcache_invalidate('inc.php', true);
require 'inc.php';
f(); // inc.php
if (!function_exists('f')) {
function f() {
for ($i = 0; $i < 200; $i++) {
var_dump($i);
}
}
} In this case we start tracing the loop when the script is already invalidated. I think your solution will handle this too, but we could avoid tracing entirely by adding a check earlier. |
Right! The early check is preferred. |
The last check may be done using address range comparison. |
For the following script:
When comparing the TSSA (via opcache.jit_debug) vs the opcache SSA (via opcache.opt_debug_level=0x400000) we note that in TSSA, the RECV op for
test
does not infer the type of the argument to be classFoo
. This is because the optimizer uses thescript
pointer to figure out known classes but TSSA always setsscript
to NULL. This in turn generates suboptimal assembly becausezend_may_throw
returns 1 due to the unknown CE in the TSSA, resulting in an extra exception check in the assembly code.