Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented May 2, 2020

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

Immediate future todos:

  • We now have a bool opcache.jit and the "implicit" flag opcache.jit_buffer_size which is 0 by default and leads to disabling the JIT. Maybe we should turn these around and set opcache.jit=0 and opcache.jit_buffer_size=16M (or whatever is a good default).
  • The default for opcache.jit_trigger should be changed to 3 (profile and jit hot functions only). This is suggested in the RFC as well.

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
@beberlei
Copy link
Contributor Author

beberlei commented May 2, 2020

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 +
Copy link
Contributor

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)

@nikic
Copy link
Member

nikic commented May 2, 2020

* We now have a bool `opcache.jit` and the "implicit" flag `opcache.jit_buffer_size` which is 0 by default and leads to disabling the JIT. Maybe we should turn these around and set `opcache.jit=0` and `opcache.jit_buffer_size=16M` (or whatever is a good default).

Maybe drop opcache.jit entirely? opcache.jit_buffer_size alone is sufficient to control whether jit is enabled or not.

@TysonAndre
Copy link
Contributor

Maybe drop opcache.jit entirely? opcache.jit_buffer_size alone is sufficient to control whether jit is enabled or not.

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 opcache.memory_consumption or a hardcoded value).

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 opcache.memory_consumption changed from 64MB to 128MB in PHP 7.0.

@beberlei
Copy link
Contributor Author

beberlei commented May 2, 2020

I agree with @TysonAndre here. Implicit settings are confusing, and the precedent here with opcache.enable(_cli) and opcache.max_memory_consumption is also something to honor.

@dragoonis
Copy link
Contributor

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.

  • opcache.jit = 1
    or
  • opcache.jit.enabled = 1

I'm happy to amend the code to reflect the latter if we agree on that approach.

@iluuu1994
Copy link
Member

I agree with @TysonAndre. The average user won't know what to set jit_buffer_size to so it should be set to a sensible default value.

@dstogov
Copy link
Member

dstogov commented May 13, 2020

@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.

@beberlei
Copy link
Contributor Author

@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 opcache.enable_jit boolean defaulting to 0 and instead a pre-defined buffer size of $x?

As for <<Opcache\Jit>> i did this prototype here beberlei#7 with a little behavior change I think would be great where Opcache\Jit always takes precedence over any other trigger. I think this would be more powerful than making it its only available with its own trigger mode, because users would have a way of "hinting" the JIT when its running in "hot mode", similar how in databases you can FORCE INDEX to work around the query planer/optimizer.

@dstogov
Copy link
Member

dstogov commented May 14, 2020

@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
opcache.jit=disabled ; disabled and can't be enabled at run-time (may be set only at INI_SYSTEM)
opcache.jit=off ; temporary disabled
opcache.jit=on ; enabled (default trigger)
opcache.jit=load ; JIT all functions when compile php file (trigger=0)
opcache.jit=used ; JIT functions on first usage (trigger=1)
opcache.jit=hot ; JIT functions after they called N times (trigger=3)
opcache.jit=trace ; JIT hot traces (trigger=5)

; trigger=2 doesn't fit into this design :(
; trigger=4 shuold be replaced by attributes

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"
; a string parsed by jit extension (INI_ALL)
; it's possible to have many different options. some of them can't be changed at run-time.

opcache.jit_debug - become INI_ALL (may be converted to string)

Then we may have PHP attributes that override the default
<<JIT>>
<<JIT, "off">>
<<JIT, "trace">>
<<JIT, "on", "O3">>

@dragoonis
Copy link
Contributor

dragoonis commented May 14, 2020

@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

@dstogov
Copy link
Member

dstogov commented May 14, 2020

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"

@dstogov
Copy link
Member

dstogov commented May 14, 2020

@nikic see above, what do you think?

@iluuu1994
Copy link
Member

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"

Only if you can still do opcache.jit=on as most people (myself included) don't know what those flags actually mean.

@beberlei
Copy link
Contributor Author

Closing in favor of steps in #5580, may need a full rewrite after this PR.

@beberlei beberlei closed this May 15, 2020
@beberlei
Copy link
Contributor Author

beberlei commented Jul 25, 2020

@dstogov we have mid july now and no changes in this regard, do you have the time to do this before feature freeze?

  1. ini settings
  2. attributes

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:

  1. disable jit for a specific op_array, even though for example jit is triggered on load.
  2. force enable jit for a specific op_array, even though jit is set to tracing/hot
  3. change jit setting for a specific op_array from the default tracing to hot

If I undrestand that correctly, then the trigger "attribute" means "Only JIT op_arrays that have an @@Jit attribute declared that is not itself disabling the JIT".

Example:

// opcache.jit=1245 means trigger=attribute

@@Jit
function foo () { } // this one gets jitted immediately on load

@@Jit("off")
function bar() { } // this does not get jitted, because its individual setting is off

@@Jit("trace")
function baz() { } // this gets jitted, but only if tracing jit decides to

@dstogov
Copy link
Member

dstogov commented Jul 27, 2020

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.

@@Jit("off") - disable jit for this function (and its invocation from traces)

@@Jit("function") - JIT on load (1205)  (you may think about better name)

@@Jit("trace") - setup counters for tracing JIT (1254)

It's also possible to add everything/trace for opcahce.jit directive.
I wouldn't add "human settings" for less useful options: -On, -avx, and less useful triggers.

@nikic may be you have some related thoughts?

If you can lead the implementation, that would be great.
I may help with some details (e.g. handling "off" for tracing JIT).

@beberlei
Copy link
Contributor Author

@dstogov i have #5913 for the ini improvements, but I need some feedback on attributes.

The case for @@jit("off") is trivial to do early in zend_jit_op_array, so that is no problem.

But what should the others do, depending on what state the JIT is in? Wouldn't it make more sense to have @@Jit and @@Jit("on") that immediately JIT the code, independent of what trigger the JIT is in?

Because @Jit("function") only makes sense from my understanding, when the JIT is not already in "function" mode. And @Jit("traceing") only makes sense when JIT is not already in "tracing" mode. But why should code know such low level runtime details?

Or would these attributes only be read when trigger=ATTRIBUTE (4) and ignored for all other triggers?

@beberlei
Copy link
Contributor Author

Jit Attribute Support: #5915

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants