-
Notifications
You must be signed in to change notification settings - Fork 2k
Enable opcache by default #1587
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
Enable opcache by default #1587
Conversation
OPcache is an essential part of modern PHP and will always be included in PHP starting with 8.5 (https://wiki.php.net/rfc/make_opcache_required). Enable OPcache by default in Docker to simplify the set-up.
@@ -126,7 +126,7 @@ for ext in $exts; do | |||
find modules \ | |||
-maxdepth 1 \ | |||
-name '*.so' \ | |||
-exec basename '{}' ';' \ | |||
-exec basename '{}' '.so' ';' \ |
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.
Can you elaborate on this (seemingly unrelated) change? The .so
vs not-.so
is already handled in docker-php-ext-enable
unless I've missed something.
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.
Without this change, this would call docker-php-ext-enable opcache.so
. If the INI already has zend_extension=opcache
then for the docker-php-ext-enable opcache.so
call neither this:
Lines 96 to 103 in 85abdb7
if php -d 'display_errors=stderr' -r 'exit(extension_loaded("'"$ext"'") ? 0 : 1);'; then | |
# this isn't perfect, but it's better than nothing | |
# (for example, 'opcache.so' presents inside PHP as 'Zend OPcache', not 'opcache') | |
echo >&2 | |
echo >&2 "warning: $ext ($module) is already loaded!" | |
echo >&2 | |
continue | |
fi |
nor this:
Line 114 in 85abdb7
if ! grep -qFx -e "$line" -e "$line.so" "$ini" 2>/dev/null; then |
check trigger and as a result the the INI will be updated to:
zend_extension=opcache
zend_extension=opcache.so
My personal experience with opcache has been primarily negative, so this certainly causes me some pause, but that experience is admittedly really outdated now and this is probably sane (especially as it'll be the effective state in 8.5). |
Changes: - docker-library/php@33c1de7e: Merge pull request docker-library/php#1587 from TimWolla/opcache-enable-by-default - docker-library/php@2a526901: Enable opcache by default
{{ if rcVersion | IN("8.1", "8.2", "8.3", "8.4") then ( -}} | ||
# enable OPcache by default (https://wiki.php.net/rfc/make_opcache_required) | ||
RUN docker-php-ext-enable opcache | ||
{{ ) end -}} |
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.
Gah, if
without else
isn't supported until jq
1.7 (https://github.com/jqlang/jq/releases/tag/jq-1.7), which we don't have on our primary infra (Debian-based, thus still jq
1.6). I've fixed it in a follow-up commit, but that's something we'll need to be aware of and watching for now (or will have to update CI across all our repos to deal with somehow). 😞
(If only GHA let us provide our own VM images without having to manage the VMs ourselves too 😭)
Since this has been updated we've found that we need to disable OpCache within our Dockerfile to avoid some addition/division failures: |
As said on the php-src report, I doubt this is a general opcache issue but a JIT issue. |
It is now loaded by default upstream: docker-library/php#1587
As noted in #1589 (comment), I think #1589 (comment) makes me think we probably merged this a bit prematurely (we should've waited until it was at least merged/enabled in upstream's development branch), but it is what it is at this point and we're probably better off just riding it through now to avoid too much churn (just recording this here for posterity / to make it easier to find). |
@tianon Yeah, I had seen the comment over in the other PR and agree that the process was not ideal. Given that the RFC already had a working PoC implementation, I had anticipated it being merged more quickly. It will probably happen for Alpha 3, php/php-src#18939 - the main part of the implementation is now already reviewed, but due to a vacation it will likely be merged after Alpha 2. |
OPcache is an essential part of modern PHP and will always be included in PHP starting with 8.5 (https://wiki.php.net/rfc/make_opcache_required). Enable OPcache by default in Docker to simplify the set-up.
I've verified that running
docker-php-ext-install opcache
anddocker-php-ext-enable opcache
in a custom Docker image based on the PHP image will not cause issues.