Skip to content
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

Add JIT options #1505

Merged
merged 5 commits into from Nov 16, 2023
Merged

Add JIT options #1505

merged 5 commits into from Nov 16, 2023

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Oct 1, 2023

This PR adds PHP 8 OPcache JIT related options minimally required for (optionally) enabling JIT.

The Ubuntu default configuration file /etc/php/<php_version>/fpm/conf.d/10-opcache.ini disables JIT (by explicitly setting it to off), hence the configuration file is controlled by a template to allow enabling or disabling JIT.
The default value for php_opcache_jit (corresponding ansible variable for opcache.jit) is Off tracing (which enables JIT by default). (Note: Other option values available: disable (like off, but prevents changing option on PHP runtime; on (enables JIT; aliases to tracing); tracing (currently the same as on); function.)

In order to enable JIT, the value opcache.jit_buffer_size (corresponding ansible variable php_opcache_jit_buffer_size) option also has to be set to something different than its default 0, as 0 also disables JIT. 0 is used as the default for php_opcache_jit_buffer_size. (Note: A reasonable, often proposed size for opcache.jit_buffer_size would be 256M.)

🤔 when someone manually edited that /etc/php/<php_version>/fpm/conf.d/10-opcache.ini file, it would be overwritten by Ansible of course, as it is now managed by Ansible. Add warning, skip when edited or something else in that case?

@swalkinshaw
Copy link
Member

Add warning, skip when edited or something else in that case?

🤔 interesting!

That's a good question.. but I'm tempted to do nothing since that could also apply to lots of other configuration files that Trellis has never handled in any special way?

What do you think about making tracing the default? I can't think of a reason not to enable it by default.

@strarsis
Copy link
Contributor Author

strarsis commented Oct 4, 2023

That's a good question.. but I'm tempted to do nothing since that could also apply to lots of other configuration files that Trellis has never handled in any special way?

Yes, a release or changelog note about this file now being managed by Ansible should suffice. When merging in new changes from upstream Trellis and following good/best practices one should check and notice this anyway.

What do you think about making tracing the default? I can't think of a reason not to enable it by default.

There appears to be no downside to enabling JIT, and one can always disable it via an ansible option
(the option default in the PR is set to tracing now).

@swalkinshaw
Copy link
Member

Just testing this out and it's working fine. But if we want it enabled by default, don't we need php_opcache_jit_buffer_size set to a non-zero value? For reference, the default opcache memory consumption default is 128M both in Trellis and in PHP.

@strarsis
Copy link
Contributor Author

@swalkinshaw: Indeed, jit_buffer_size must be non-zero for enabling JIT.
I set it to 256M which appears to be a sensible default in conjunction with the other default settings:
0eba0dc#diff-5fe3b7ecf9bfc04c7b32d532cb0c877aa59f67457d4c7f5725c40655668f081bR37

@swalkinshaw
Copy link
Member

I realized that this wasn't conditional for PHP >=8 so I was curious what would happen on 7.4 .... and the answer is thankfully nothing? PHP just ignores the jit variables seemingly since I get no errors or warnings and PHP is working as expected.

@swalkinshaw swalkinshaw merged commit ff74ca2 into roots:master Nov 16, 2023
2 checks passed
@swalkinshaw
Copy link
Member

Thanks @strarsis 🎉

@strarsis strarsis deleted the add-opcache-jit-options branch November 16, 2023 00:47
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.

None yet

2 participants