From 9cb863cb26fe113f3da31ed29cb8f6e622b0f1fc Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Fri, 21 Sep 2018 14:42:21 +1200 Subject: [PATCH 1/5] FIX Opt-in performance fix for many consecutive lookups using isLocalisedInStage --- src/Extension/FluentVersionedExtension.php | 80 ++++++++++++++++++++-- 1 file changed, 76 insertions(+), 4 deletions(-) diff --git a/src/Extension/FluentVersionedExtension.php b/src/Extension/FluentVersionedExtension.php index 6eced6f0..5838f101 100644 --- a/src/Extension/FluentVersionedExtension.php +++ b/src/Extension/FluentVersionedExtension.php @@ -3,8 +3,13 @@ namespace TractorCow\Fluent\Extension; use InvalidArgumentException; +use SilverStripe\CMS\Model\SiteTree; +use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataQuery; use SilverStripe\ORM\DB; +use SilverStripe\ORM\Queries\SQLExpression; use SilverStripe\ORM\Queries\SQLSelect; use SilverStripe\Versioned\Versioned; use TractorCow\Fluent\Model\Locale; @@ -60,6 +65,21 @@ class FluentVersionedExtension extends FluentExtension */ protected $localisedStageCache = []; + /** + * Array of objectIds keyed by table (ie. stage) and locale. Indicates which object IDs exist in which locales. + * + * static::$idsInLocaleCache[ $locale ][ $table(.self::SUFFIX_LIVE) ][ $objectId ] === true + * + * @var string[][][] + */ + protected static $idsInLocaleCache = []; + + /** + * @config + * @var array + */ + private static $prepopulate_locale_object_types = []; + protected function augmentDatabaseDontRequire($localisedTable) { DB::dont_require_table($localisedTable); @@ -297,6 +317,12 @@ protected function isLocalisedInStage($stage, $locale = null) } } + $ownerClass = get_class($this->owner); + $prepopulateClasses = Config::inst()->get(self::class, 'prepopulate_locale_object_types'); + if (is_array($prepopulateClasses) && in_array($ownerClass, $prepopulateClasses)) { + self::prepoulateIdsInLocale($locale, $ownerClass); + } + // Get table $baseTable = $this->owner->baseTable(); $table = $this->getLocalisedTable($baseTable); @@ -304,21 +330,26 @@ protected function isLocalisedInStage($stage, $locale = null) $table .= self::SUFFIX_LIVE; } + if (isset(self::$idsInLocaleCache[$locale][$table])) { + return in_array($this->owner->ID, self::$idsInLocaleCache[$locale][$table]); + } + // Check cache $key = $table . '/' . $locale . '/' . $this->owner->ID; if (isset($this->localisedStageCache[$key])) { return $this->localisedStageCache[$key]; } - $query = new SQLSelect(); - $query->selectField('"ID"'); + $query = new SQLSelect( + '"ID"' + ); $query->addFrom('"'. $table . '"'); $query->addWhere([ '"RecordID"' => $this->owner->ID, '"Locale"' => $locale, ]); - $query->firstRow(); - $result = $query->execute()->value() !== null; + + $result = $query->firstRow()->execute()->value() !== null; // Set cache $this->localisedStageCache[$key] = $result; @@ -329,4 +360,45 @@ public function flushCache() { $this->localisedStageCache = []; } + + /** + * Prepopulate the cache of + * + * @param string $locale + * @param string $dataObject + */ + public static function prepoulateIdsInLocale($locale, $dataObjectClass, $populateLive = true, $populateDraft = true) + { + // Get the table for the given DataObject class + $self = new static(); + $table = $self->getLocalisedTable(Injector::inst()->get($dataObjectClass)->baseTable()); + + // If we already have items then we've been here before... + if (isset(self::$idsInLocaleCache[$locale][$table])) { + return; + } + + $tables = []; + if ($populateDraft) { + $tables[] = $table; + } + if ($populateLive) { + $tables[] = $table . self::SUFFIX_LIVE; + } + + // Populate both the draft and live stages + foreach ($tables as $table) { + /** @var SQLSelect $select */ + $select = SQLSelect::create( + ['"RecordID"'], + '"' . $table . '"', + ['Locale' => $locale] + ); + $result = $select->execute(); + $ids = $result->column('RecordID'); + + // We need to execute ourselves as the param is lost from the subSelect + self::$idsInLocaleCache[$locale][$table] = $ids; + } + } } From c724a85d42c5b397e82b881340f40a6018da2dd9 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 21 Sep 2018 18:34:47 +1200 Subject: [PATCH 2/5] FIX: Use Hierarchy::prepopulateTreeDataCache() in fluent Requires https://github.com/silverstripe/silverstripe-framework/pull/8395 --- src/Extension/FluentVersionedExtension.php | 40 +++++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/src/Extension/FluentVersionedExtension.php b/src/Extension/FluentVersionedExtension.php index 5838f101..148a7e0f 100644 --- a/src/Extension/FluentVersionedExtension.php +++ b/src/Extension/FluentVersionedExtension.php @@ -75,10 +75,13 @@ class FluentVersionedExtension extends FluentExtension protected static $idsInLocaleCache = []; /** + * Used to enable or disable the prepopulation of the locale content cache + * Defaults to true. + * * @config - * @var array + * @var boolean */ - private static $prepopulate_locale_object_types = []; + private static $prepopulate_localecontent_cache = true; protected function augmentDatabaseDontRequire($localisedTable) { @@ -317,12 +320,6 @@ protected function isLocalisedInStage($stage, $locale = null) } } - $ownerClass = get_class($this->owner); - $prepopulateClasses = Config::inst()->get(self::class, 'prepopulate_locale_object_types'); - if (is_array($prepopulateClasses) && in_array($ownerClass, $prepopulateClasses)) { - self::prepoulateIdsInLocale($locale, $ownerClass); - } - // Get table $baseTable = $this->owner->baseTable(); $table = $this->getLocalisedTable($baseTable); @@ -362,7 +359,32 @@ public function flushCache() } /** - * Prepopulate the cache of + * Hook into {@link Hierarchy::prepopulateTreeDataCache}. + * + * @param DataList $recordList The list of records to prepopulate caches for. Null for all records. + * @param array $options A map of hints about what should be cached. "numChildrenMethod" and + * "childrenMethod" are allowed keys. + */ + public function onPrepopulateTreeDataCache(DataList $recordList = null, array $options = []) + { + if (!Config::inst()->get(static::class, 'prepopulate_localecontent_cache')) { + return; + } + + // This functionality hasn't been implemented; it's not actually used right now, so assume you aren't going to + // need it. + if ($recordList) { + user_error( + "FluentVersionedExtension::onPrepopulateTreeDataCache not currently optimised for recordList use", + E_USER_WARNING + ); + } + + self::prepoulateIdsInLocale(FluentState::singleton()->getLocale(), $this->owner->baseClass()); + } + + /** + * Prepopulate the cache of IDs in a locale, to optimise batch calls to isLocalisedInStage. * * @param string $locale * @param string $dataObject From 77a3a6fa4a882d32847514b6cf184dfa8870bfec Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Wed, 26 Sep 2018 17:12:30 +1200 Subject: [PATCH 3/5] Clarifying difference in caches & using isset instead of in_array --- src/Extension/FluentVersionedExtension.php | 43 +++++++++++----------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/Extension/FluentVersionedExtension.php b/src/Extension/FluentVersionedExtension.php index 148a7e0f..53d3ee3d 100644 --- a/src/Extension/FluentVersionedExtension.php +++ b/src/Extension/FluentVersionedExtension.php @@ -63,14 +63,18 @@ class FluentVersionedExtension extends FluentExtension * * @var array */ - protected $localisedStageCache = []; + protected static $localisedStageCache = []; /** - * Array of objectIds keyed by table (ie. stage) and locale. Indicates which object IDs exist in which locales. + * Array of objectIds keyed by table (ie. stage) and locale. This knows ALL object IDs that exist in the given table + * and locale. * - * static::$idsInLocaleCache[ $locale ][ $table(.self::SUFFIX_LIVE) ][ $objectId ] === true + * This is different from the above cache which caches the result per object - each array (keyed by locale & table) + * will have ALL object IDs for that locale & table. * - * @var string[][][] + * static::$idsInLocaleCache[ $locale ][ $table(.self::SUFFIX_LIVE) ][ $objectId ] = $objectId + * + * @var int[][][] */ protected static $idsInLocaleCache = []; @@ -327,14 +331,14 @@ protected function isLocalisedInStage($stage, $locale = null) $table .= self::SUFFIX_LIVE; } - if (isset(self::$idsInLocaleCache[$locale][$table])) { - return in_array($this->owner->ID, self::$idsInLocaleCache[$locale][$table]); + if (isset(static::$idsInLocaleCache[$locale][$table])) { + return isset(static::$idsInLocaleCache[$locale][$table][$this->owner->ID]); } // Check cache $key = $table . '/' . $locale . '/' . $this->owner->ID; - if (isset($this->localisedStageCache[$key])) { - return $this->localisedStageCache[$key]; + if (isset(static::$localisedStageCache[$key])) { + return static::$localisedStageCache[$key]; } $query = new SQLSelect( @@ -349,35 +353,32 @@ protected function isLocalisedInStage($stage, $locale = null) $result = $query->firstRow()->execute()->value() !== null; // Set cache - $this->localisedStageCache[$key] = $result; + static::$localisedStageCache[$key] = $result; return $result; } public function flushCache() { - $this->localisedStageCache = []; + static::$localisedStageCache = []; } /** * Hook into {@link Hierarchy::prepopulateTreeDataCache}. * - * @param DataList $recordList The list of records to prepopulate caches for. Null for all records. + * @param DataList|array $recordList The list of records to prepopulate caches for. Null for all records. * @param array $options A map of hints about what should be cached. "numChildrenMethod" and * "childrenMethod" are allowed keys. */ public function onPrepopulateTreeDataCache(DataList $recordList = null, array $options = []) { - if (!Config::inst()->get(static::class, 'prepopulate_localecontent_cache')) { + if (!Config::inst()->get(self::class, 'prepopulate_localecontent_cache')) { return; } - // This functionality hasn't been implemented; it's not actually used right now, so assume you aren't going to - // need it. + // Prepopulating for a specific list of records hasn't been implemented yet and will have to rely on the + // fallback implementation of caching per record. if ($recordList) { - user_error( - "FluentVersionedExtension::onPrepopulateTreeDataCache not currently optimised for recordList use", - E_USER_WARNING - ); + return; } self::prepoulateIdsInLocale(FluentState::singleton()->getLocale(), $this->owner->baseClass()); @@ -392,8 +393,8 @@ public function onPrepopulateTreeDataCache(DataList $recordList = null, array $o public static function prepoulateIdsInLocale($locale, $dataObjectClass, $populateLive = true, $populateDraft = true) { // Get the table for the given DataObject class - $self = new static(); - $table = $self->getLocalisedTable(Injector::inst()->get($dataObjectClass)->baseTable()); + $dataObject = DataObject::singleton($dataObjectClass); + $table = $dataObject->getLocalisedTable($dataObject->baseTable()); // If we already have items then we've been here before... if (isset(self::$idsInLocaleCache[$locale][$table])) { @@ -420,7 +421,7 @@ public static function prepoulateIdsInLocale($locale, $dataObjectClass, $populat $ids = $result->column('RecordID'); // We need to execute ourselves as the param is lost from the subSelect - self::$idsInLocaleCache[$locale][$table] = $ids; + self::$idsInLocaleCache[$locale][$table] = array_combine($ids, $ids); } } } From a887bedc9e438475e14101695b417e22200f6868 Mon Sep 17 00:00:00 2001 From: Guy Marriott Date: Thu, 27 Sep 2018 13:37:25 +1200 Subject: [PATCH 4/5] Fixing method signature & docblocks --- src/Extension/FluentVersionedExtension.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Extension/FluentVersionedExtension.php b/src/Extension/FluentVersionedExtension.php index 53d3ee3d..71daadaf 100644 --- a/src/Extension/FluentVersionedExtension.php +++ b/src/Extension/FluentVersionedExtension.php @@ -6,6 +6,7 @@ use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; +use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataQuery; use SilverStripe\ORM\DB; @@ -369,7 +370,7 @@ public function flushCache() * @param array $options A map of hints about what should be cached. "numChildrenMethod" and * "childrenMethod" are allowed keys. */ - public function onPrepopulateTreeDataCache(DataList $recordList = null, array $options = []) + public function onPrepopulateTreeDataCache($recordList = null, array $options = []) { if (!Config::inst()->get(self::class, 'prepopulate_localecontent_cache')) { return; @@ -388,7 +389,9 @@ public function onPrepopulateTreeDataCache(DataList $recordList = null, array $o * Prepopulate the cache of IDs in a locale, to optimise batch calls to isLocalisedInStage. * * @param string $locale - * @param string $dataObject + * @param $dataObjectClass + * @param bool $populateLive + * @param bool $populateDraft */ public static function prepoulateIdsInLocale($locale, $dataObjectClass, $populateLive = true, $populateDraft = true) { From 7128efedbad3a6dbb15153bf0584df2d4ae95285 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Thu, 27 Sep 2018 13:09:19 +0200 Subject: [PATCH 5/5] Add owner ID to optimistic cache isset check, augment doc blocks, add tests for static caching --- src/Extension/FluentVersionedExtension.php | 42 +++++++++++------ .../FluentVersionedExtensionTest.php | 46 ++++++++++++++++++- 2 files changed, 72 insertions(+), 16 deletions(-) diff --git a/src/Extension/FluentVersionedExtension.php b/src/Extension/FluentVersionedExtension.php index 71daadaf..005801f9 100644 --- a/src/Extension/FluentVersionedExtension.php +++ b/src/Extension/FluentVersionedExtension.php @@ -3,14 +3,11 @@ namespace TractorCow\Fluent\Extension; use InvalidArgumentException; -use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Core\Config\Config; -use SilverStripe\Core\Injector\Injector; use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataQuery; use SilverStripe\ORM\DB; -use SilverStripe\ORM\Queries\SQLExpression; use SilverStripe\ORM\Queries\SQLSelect; use SilverStripe\Versioned\Versioned; use TractorCow\Fluent\Model\Locale; @@ -332,34 +329,48 @@ protected function isLocalisedInStage($stage, $locale = null) $table .= self::SUFFIX_LIVE; } - if (isset(static::$idsInLocaleCache[$locale][$table])) { + // Check for a cached item in the full list of all objects. These are populated optimistically. + if (isset(static::$idsInLocaleCache[$locale][$table][$this->owner->ID])) { return isset(static::$idsInLocaleCache[$locale][$table][$this->owner->ID]); } - // Check cache + // Check cache from local instance calls $key = $table . '/' . $locale . '/' . $this->owner->ID; if (isset(static::$localisedStageCache[$key])) { return static::$localisedStageCache[$key]; } - $query = new SQLSelect( - '"ID"' - ); + // Set cache and return + return static::$localisedStageCache[$key] = $this->findRecordInLocale($locale, $table, $this->owner->ID); + } + + /** + * Checks whether the given record ID exists in the given locale, in the given table. Skips using the ORM because + * we don't need it for this call. + * + * @param string $locale + * @param string $table + * @param int $id + * @return bool + */ + protected function findRecordInLocale($locale, $table, $id) + { + $query = SQLSelect::create('"ID"'); $query->addFrom('"'. $table . '"'); $query->addWhere([ - '"RecordID"' => $this->owner->ID, + '"RecordID"' => $id, '"Locale"' => $locale, ]); - $result = $query->firstRow()->execute()->value() !== null; - - // Set cache - static::$localisedStageCache[$key] = $result; - return $result; + return $query->firstRow()->execute()->value() !== null; } + /** + * Clear internal static property caches + */ public function flushCache() { + static::$idsInLocaleCache = []; static::$localisedStageCache = []; } @@ -389,13 +400,14 @@ public function onPrepopulateTreeDataCache($recordList = null, array $options = * Prepopulate the cache of IDs in a locale, to optimise batch calls to isLocalisedInStage. * * @param string $locale - * @param $dataObjectClass + * @param string $dataObjectClass * @param bool $populateLive * @param bool $populateDraft */ public static function prepoulateIdsInLocale($locale, $dataObjectClass, $populateLive = true, $populateDraft = true) { // Get the table for the given DataObject class + /** @var DataObject|FluentExtension $dataObject */ $dataObject = DataObject::singleton($dataObjectClass); $table = $dataObject->getLocalisedTable($dataObject->baseTable()); diff --git a/tests/php/Extension/FluentVersionedExtensionTest.php b/tests/php/Extension/FluentVersionedExtensionTest.php index 316aca53..9dda7fb5 100644 --- a/tests/php/Extension/FluentVersionedExtensionTest.php +++ b/tests/php/Extension/FluentVersionedExtensionTest.php @@ -6,6 +6,7 @@ use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Dev\SapphireTest; use TractorCow\Fluent\Extension\FluentSiteTreeExtension; +use TractorCow\Fluent\Extension\FluentVersionedExtension; use TractorCow\Fluent\Model\Domain; use TractorCow\Fluent\Model\Locale; use TractorCow\Fluent\State\FluentState; @@ -27,6 +28,8 @@ protected function setUp() // Clear cache Locale::clearCached(); Domain::clearCached(); + (new FluentVersionedExtension)->flushCache(); + FluentState::singleton() ->setLocale('en_NZ') ->setIsDomainMode(false); @@ -56,7 +59,6 @@ public function testExistsInLocale() $this->assertTrue($page->existsInLocale()); } - /** @group wip */ public function testSourceLocaleIsCurrentWhenPageExistsInIt() { // Read from the locale that the page exists in already @@ -65,4 +67,46 @@ public function testSourceLocaleIsCurrentWhenPageExistsInIt() $this->assertEquals('en_NZ', $page->getSourceLocale()->Locale); } + + public function testLocalisedStageCacheIsUsedForIsLocalisedInLocale() + { + /** @var Page $page */ + $page = $this->objFromFixture(Page::class, 'home'); + + /** @var FluentVersionedExtension $extension */ + $extension = $this->getMockBuilder(FluentVersionedExtension::class) + ->setMethods(['findRecordInLocale']) + ->getMock(); + $extension->setOwner($page); + + // We only expect one call to this method, because subsequent calls should be cached + $extension->expects($this->once())->method('findRecordInLocale')->willReturn('foo'); + + // Initial request + $result = $extension->isPublishedInLocale('en_NZ'); + $this->assertSame('foo', $result, 'Original method result is returned'); + + // Checking the cache + $result2 = $extension->isPublishedInLocale('en_NZ'); + $this->assertSame('foo', $result2, 'Cached result is returned'); + } + + public function testIdsInLocaleCacheIsUsedForIsLocalisedInLocale() + { + // Optimistically generate the cache + FluentVersionedExtension::prepoulateIdsInLocale('en_NZ', Page::class, true, true); + + /** @var Page $page */ + $page = $this->objFromFixture(Page::class, 'home'); + + /** @var FluentVersionedExtension $extension */ + $extension = $this->getMockBuilder(FluentVersionedExtension::class) + ->setMethods(['findRecordInLocale']) + ->getMock(); + $extension->setOwner($page); + + // We expect the lookup method to never get called, because the results are optimistically cached + $extension->expects($this->never())->method('findRecordInLocale'); + $this->assertTrue($extension->isPublishedInLocale('en_NZ'), 'Fixtured page is published'); + } }