Do OPcache recommendations based on actual cache usage

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. Signed-off-by: MichaIng <[email protected]>
nextcloud · Jun 8, 2021 · 9dc17be · 9dc17be
1 parent b3cfa18
commit 9dc17be
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 34 deletions.
diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php
@@ -220,7 +220,7 @@ protected function getCurlVersion() {
 	}
 
 	/**
-	 * Check if the used  SSL lib is outdated. Older OpenSSL and NSS versions do
+	 * Check if the used SSL lib is outdated. Older OpenSSL and NSS versions do
 	 * have multiple bugs which likely lead to problems in combination with
 	 * functionality required by ownCloud such as SNI.
 	 *
@@ -426,31 +426,39 @@ public function getFailedIntegrityCheckFiles(): DataDisplayResponse {
 	}
 
 	/**
-	 * Checks whether a PHP opcache is properly set up
-	 * @return bool
+	 * Checks whether a PHP OPcache is properly set up
+	 * @return string[] The list of OPcache setup recommendations
 	 */
-	protected function isOpcacheProperlySetup() {
+	protected function isOpcacheProperlySetup(): array {
+		$recommendations = [];
+
 		if (!$this->iniGetWrapper->getBool('opcache.enable')) {
-			return false;
-		}
+			$recommendations[] = 'OPcache is disabled. For better performance, it is recommended to apply <code>opcache.enable=1</code> to your PHP configuration.';
 
-		if (!$this->iniGetWrapper->getBool('opcache.save_comments')) {
-			return false;
-		}
+			// Check for saved comments only when OPcache is currently disabled. If it was enabled, opcache.save_comments=0 would break Nextcloud in the first place.
+			if (!$this->iniGetWrapper->getBool('opcache.save_comments')) {
+				$recommendations[] = 'OPcache is configured to remove code comments. With OPcache enabled, <code>opcache.save_comments=1</code> must be set for Nextcloud to function.';
+			}
+		} else {
+			// Mute error when opcache.restrict_api is set and does not permit Nextcloud to use opcache_get_status(). All below conditions will then return false.
+			// ToDo: Add a check for opcache.restrict_api, since it can cause issues when Nextcloud cannot evict cached scripts: https://github.com/nextcloud/server/pull/8188
+			$status = @opcache_get_status();
 
-		if ($this->iniGetWrapper->getNumeric('opcache.max_accelerated_files') < 10000) {
-			return false;
-		}
+			// Recommend to raise value, if more than 90% of max value is reached
+			if ($status['opcache_statistics']['num_cached_keys'] / $status['opcache_statistics']['max_cached_keys'] > 0.9) {
+				$recommendations[] = 'The maximum number of OPcache keys is nearly exceeded. To assure that all scripts can be hold in cache, it is recommended to apply <code>opcache.max_accelerated_files</code> to your PHP configuration with a value higher than <code>' . ($this->iniGetWrapper->getNumeric('opcache.max_accelerated_files') ?: 'currently') . '</code>.';
+			}
 
-		if ($this->iniGetWrapper->getNumeric('opcache.memory_consumption') < 128) {
-			return false;
-		}
+			if ($status['memory_usage']['used_memory'] / $status['memory_usage']['free_memory'] > 9) {
+				$recommendations[] = 'The OPcache buffer is nearly full. To assure that all scripts can be hold in cache, it is recommended to apply <code>opcache.memory_consumption</code> to your PHP configuration with a value higher than <code>' . ($this->iniGetWrapper->getNumeric('opcache.memory_consumption') ?: 'currently') . '</code>.';
+			}
 
-		if ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') < 8) {
-			return false;
+			if ($status['interned_strings_usage']['used_memory'] / $status['interned_strings_usage']['free_memory'] > 9) {
+				$recommendations[] = 'The OPcache interned strings buffer is nearly full. To assure that repeating strings can be effectively cached, it is recommended to apply <code>opcache.interned_strings_buffer</code> to your PHP configuration with a value higher than <code>' . ($this->iniGetWrapper->getNumeric('opcache.interned_strings_buffer') ?: 'currently') . '</code>.';
+			}
 		}
 
-		return true;
+		return $recommendations;
 	}
 
 	/**

diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js
@@ -329,11 +329,16 @@
 								.replace('{linkend}', '</a>'),
 							type: OC.SetupChecks.MESSAGE_TYPE_INFO
 						});
-					} else if(!data.isOpcacheProperlySetup) {
+					} else if(data.isOpcacheProperlySetup.length > 0) {
+						var listOfOPcacheRecommends = "";
+						data.isOpcacheProperlySetup.forEach(function(element){
+							listOfOPcacheRecommends += "<li>" + element + "</li>";
+						});
 						messages.push({
-							msg: t('core', 'The PHP OPcache module is not properly configured. {linkstart}For better performance it is recommended ↗{linkend} to use the following settings in the <code>php.ini</code>:')
-								.replace('{linkstart}', '<a target="_blank" rel="noreferrer noopener" class="external" href="' + data.phpOpcacheDocumentation + '">')
-								.replace('{linkend}', '</a>') + "<pre><code>opcache.enable=1\nopcache.interned_strings_buffer=8\nopcache.max_accelerated_files=10000\nopcache.memory_consumption=128\nopcache.save_comments=1\nopcache.revalidate_freq=1</code></pre>",
+							msg: t(
+								'core',
+								'The PHP OPcache module is not properly configured:'
+							) + "<ul>" + listOfOPcacheRecommends + "</ul>",
 							type: OC.SetupChecks.MESSAGE_TYPE_INFO
 						});
 					}

diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js
@@ -235,7 +235,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -291,7 +291,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -348,7 +348,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -403,7 +403,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -456,7 +456,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: false,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -509,7 +509,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -564,7 +564,7 @@ describe('OC.SetupChecks tests', function() {
 					reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html',
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -617,7 +617,7 @@ describe('OC.SetupChecks tests', function() {
 					reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html',
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: false,
 					hasFreeTypeSupport: true,
@@ -670,7 +670,7 @@ describe('OC.SetupChecks tests', function() {
 					reverseProxyDocs: 'https://docs.nextcloud.com/foo/bar.html',
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -744,7 +744,7 @@ describe('OC.SetupChecks tests', function() {
 					phpSupported: {eol: true, version: '5.4.0'},
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: true,
+					isOpcacheProperlySetup: [],
 					hasOpcacheLoaded: true,
 					isSettimelimitAvailable: true,
 					hasFreeTypeSupport: true,
@@ -797,7 +797,7 @@ describe('OC.SetupChecks tests', function() {
 					forwardedForHeadersWorking: true,
 					isCorrectMemcachedPHPModuleInstalled: true,
 					hasPassedCodeIntegrityCheck: true,
-					isOpcacheProperlySetup: false,
+					isOpcacheProperlySetup: ['recommendation1', 'recommendation2'],
 					hasOpcacheLoaded: true,
 					phpOpcacheDocumentation: 'https://example.org/link/to/doc',
 					isSettimelimitAvailable: true,
@@ -822,7 +822,7 @@ describe('OC.SetupChecks tests', function() {
 
 			async.done(function( data, s, x ){
 				expect(data).toEqual([{
-						msg: 'The PHP OPcache module is not properly configured. <a target="_blank" rel="noreferrer noopener" class="external" href="https://example.org/link/to/doc">For better performance it is recommended ↗</a> to use the following settings in the <code>php.ini</code>:' + "<pre><code>opcache.enable=1\nopcache.interned_strings_buffer=8\nopcache.max_accelerated_files=10000\nopcache.memory_consumption=128\nopcache.save_comments=1\nopcache.revalidate_freq=1</code></pre>",
+						msg: 'The PHP OPcache module is not properly configured:<ul><li>recommendation1</li><li>recommendation2</li></ul>',
 						type: OC.SetupChecks.MESSAGE_TYPE_INFO
 					}]);
 				done();