roots / trellis

WordPress LEMP stack with PHP 8.2, Composer, WP-CLI and more
https://roots.io/trellis/
MIT License
2.51k stars 607 forks source link

Add JIT options #1505

Closed strarsis closed 1 year ago

strarsis commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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: https://github.com/roots/trellis/pull/1505/commits/0eba0dc2b0bf95572f3660396a20d8f596795226#diff-5fe3b7ecf9bfc04c7b32d532cb0c877aa59f67457d4c7f5725c40655668f081bR37

swalkinshaw commented 1 year ago

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 commented 1 year ago

Thanks @strarsis 🎉