-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[jit] Split up opcache.jit bitmask into four distinct INI settings. #5510
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
To improve developer experience the opcache.jit bitmask was split up into these four settings: - opcache.jit_optimization_level=5 - opcache.jit_trigger=0 - opcache.jit_register_allocation=2 - opcache.jit_cpu_flags=1
This needs some more changes, have not found all the places before |
@@ -3010,10 +3010,15 @@ static int accel_post_startup(void) | |||
|
|||
zend_shared_alloc_lock(); | |||
#ifdef HAVE_JIT | |||
zend_long jit_flags = ZCG(accel_directives).jit_optimization_level * 1 + |
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.
I think this should emit a notice or startup error if any of the individual flags is out of range. This is somewhat of a pre-existing issue, the jit didn't warn before.
For example, ZCG(accel_directives).jit_optimization_level < 0 || ZCG(accel_directives).jit_optimization_level > 5
should emit a notice along the lines of opcache.jit_optimization should be an integer between 0 and 5, but got %d
(I'd agree that the opcache.jit bitmask would be better off as individual settings and this is an improvement, especially since there was previously no way to change just one of these)
Maybe drop |
I think it would be more user-friendly to have a flag to enable or disable the jit, and have opcache give the user reasonable defaults for how much memory is devoted to the jit (e.g. based on That way, new php releases could change those defaults if the JIT changed or if typical php applications became larger. For example, https://www.php.net/manual/en/opcache.configuration.php#opcache.configuration |
I agree with @TysonAndre here. Implicit settings are confusing, and the precedent here with |
I third this suggestion. Developer Experience is paramount here. @nikic Imagine if we removed opcache.enabled and just turned opcache on based on a buffer size? Nightmare! The same DX applies here. Explicitness always wins over implicitness. Things to consider.
I'm happy to amend the code to reflect the latter if we agree on that approach. |
I agree with @TysonAndre. The average user won't know what to set |
@beberlei thanks for this PR. I'm, in general, agree that opcache.jit setting should be split or improved in some way. However, this should be done together with few other settings, that are hard-coded for now (hotnes-counters). I would also try to use PHP attributes for enabling/disabling JIT for particular functions instead of current doc-comment trigger. This is in my TODO list, and I plan to finalize this by the mid of June. |
@dstogov If you think this PR is a good start that you would merge, just let me know what else you think should be configured and I take it off your plate. What do you think about As for |
@beberlei, I don't think this should be merged now. Actually opcache.jit_trigger=N is not much better than opcache.jit=ABCD. See the outline of my thoughts: opcacge.jit - become INI_ALL ; trigger=2 doesn't fit into this design :( opcache.jit_buffer_size=N; INI_SYSTEM with default non-zero value opcache.jit_options="O5 +avx hot_func=127 hot_loop=64 bisect_limit=0" opcache.jit_debug - become INI_ALL (may be converted to string) Then we may have PHP attributes that override the default |
@dstogov - you're right, jit_trigger with a numerical value isn't much better, and has a lower DX value. Strings beat Numbers here because we're humans. load, used, hot, trace are all good names which, if well documented, will be useful to the community |
May be even better to merge opcache.jit with opcache.jit_options back into single option, just make it readable. opcache.jit="trace, O4 +avx" |
@nikic see above, what do you think? |
Only if you can still do |
Closing in favor of steps in #5580, may need a full rewrite after this PR. |
@dstogov we have mid july now and no changes in this regard, do you have the time to do this before feature freeze?
otherwise, please give me guidance on how to proceed at least with attributes. As far as I understand your comment, it is now possible to have the settings different for every op_array, as such you have the following use cases with attributes:
If I undrestand that correctly, then the trigger "attribute" means "Only JIT op_arrays that have an Example:
|
Hi @beberlei, I do all my best to stabilize the tracing JIT. It is the default JIT method now. The @@jit attribute looks fine for me. I would propose the following setting.
It's also possible to add @nikic may be you have some related thoughts? If you can lead the implementation, that would be great. |
@dstogov i have #5913 for the ini improvements, but I need some feedback on attributes. The case for But what should the others do, depending on what state the JIT is in? Wouldn't it make more sense to have Because Or would these attributes only be read when trigger=ATTRIBUTE (4) and ignored for all other triggers? |
Jit Attribute Support: #5915 |
To improve developer experience the opcache.jit bitmask was split up into these four settings:
Immediate future todos:
opcache.jit
and the "implicit" flagopcache.jit_buffer_size
which is 0 by default and leads to disabling the JIT. Maybe we should turn these around and setopcache.jit=0
andopcache.jit_buffer_size=16M
(or whatever is a good default).opcache.jit_trigger
should be changed to 3 (profile and jit hot functions only). This is suggested in the RFC as well.