Enhance and complement OPcache setup checks

[MichaIng]

The current OPcache recommendations match the PHP defaults, but the values are much higher than required to run Nextcloud, even with a high number of installed apps. On the other hand, when other applications use the same OPcache instance, the recommended values might not be sufficient. Accurate recommendations need to take into account actual OPcache usage.

With this commit, recommendations are shown to raise the config value if more than 90% of max cache size or number of keys is used.

The checks whether the module is loaded and whether the OPcache is properly configured have been merged into a single function. This allowed to reduce the overhead of OPcache configuration checks when the module is not loaded.

A check has been added whether Nextcloud is permitted to use the OPcache API. Without this, inconsistencies during core or app upgrades may cause errors and OPcache usage cannot be determined for the new usage based checks.

OPcache usage based checks are skipped when Nextcloud is not permitted to use the API, or when the opcache_get_status function has been explicitly disabled: see below

[MichaIng]

All checks and functional tests passed:

Probably the output lines can be a little enhanced, e.g. showing config key and value on a separate line, or making them fat?

Also we could recommend the next power of two for the interned strings buffer, as smaller increments can lead to even a reduced resulting cache size: https://stackoverflow.com/q/67853338

In case of max accelerated files we could recommend the next effective value, as e.g. raising the config from 10000 to 16000 has zero effect, as in both cases 16229 is the resulting value: https://www.php.net/manual/opcache.configuration.php#ini.opcache.max-accelerated-files

But also too much magic is probably not great and instead it could make more sense to mention such details in the docs, and re-add the docs URL for further details. The docs should be adjusted then as well, I'll open a companion docs PR when this one is generally accepted.

Ah and the isOpcacheProperlySetup function and data variable could be renamed of course, to not get confused with a boolean returned, e.g. into getOPcacheRecommends, or so?

And finally, as alternative to iniGetWrapper, it would be possible to use opcache-get-configuration, to get all OPcache settings with a single command call. It has the downside that it cannot be used when opcache.restrict_api is set and does not include Nextcloud. In this case also the used opcache-get-status fails, so that all checks "succeed" and no recommendation is shown. In this case Nextcloud is not able to invalidated cached keys, which potentially causes issues during updates of apps or Nextcloud itself, when new files are expected or required but the old files loaded from cache. So finally it would be even a good thing to check for proper (or no) opcache.restrict_api, either directly via iniGetWrapper or indirectly via opcache-get-configuration/opcache-get-status failure. As no OPcache usage stats can be retrieved then, instead a recommendation to allow Nextcloud using the OPcache API could be shown. But this deserves a separate PR as it is an own issue.

[kesselb]

Thanks for your pull request 👍

Code looks good. The recommendations make sense to me. In serverinfo we have an additional check for the module extension_loaded('Zend OPcache').

I prefer #27301 because it does not call opcache_get_status and just check for the minimum values. We should have a short section in our documentation that opcache needs to adjusted for the actual use case and give some examples and references to the php documentation.

We have to take into account the people hate the setup checks. Especially when they are not able to fix it.
It's (for some) super important to see the green check in the admin overview and we have many discussions on GitHub and help.nextcloud.com about that.

What do you think about writing an app for Nextcloud to monitor the opcache usage, give recommendations how to optimize and maybe take over the opcache api from serverinfo?

[MichaIng]

Whoops, sorry for the long text 😄...

In serverinfo we have an additional check for the module extension_loaded('Zend OPcache').

You mean hasOpcacheLoaded()? Yes that makes sense and is untouched by this commit. isOpcacheProperlySetup is only evaluated if the module itself is enabled. But actually, both functions are always called, while isOpcacheProperlySetup() could be skipped in the first place when hasOpcacheLoaded() returned false, to reduce overhead a bit. Not sure how to achieve this best with the way the DataResponse is currently generated. Alternatively both functions could be merged via:

protected function isOpcacheProperlySetup(): array {
	$recommendations = [];

	if (!extension_loaded('Zend OPcache')) {
			$recommendations = ['The PHP OPcache module is not loaded. <a target="_blank" rel="noreferrer noopener" class="external" href="' . $this->urlGenerator->linkToDocs('admin-php-opcache') . '">For better performance it is recommended ↗</a> to load it into your PHP installation.'];
	} elseif (!$this->iniGetWrapper->getBool('opcache.enable')) {
...

which then also simplified the evaluation in setupchecks.js.

I prefer #27301 because it does not call opcache_get_status and just check for the minimum values.

#27301 does not address the underlying issue:

• It lowers the opcache.max_accelerated_files to 7963, to match one of the actually effective prime numbers. Not sure why PHP defaults to the ineffective 10000, which always results in 16229 max keys. But there is no guarantee that this is sufficient or still too high for the particular case. In my case ~2000 files are cached, so 3907 is sufficient. When you run other web applications on the same server, even 16229 might be too low.
• It raises the recommendation for opcache.interned_strings_buffer from 8 to 16, as in the authors particular case, 8 was not sufficient. In my case ~3.5 MiB interned strings buffer is used, with an older PHP version it was 2-3 MiB, hence 8 is sufficient (4 leads to 3145304 bytes, which was sufficient earlier), but in other cases again it can be too low or too high. The interned strings buffer changed quite a lot with the last PHP versions, so a fixed value cannot be right at all currently.
• opcache.memory_consumption is left untouched. I once tried to install literally ALL apps from the Nextcloud apps store (not all were possible, since some conflicted or did not work well with current NC that time) and accessed all their settings and apps pages, so test how high the OPcache usage can raise. And I was reaching only a tiny little above 64 MiB. This cannot be seen as representative of course. My current instance uses ~30 MiB OPcache memory (which includes the interned strings buffer always). But also here, as my instance and the authors comment that "some mentions of specific apps capable of throwing the memory requirement" shows quite well that it is very case-specific (use case, installed apps, PHP version, ...) and it cannot be right to recommend a fixed set of values.

We should have a short section in our documentation that opcache needs to adjusted for the actual use case and give some examples and references to the php documentation.

Yes, definitely, and this is what I'll do when this PR is accepted. But most users do not read every section of the docs, I suppose, and OPcache is critical for access performance, so doing as good as possible recommendations, that match the actual usage on the particular system, IMO is important. Otherwise users might complain about bad Nextcloud performance (compared to other applications with less OPcache usage), while raising max buffer values could fix it.

We have to take into account the people hate the setup checks. Especially when they are not able to fix it.

Exactly, we regularly receive reports of users who want to have all admin panel recommendations/warnings resolved, regardless if it can be sometimes well argued that they can/should be ignored in a particular case. In most cases the recommended values are too high currently, so this change will reduce the warnings in most cases. If the usage indeed exceeds the currently applied value too often, e.g. because PHP internally handles those differently or Nextcloud grows, it could be discussed whether to have the specifically affected value removed from the test and refer to the documentation instead. But for now I'm 100% sure that this PR helps reducing the warning. #27094 clearly implies that the recommendations are shown much more often, as the interned strings buffer in PHP defaults to 8, which is the current recommendation, but that PR raises it to 16. So practically all users will at first see that warning (while currently none does when the values were not manually reduced) and I guess nearly all users of shared hostings as well, when each customer has an own OPcache instance, at least.

What do you think about writing an app for Nextcloud to monitor the opcache usage, give recommendations how to optimize and maybe take over the opcache api from serverinfo?

If someone has time to write it, why not. But actually the change and check here is quite simple and preserves what the admin panel checks are intended for, including the IMO quite important performance aspect, that is part of the checks headline for a reason. There are other checks which are IMO way less relevant or should be part of a discussion to have them removed and addressed differently instead, like the case of the Imagick module or these. So when it's about reducing the amount of warnings, then I'd personally not start with OPcache. To have some more overview over OPcache usage in general, allow manual invalidation etc, what I aim to add to the docs is a hint about https://github.com/amnuts/opcache-gui.

[MichaIng]

Thanks. Makes sense to checks this as well before calling it then. Since this only means the checks cannot be done, while it does not affect Nextcloud directly (like opcache.restrict_api), shall we add a "recommendation" to allow opcache_get_status for Nextcloud to be able to check OPcache usage, or shall we ignore it silently? There might be admins who want to have the NC admin panel free of warnings/recommendations but want to have opcache_get_status disabled, knowing/believing that their values are fine.

EDIT: For now I added it a way that the checks are skipped silently. This preserves the previous behaviour when it was not checked but opcache_get_status returned a muted error instead. So no new warnings are added.

All green again, ready for review from my end.

[MichaIng]

I retested all combinations of OPcache and whether the warnings show up as expected, once on master and once on Nextcloud 23, which both works well.

[MichaIng]

/backport to stable23

[MichaIng]

Related documentation update is proposed here: nextcloud/documentation#7859