From 9ea7b58a8f26ca8856211da30eed5751706d0c4b Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 4 Oct 2018 17:32:51 +0200 Subject: [PATCH 1/5] NEW Add memory cache to ThemeResourceLoader::findTemplate() For large sites, this reduces the number of file_exists, Path::join, getThemePaths etc calls and can contribute to 26% reduction in TTFB load time --- src/View/ThemeResourceLoader.php | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/View/ThemeResourceLoader.php b/src/View/ThemeResourceLoader.php index 60945e5f2..74c619446 100644 --- a/src/View/ThemeResourceLoader.php +++ b/src/View/ThemeResourceLoader.php @@ -17,6 +17,13 @@ class ThemeResourceLoader */ private static $instance; + /** + * Internal memory cache for large sets of repeated calls + * + * @var array + */ + protected static $cacheData = []; + /** * The base path of the application * @@ -174,6 +181,12 @@ class ThemeResourceLoader */ public function findTemplate($template, $themes = null) { + // Look for a cached result for this data set + $cacheKey = json_encode($template) . json_encode($themes); + if (isset(static::$cacheData[$cacheKey])) { + return static::$cacheData[$cacheKey]; + } + if ($themes === null) { $themes = SSViewer::get_themes(); } @@ -196,7 +209,7 @@ class ThemeResourceLoader if (is_array($template)) { $path = $this->findTemplate($template, $themes); if ($path) { - return $path; + return static::$cacheData[$cacheKey] = $path; } continue; } @@ -205,7 +218,7 @@ class ThemeResourceLoader // pass in templates without extensions in order for template manifest to find // files dynamically. if (substr($template, -3) == '.ss' && file_exists($template)) { - return $template; + return static::$cacheData[$cacheKey] = $template; } // Check string template identifier @@ -220,13 +233,13 @@ class ThemeResourceLoader $pathParts = [ $this->base, $themePath, 'templates', $head, $type, $tail ]; $path = Path::join($pathParts) . '.ss'; if (file_exists($path)) { - return $path; + return static::$cacheData[$cacheKey] = $path; } } } // No template found - return null; + return static::$cacheData[$cacheKey] = null; } /** From 172d8915b773e7768db4308911912db987d8158e Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 4 Oct 2018 17:49:35 +0200 Subject: [PATCH 2/5] Clear cache between ThemeResourceLoader tests --- src/View/ThemeResourceLoader.php | 8 ++++++++ tests/php/Core/Manifest/ThemeResourceLoaderTest.php | 3 +++ 2 files changed, 11 insertions(+) diff --git a/src/View/ThemeResourceLoader.php b/src/View/ThemeResourceLoader.php index 74c619446..9a742489b 100644 --- a/src/View/ThemeResourceLoader.php +++ b/src/View/ThemeResourceLoader.php @@ -350,4 +350,12 @@ class ThemeResourceLoader } return $paths; } + + /** + * Clear any internally cached data from memory + */ + public static function flushCache() + { + static::$cacheData = []; + } } diff --git a/tests/php/Core/Manifest/ThemeResourceLoaderTest.php b/tests/php/Core/Manifest/ThemeResourceLoaderTest.php index 6ff3f7c21..aaf8ff27e 100644 --- a/tests/php/Core/Manifest/ThemeResourceLoaderTest.php +++ b/tests/php/Core/Manifest/ThemeResourceLoaderTest.php @@ -54,6 +54,9 @@ class ThemeResourceLoaderTest extends SapphireTest // New Loader for that root $this->loader = new ThemeResourceLoader($this->base); $this->loader->addSet('$default', $this->manifest); + + // Flush any cached values + ThemeResourceLoader::flushCache(); } protected function tearDown() From da9301f241085a6359d36a88f28df4d5dc1ec529 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 5 Oct 2018 14:58:48 +0200 Subject: [PATCH 3/5] Use SilverStripe cache API instead of memory cache, add tests and fix cache config indentation --- _config/cache.yml | 26 +++++---- src/View/ThemeResourceLoader.php | 57 +++++++++++++++---- .../Core/Manifest/ThemeResourceLoaderTest.php | 29 +++++++++- 3 files changed, 88 insertions(+), 24 deletions(-) diff --git a/_config/cache.yml b/_config/cache.yml index 48715eb4f..7719370f3 100644 --- a/_config/cache.yml +++ b/_config/cache.yml @@ -15,20 +15,24 @@ SilverStripe\Core\Injector\Injector: namespace: "cacheblock" defaultLifetime: 600 Psr\SimpleCache\CacheInterface.VersionProvider_composerlock: - factory: SilverStripe\Core\Cache\CacheFactory - constructor: - namespace: "VersionProvider_composerlock" - args: - disable-container: true + factory: SilverStripe\Core\Cache\CacheFactory + constructor: + namespace: "VersionProvider_composerlock" + args: + disable-container: true Psr\SimpleCache\CacheInterface.RateLimiter: - factory: SilverStripe\Core\Cache\CacheFactory - constructor: - namespace: 'ratelimiter' - args: - disable-container: true + factory: SilverStripe\Core\Cache\CacheFactory + constructor: + namespace: 'ratelimiter' + args: + disable-container: true Psr\SimpleCache\CacheInterface.InheritedPermissions: factory: SilverStripe\Core\Cache\CacheFactory constructor: namespace: "InheritedPermissions" args: - disable-container: true \ No newline at end of file + disable-container: true + Psr\SimpleCache\CacheInterface.ThemeResourceLoader: + factory: SilverStripe\Core\Cache\CacheFactory + constructor: + namespace: 'ThemeResourceLoader' diff --git a/src/View/ThemeResourceLoader.php b/src/View/ThemeResourceLoader.php index 9a742489b..a0e3b07a4 100644 --- a/src/View/ThemeResourceLoader.php +++ b/src/View/ThemeResourceLoader.php @@ -3,13 +3,16 @@ namespace SilverStripe\View; use InvalidArgumentException; +use Psr\SimpleCache\CacheInterface; +use SilverStripe\Core\Flushable; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Manifest\ModuleLoader; use SilverStripe\Core\Path; /** * Handles finding templates from a stack of template manifest objects. */ -class ThemeResourceLoader +class ThemeResourceLoader implements Flushable { /** @@ -39,6 +42,11 @@ class ThemeResourceLoader */ protected $sets = []; + /** + * @var CacheInterface + */ + protected $cache; + /** * @return ThemeResourceLoader */ @@ -168,6 +176,8 @@ class ThemeResourceLoader * format "type/name", where type is the type of template to search for * (e.g. Includes, Layout). * + * The results of this method will be cached for future use. + * * @param string|array $template Template name, or template spec in array format with the keys * 'type' (type string) and 'templates' (template hierarchy in order of precedence). * If 'templates' is ommitted then any other item in the array will be treated as the template @@ -182,9 +192,9 @@ class ThemeResourceLoader public function findTemplate($template, $themes = null) { // Look for a cached result for this data set - $cacheKey = json_encode($template) . json_encode($themes); - if (isset(static::$cacheData[$cacheKey])) { - return static::$cacheData[$cacheKey]; + $cacheKey = md5(json_encode($template) . json_encode($themes)); + if ($this->getCache()->has($cacheKey)) { + return $this->getCache()->get($cacheKey); } if ($themes === null) { @@ -209,7 +219,8 @@ class ThemeResourceLoader if (is_array($template)) { $path = $this->findTemplate($template, $themes); if ($path) { - return static::$cacheData[$cacheKey] = $path; + $this->getCache()->set($cacheKey, $path); + return $path; } continue; } @@ -218,7 +229,8 @@ class ThemeResourceLoader // pass in templates without extensions in order for template manifest to find // files dynamically. if (substr($template, -3) == '.ss' && file_exists($template)) { - return static::$cacheData[$cacheKey] = $template; + $this->getCache()->set($cacheKey, $template); + return $template; } // Check string template identifier @@ -233,13 +245,15 @@ class ThemeResourceLoader $pathParts = [ $this->base, $themePath, 'templates', $head, $type, $tail ]; $path = Path::join($pathParts) . '.ss'; if (file_exists($path)) { - return static::$cacheData[$cacheKey] = $path; + $this->getCache()->set($cacheKey, $path); + return $path; } } } // No template found - return static::$cacheData[$cacheKey] = null; + $this->getCache()->set($cacheKey, null); + return null; } /** @@ -352,10 +366,31 @@ class ThemeResourceLoader } /** - * Clear any internally cached data from memory + * Flush any cached data */ - public static function flushCache() + public static function flush() { - static::$cacheData = []; + self::inst()->getCache()->clear(); + } + + /** + * @return CacheInterface + */ + public function getCache() + { + if (!$this->cache) { + $this->setCache(Injector::inst()->get(CacheInterface::class . '.ThemeResourceLoader')); + } + return $this->cache; + } + + /** + * @param CacheInterface $cache + * @return ThemeResourceLoader + */ + public function setCache(CacheInterface $cache) + { + $this->cache = $cache; + return $this; } } diff --git a/tests/php/Core/Manifest/ThemeResourceLoaderTest.php b/tests/php/Core/Manifest/ThemeResourceLoaderTest.php index aaf8ff27e..42616ef99 100644 --- a/tests/php/Core/Manifest/ThemeResourceLoaderTest.php +++ b/tests/php/Core/Manifest/ThemeResourceLoaderTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Core\Tests\Manifest; +use Psr\SimpleCache\CacheInterface; use SilverStripe\Control\Director; use SilverStripe\Core\Manifest\ModuleLoader; use SilverStripe\View\ThemeResourceLoader; @@ -55,8 +56,8 @@ class ThemeResourceLoaderTest extends SapphireTest $this->loader = new ThemeResourceLoader($this->base); $this->loader->addSet('$default', $this->manifest); - // Flush any cached values - ThemeResourceLoader::flushCache(); + // Ensure the cache is flushed between tests + ThemeResourceLoader::flush(); } protected function tearDown() @@ -380,4 +381,28 @@ class ThemeResourceLoaderTest extends SapphireTest { $this->assertEquals($path, $this->loader->getPath($name)); } + + public function testFindTemplateWithCacheMiss() + { + $mockCache = $this->createMock(CacheInterface::class); + $mockCache->expects($this->once())->method('has')->willReturn(false); + $mockCache->expects($this->never())->method('get'); + $mockCache->expects($this->once())->method('set'); + + $loader = new ThemeResourceLoader(); + $loader->setCache($mockCache); + $loader->findTemplate('Page', ['$default']); + } + + public function testFindTemplateWithCacheHit() + { + $mockCache = $this->createMock(CacheInterface::class); + $mockCache->expects($this->once())->method('has')->willReturn(true);; + $mockCache->expects($this->never())->method('set'); + $mockCache->expects($this->once())->method('get')->willReturn('mock_template.ss'); + + $loader = new ThemeResourceLoader(); + $loader->setCache($mockCache); + $this->assertSame('mock_template.ss', $loader->findTemplate('Page', ['$default'])); + } } From 024762f03b1d89604fd21151e25e97e8db45bb4d Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 5 Oct 2018 15:00:32 +0200 Subject: [PATCH 4/5] Move default themes lookup before cache check --- src/View/ThemeResourceLoader.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/View/ThemeResourceLoader.php b/src/View/ThemeResourceLoader.php index a0e3b07a4..390f64e36 100644 --- a/src/View/ThemeResourceLoader.php +++ b/src/View/ThemeResourceLoader.php @@ -191,16 +191,16 @@ class ThemeResourceLoader implements Flushable */ public function findTemplate($template, $themes = null) { + if ($themes === null) { + $themes = SSViewer::get_themes(); + } + // Look for a cached result for this data set $cacheKey = md5(json_encode($template) . json_encode($themes)); if ($this->getCache()->has($cacheKey)) { return $this->getCache()->get($cacheKey); } - if ($themes === null) { - $themes = SSViewer::get_themes(); - } - $type = ''; if (is_array($template)) { // Check if templates has type specified From 98568262f2c5d7cc9a9cd39af158d5df7dce12a7 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Fri, 5 Oct 2018 16:07:33 +0200 Subject: [PATCH 5/5] Fixed phpcs violations --- src/ORM/Connect/MySQLSchemaManager.php | 2 +- tests/php/Core/Manifest/ThemeResourceLoaderTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ORM/Connect/MySQLSchemaManager.php b/src/ORM/Connect/MySQLSchemaManager.php index defa17911..b076ff5fd 100644 --- a/src/ORM/Connect/MySQLSchemaManager.php +++ b/src/ORM/Connect/MySQLSchemaManager.php @@ -151,7 +151,7 @@ class MySQLSchemaManager extends DBSchemaManager public function renameTable($oldTableName, $newTableName) { if (!$this->hasTable($oldTableName)) { - throw new LogicException('Table '. $oldTableName . ' does not exist.'); + throw new LogicException('Table ' . $oldTableName . ' does not exist.'); } return $this->query("ALTER TABLE \"$oldTableName\" RENAME \"$newTableName\""); diff --git a/tests/php/Core/Manifest/ThemeResourceLoaderTest.php b/tests/php/Core/Manifest/ThemeResourceLoaderTest.php index 42616ef99..abe0d65b7 100644 --- a/tests/php/Core/Manifest/ThemeResourceLoaderTest.php +++ b/tests/php/Core/Manifest/ThemeResourceLoaderTest.php @@ -397,7 +397,7 @@ class ThemeResourceLoaderTest extends SapphireTest public function testFindTemplateWithCacheHit() { $mockCache = $this->createMock(CacheInterface::class); - $mockCache->expects($this->once())->method('has')->willReturn(true);; + $mockCache->expects($this->once())->method('has')->willReturn(true); $mockCache->expects($this->never())->method('set'); $mockCache->expects($this->once())->method('get')->willReturn('mock_template.ss');