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 Opt-in performance fix for many consecutive lookups using isLocalisedInStage #468

Merged
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 104 additions & 9 deletions src/Extension/FluentVersionedExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -58,7 +63,29 @@ class FluentVersionedExtension extends FluentExtension
*
* @var array
*/
protected $localisedStageCache = [];
protected static $localisedStageCache = [];

/**
* Array of objectIds keyed by table (ie. stage) and locale. This knows ALL object IDs that exist in the given table
* and locale.
*
* 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.
*
* static::$idsInLocaleCache[ $locale ][ $table(.self::SUFFIX_LIVE) ][ $objectId ] = $objectId
*
* @var int[][][]
*/
protected static $idsInLocaleCache = [];

/**
* Used to enable or disable the prepopulation of the locale content cache
* Defaults to true.
*
* @config
* @var boolean
*/
private static $prepopulate_localecontent_cache = true;

protected function augmentDatabaseDontRequire($localisedTable)
{
Expand Down Expand Up @@ -304,29 +331,97 @@ protected function isLocalisedInStage($stage, $locale = null)
$table .= self::SUFFIX_LIVE;
}

if (isset(static::$idsInLocaleCache[$locale][$table])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this isset check should include the owner's ID like the line below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I didn't is because we know all the IDs that exist in the locale for that table - the absence of an ID implies it's not there. So items that aren't in that stage/locale will skip this cache if you isset check the ID too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked this. Omitting owner ID drops another 105 queries... I'll PR to remove it again and we can continue discussion there 🙂

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();
$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;
static::$localisedStageCache[$key] = $result;
return $result;
}

public function flushCache()
{
$this->localisedStageCache = [];
static::$localisedStageCache = [];
}

/**
* Hook into {@link Hierarchy::prepopulateTreeDataCache}.
*
* @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 = [])
robbieaverill marked this conversation as resolved.
Show resolved Hide resolved
{
if (!Config::inst()->get(self::class, 'prepopulate_localecontent_cache')) {
return;
}

// 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) {
return;
}

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
*/
robbieaverill marked this conversation as resolved.
Show resolved Hide resolved
public static function prepoulateIdsInLocale($locale, $dataObjectClass, $populateLive = true, $populateDraft = true)
{
// Get the table for the given DataObject class
$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])) {
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] = array_combine($ids, $ids);
}
}
}