feat: disable JIT debugging by default#28
feat: disable JIT debugging by default#28carlos-granados wants to merge 3 commits intophp-debugger:mainfrom
Conversation
1ed4f0a to
d5e2d46
Compare
d5e2d46 to
39ff765
Compare
|
@carlos-granados I wonder why benchmarks workflow didnt run on this one? |
|
@pronskiy They had run but not for the latest version. They run when you tag the PR with the Looking at the results, there's something that I can't understand. The JIT version of the Symfony runs is faster than the non-JIT. That should not be the case because we are not setting observer_active to false and we are setting the ZEND_COMPILE_EXTENDED_STMT flag, so it should be much slower, which is what we are seeing for the bench and rector runs. I can't explain this, there must be something weird going on. Let me see if I can figure it out |
9dd996c to
74b55f6
Compare
5ca5ce0 to
c674431
Compare
229521f to
41cc9a2
Compare
|
@pronskiy ok, I've finally understood what was going on. The issue is that for the cases that we were using to test performance, disabling the opcache optimizer actually improves performance as the code does not have to spend the time optimizing the opcache. In the end I think that opcache optimization is not relevant for our benchmark results, so I have disabled it everywhere and now we get coherent results. This is the latest benchmark run: https://github.com/php-debugger/php-debugger/actions/runs/23696091215 |
xdebug.c
Outdated
| @@ -688,10 +697,6 @@ | |||
| xdebug_control_socket_dispatch(); | |||
There was a problem hiding this comment.
xdebug_control_socket_dispatch(); would never be reached then, wouldn't it?
ctrl_socket dispatch should happen regardless as i understand
There was a problem hiding this comment.
Yes, you are right, I had been testing some optimizations and moved this and forgot to move it back
| xdebug_library_zend_shutdown(); | ||
| } | ||
|
|
||
| ZEND_DLEXPORT void xdebug_init_oparray(zend_op_array *op_array) |
There was a problem hiding this comment.
This is unrelated cleanup, isn't it? Maybe move to a separate PR for clarity?
There was a problem hiding this comment.
OK, will move to a separate PR
| RETURN_FALSE_IF_MODE_IS_NOT(XDEBUG_MODE_STEP_DEBUG); | ||
|
|
||
| if (!xdebug_is_debug_connection_active() && !XINI_DBG(jit_debugging_enabled)) { | ||
| RETURN_FALSE; |
There was a problem hiding this comment.
Maybe add logging?
xdebug_log_ex(XLOG_CHAN_DEBUG, XLOG_INFO, "JIT-OFF",
"xdebug_break() ignored: no active debug connection and "
"xdebug.jit_debugging_enabled is not enabled");
php_error(E_NOTICE,
"xdebug_break(): no active debug session and JIT debugging is disabled. "
"Set xdebug.jit_debugging_enabled=1 to enable mid-request debugging");
or something along the lines
| RETURN_FALSE_IF_MODE_IS_NOT(XDEBUG_MODE_STEP_DEBUG); | ||
|
|
||
| if (!XINI_DBG(jit_debugging_enabled)) { | ||
| RETURN_FALSE; |
|
"JIT" in PHP context exclusively means the JIT compiler. Calling this feature "JIT debugging" will confuse users: "does this control debugging the JIT? debugging with JIT enabled?". I was confused too. How about something like "deferred_debugging" or "late_attach" or "on_demand_debugging" instead? The README section is well-written but the name itself creates ambiguity |
OK, let me think of something else 😄 |
|
@pronskiy I decided to use on-demand, updated |
# Conflicts: # .github/benchmark-files/benchmark.ini # Conflicts: # README.md
374cfa9 to
b7bf398
Compare
This PR adds a new
jit_debugging_enabledini setting that allows you to disable some of the optimizations that we have added. When this is enabled you will be able to do JIT debugging even if you don't initially connect to any client by starting a connection using thexdebug_connect_to_clientandxdebug_breakfunctions or when an error or exception is raised. If you enable it then the performance gain is much smaller, instead of improving performance by 97% you will improve it by 60% (not bad in any case)With this disabled I wanted to be able to disable even more operations so that we could save even more time but I found out that the only significant thing that we could add was not disabling the opcache optimizer, the other main source of overhead is the addition of the observer functions but this needs to be set up at the module initialization level, so we cannot actually disable it for each request depending on whether we connect or not
And the opcache optimizer is really only relevant for cgi/fpm processes, not cli processes, as opcache is not really useful for single use CLI commands and is usually disabled there, so I did not manage to really improve the current optimization by much
But what I was able to do is to get the vast majority of tests which had been marked as XFAIL to pass by setting this ini setting to enabled. There are still a couple which are still failing and where I need to take a deeper look. I also managed to get a few of the tests which had been skipped to pass by using this new setting or by changing some basic settings of the test setup