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

fix: opcache configuration #2185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

adripo
Copy link
Contributor

@adripo adripo commented Mar 21, 2024

@MichaIng
Copy link
Member

As stated in the AIO issue, I would not raise OPcache size and interned strings buffer, unless there is really a case where this is required. Reducing JIT to 8 MiB alone should solve the issue.

@adripo
Copy link
Contributor Author

adripo commented Apr 3, 2024

@MichaIng @J0WI I tried to start a discussion on AIO repo before your review for the correct JIT value, but no further analysis has been done on this topic. If you want to participate here is the link. It would be nice to have the same configurations in Docker and AIO solution.

nextcloud/all-in-one#4416

adripo

This comment was marked as off-topic.

@MichaIng
Copy link
Member

MichaIng commented Apr 3, 2024

I think the solution done here, as well as in AIO, is fine. Since the JIT cache size seems to pre-allocate memory from overall OPcache (same as interned strings buffer does), it should fix the warning.

My idea to generate an image with JIT disabled completely was more for testing purpose, to see whether it has the same larger than expected impact on OPcache buffer usage that I observed on my test system, strengthening my guess that JIT uses in fact more OPcache buffer than what one configures with opcache.jit_buffer_size (and what usage stats report back). I find such hidden memory usage weird and it makes it more difficult to find right values other than trial&error. So in this case, I would try to get some clarification on this in PHP bug tracker.

AIO uses doubled OPcache size now. On x86_64, this is not such a problem (compared to embedded devices, ARM SBCs and such), and the image comes with quite a number of additional apps, also larger ones like Nextcloud Office, so it makes sense cover possible edge cases with high OPcache usage, for Docker containers where changing such configs is not so easy/common.

For this lighter Nextcloud Docker image, it should be fine to leave the overall OPcache size at 128M (default). It can be raised later in case there appear really cases, where it is still insufficient.

@MichaIng MichaIng added the bug label Apr 3, 2024
@adripo
Copy link
Contributor Author

adripo commented Apr 7, 2024

I agree. Let's fix the alert first and do other fixes if necessary in the future.

@joshtrichards
Copy link
Member

joshtrichards commented May 17, 2024

I'm not fully up to speed on this topic, but to help nudge this along:

In general we try to keep this image set aligned with what's recommended in the official Nextcloud Admin Manual for a baseline deployment.

My concern here is that we're going off book.

Maybe we work on clarifying / updating the upstream docs first: https://docs.nextcloud.com/server/latest/admin_manual/installation/server_tuning.html#enable-php-opcache

After all part of the problem seems to be that the docs lack clarity in this area.

AIO is sort of a different creature. There things can be tuned based on fewer unknowns (it has a specific way of doing things, is designed for a specific type of deployment, and the pieces it is commonly deployed with are easier to predict).

This image, however, isn't like that.

If nothing else, let's push an update of the upstream docs in parallel. After all, you all have already done the mental work of figuring out some values that apparently make more sense as baseline defaults. The benefits should not be limited to only this Docker image. Let's make things smoother for others as well. :-)

@MichaIng
Copy link
Member

Maybe we work on clarifying / updating the upstream docs first: https://docs.nextcloud.com/server/latest/admin_manual/installation/server_tuning.html#enable-php-opcache

Absolutely. Looks like someone threw inside a number without checking back anything (no offence 😄). I think I mentioned it elsewhere already, that additionally it makes sense to mention that JIT is x86_64-only, not supported on ARM, RISC-V or anything else.

@J0WI
Copy link
Contributor

J0WI commented Oct 8, 2024

@MichaIng https://github.com/nextcloud/documentation/pull/11872/files#r1638279688 didn't update the recommend jit_buffer_size. Are you still planning to update that in the docs?

@MichaIng
Copy link
Member

MichaIng commented Oct 8, 2024

I'll do when I find time. Kinda busy recently, though it is a small change only.

@MichaIng
Copy link
Member

@J0WI
PR is up: nextcloud/documentation#12284

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

Successfully merging this pull request may close these issues.

warning: The OPcache buffer is nearly full
4 participants