Skip to content

Commit

Permalink
[BUGFIX] Speed up FormEngine with hundreds of categories
Browse files Browse the repository at this point in the history
FormEngine's TreeRendering for categories utilizes
the TreeDataProvider logic, which is handed over
a list of available items loaded by FormEngine's
AbstractItemProvider. These records have already
been validated and a full record was already loaded
by FormEngine at this point.

The TreeDataProvider construct then fetches ALL records
again, as it only works with the "IDs", but not with
already loaded full rows. This makes the construct
expensive. It needs a bigger overhaul.

To mitigate the performance impact of bigger category
trees, the patch applies a hack to suppress loading
single category rows multiple times by handing them
over from FormEngine to the TreeDataProvider construct.

As this change is also backported to 11.5, the
optimizations are only done for type=category to
minimize impact for other Tree usages.

Resolves: #84558
Resolves: #90398
Resolves: #97003
Resolves: #76581
Resolves: #98807
Releases: main, 11.5
Change-Id: Ief183f1673d3a17036ca5e68c62dc18a4277def1
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/76443
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Oliver Bartsch <bo@cedev.de>
Tested-by: Susanne Moog <look@susi.dev>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Susanne Moog <look@susi.dev>
Reviewed-by: Oliver Bartsch <bo@cedev.de>
Reviewed-by: Benni Mack <benni@typo3.org>
  • Loading branch information
bmack committed Nov 8, 2022
1 parent 98cc238 commit 971be90
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 194 deletions.
Expand Up @@ -237,12 +237,13 @@ protected function addItemsFromFolder(array $result, $fieldName, array $items)
* Used by TcaSelectItems and TcaSelectTreeItems data providers
*
* @param array $result Result array
* @param string $fieldName Current handle field name
* @param string $fieldName Current handled field name
* @param array $items Incoming items
* @param bool $includeFullRows @internal Hack for category tree to speed up tree processing, adding full db row as _row to item
* @return array Modified item array
* @throws \UnexpectedValueException
*/
protected function addItemsFromForeignTable(array $result, $fieldName, array $items)
protected function addItemsFromForeignTable(array $result, $fieldName, array $items, bool $includeFullRows = false)
{
$databaseError = null;
$queryResult = null;
Expand All @@ -265,7 +266,7 @@ protected function addItemsFromForeignTable(array $result, $fieldName, array $it
);
}

$queryBuilder = $this->buildForeignTableQueryBuilder($result, $fieldName);
$queryBuilder = $this->buildForeignTableQueryBuilder($result, $fieldName, $includeFullRows);
try {
$queryResult = $queryBuilder->executeQuery();
} catch (DBALException $e) {
Expand Down Expand Up @@ -320,12 +321,16 @@ protected function addItemsFromForeignTable(array $result, $fieldName, array $it
// Else, determine icon based on record type, or a generic fallback
$icon = $iconFactory->mapRecordTypeToIconIdentifier($foreignTable, $foreignRow);
}
// Add the item
$items[] = [
$labelPrefix . BackendUtility::getRecordTitle($foreignTable, $foreignRow),
$foreignRow['uid'],
$icon,
$item = [
0 => $labelPrefix . BackendUtility::getRecordTitle($foreignTable, $foreignRow),
1 => $foreignRow['uid'],
2 => $icon,
];
if ($includeFullRows) {
// @todo: This is part of the category tree performance hack
$item['_row'] = $foreignRow;
}
$items[] = $item;
}
}

Expand Down Expand Up @@ -527,16 +532,23 @@ static function (array $item) use ($allowedStorageIds) {
*
* @param array $result Result array
* @param string $localFieldName Current handle field name
* @return QueryBuilder
* @param bool $selectAllFields @internal True to select * all fields of row, otherwise an auto-calculated list.
* Select * is an optimization hack to speed up category tree calculation.
*/
protected function buildForeignTableQueryBuilder(array $result, string $localFieldName): QueryBuilder
protected function buildForeignTableQueryBuilder(array $result, string $localFieldName, bool $selectAllFields = false): QueryBuilder
{
$backendUser = $this->getBackendUser();

$foreignTableName = $result['processedTca']['columns'][$localFieldName]['config']['foreign_table'];
$foreignTableClauseArray = $this->processForeignTableClause($result, $foreignTableName, $localFieldName);

$fieldList = BackendUtility::getCommonSelectFields($foreignTableName, $foreignTableName . '.');
if ($selectAllFields) {
$fieldList = [$foreignTableName . '.*'];
} else {
$fieldList = BackendUtility::getCommonSelectFields($foreignTableName, $foreignTableName . '.');
$fieldList = GeneralUtility::trimExplode(',', $fieldList, true);
}

$queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)
->getQueryBuilderForTable($foreignTableName);

Expand All @@ -546,7 +558,7 @@ protected function buildForeignTableQueryBuilder(array $result, string $localFie
->add(GeneralUtility::makeInstance(WorkspaceRestriction::class, $this->getBackendUser()->workspace));

$queryBuilder
->select(...GeneralUtility::trimExplode(',', $fieldList, true))
->select(...$fieldList)
->from($foreignTableName)
->where($foreignTableClauseArray['WHERE']);

Expand Down
19 changes: 17 additions & 2 deletions typo3/sysext/backend/Classes/Form/FormDataProvider/TcaCategory.php
Expand Up @@ -86,14 +86,27 @@ public function addData(array $result): array
}

// Fetch the list of all possible "related" items and apply processing
$dynamicItems = $this->addItemsFromForeignTable($result, $fieldName, []);
// @todo: This uses 4th param 'true' as hack to add the full item rows speeding up
// processing of items in the tree class construct below. Simplify the construct:
// The entire $treeDataProvider / $treeRenderer / $tree construct should probably
// vanish and the tree processing could happen here in the data provider? Watch
// out for the permission event in the tree construct when doing this.
$dynamicItems = $this->addItemsFromForeignTable($result, $fieldName, [], true);
// Remove items as configured via TsConfig
$dynamicItems = $this->removeItemsByKeepItemsPageTsConfig($result, $fieldName, $dynamicItems);
$dynamicItems = $this->removeItemsByRemoveItemsPageTsConfig($result, $fieldName, $dynamicItems);
// Finally, the only data needed for the tree code are the valid uids of the possible records
$uidListOfAllDynamicItems = array_map('intval', array_filter(
array_values(array_column($dynamicItems, 1)),
static fn ($uid) => (int)$uid > 0
));
$fullRowsOfDynamicItems = [];
foreach ($dynamicItems as $item) {
// @todo: Prepare performance hack for tree calculation below.
if (isset($item['_row'])) {
$fullRowsOfDynamicItems[(int)$item['_row']['uid']] = $item['_row'];
}
}
// Initialize the tree data provider
$treeDataProvider = TreeDataProviderFactory::getDataProvider(
$result['processedTca']['columns'][$fieldName]['config'],
Expand All @@ -103,7 +116,9 @@ public function addData(array $result): array
);
$treeDataProvider->setSelectedList(implode(',', $result['databaseRow'][$fieldName]));
// Basically the tree data provider fetches all tree nodes again and
// then verifies if a given rows' uid is within the item whilelist.
// then verifies if a given rows' uid is within the item whitelist.
// @todo: Simplify construct, probably remove entirely. See @todo above as well.
$treeDataProvider->setAvailableItems($fullRowsOfDynamicItems);
$treeDataProvider->setItemWhiteList($uidListOfAllDynamicItems);
$treeDataProvider->initializeTreeData();
$treeRenderer = GeneralUtility::makeInstance(ArrayTreeRenderer::class);
Expand Down
17 changes: 16 additions & 1 deletion typo3/sysext/backend/Classes/Tree/TreeNode.php
Expand Up @@ -44,6 +44,11 @@ class TreeNode implements ComparableNodeInterface
*/
protected $childNodes;

/**
* @internal This is part of the category tree performance hack.
*/
protected array $additionalData = [];

/**
* Constructor
*
Expand Down Expand Up @@ -173,6 +178,14 @@ public function compareTo($other)
return $this->id > $other->getId() ? 1 : -1;
}

/**
* @internal This is part of the category tree performance hack
*/
public function getAdditionalData(): array
{
return $this->additionalData;
}

/**
* Returns the node in an array representation that can be used for serialization
*
Expand Down Expand Up @@ -205,7 +218,7 @@ public function toArray($addChildNodes = true)
*/
public function dataFromArray($data)
{
$this->setId($data['id']);
$this->setId($data['id'] ?? $data['uid']);
if (isset($data['parentNode']) && $data['parentNode'] !== '') {
/** @var TreeNode $parentNode */
$parentNode = GeneralUtility::makeInstance($data['parentNode']['serializeClassName'], $data['parentNode']);
Expand All @@ -216,6 +229,8 @@ public function dataFromArray($data)
$childNodes = GeneralUtility::makeInstance($data['childNodes']['serializeClassName'], $data['childNodes']);
$this->setChildNodes($childNodes);
}
// @todo: This is part of the category tree performance hack
$this->additionalData = $data;
}

/**
Expand Down
Expand Up @@ -73,6 +73,13 @@ abstract class AbstractTableConfigurationTreeDataProvider extends AbstractTreeDa
*/
protected $itemUnselectableList = [];

/**
* @todo: This is a hack to speed up category tree calculation. See the comments
* in TcaCategory and AbstractItemProvider FormEngine classes.
* @internal
*/
protected array $availableItems = [];

/**
* Sets the id of the tree
*
Expand Down Expand Up @@ -251,4 +258,12 @@ public function getItemUnselectableList()
{
return $this->itemUnselectableList;
}

/**
* @internal See property comment
*/
public function setAvailableItems(array $availableItems)
{
$this->availableItems = $availableItems;
}
}
Expand Up @@ -214,7 +214,12 @@ protected function buildRepresentationForNode(TreeNode $basicNode, ?DatabaseTree
$node->setExpanded(true);
$node->setLabel($this->getLanguageService()?->sL($GLOBALS['TCA'][$this->tableName]['ctrl']['title']));
} else {
$row = BackendUtility::getRecordWSOL($this->tableName, (int)$basicNode->getId(), '*', '', false) ?? [];
if ($basicNode->getAdditionalData() === []) {
$row = BackendUtility::getRecordWSOL($this->tableName, (int)$basicNode->getId(), '*', '', false) ?? [];
} else {
// @todo: This is part of the category tree performance hack
$row = $basicNode->getAdditionalData();
}
$node->setLabel(BackendUtility::getRecordTitle($this->tableName, $row) ?: $basicNode->getId());
$node->setSelected(GeneralUtility::inList($this->getSelectedList(), $basicNode->getId()));
$node->setExpanded($this->isExpanded($basicNode));
Expand Down Expand Up @@ -310,20 +315,12 @@ protected function getChildrenOf(TreeNode $node, int $level): ?TreeNodeCollectio
{
$nodeData = null;
if ($node->getId() !== 0 && $node->getId() !== '0') {
$queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)
->getQueryBuilderForTable($this->getTableName());
$queryBuilder->getRestrictions()->removeAll();
$nodeData = $queryBuilder->select('*')
->from($this->getTableName())
->where(
$queryBuilder->expr()->eq(
'uid',
$queryBuilder->createNamedParameter($node->getId(), Connection::PARAM_INT)
)
)
->setMaxResults(1)
->executeQuery()
->fetchAssociative();
if (is_array($this->availableItems[(int)$node->getId()] ?? false)) {
// @todo: This is part of the category tree performance hack
$nodeData = $this->availableItems[(int)$node->getId()];
} else {
$nodeData = BackendUtility::getRecord($this->tableName, $node->getId(), '*', '', false);
}
}
if (empty($nodeData)) {
$nodeData = [
Expand All @@ -336,7 +333,7 @@ protected function getChildrenOf(TreeNode $node, int $level): ?TreeNodeCollectio
if (!empty($children)) {
$storage = GeneralUtility::makeInstance(TreeNodeCollection::class);
foreach ($children as $child) {
$node = GeneralUtility::makeInstance(TreeNode::class);
$node = GeneralUtility::makeInstance(TreeNode::class, $this->availableItems[(int)$child] ?? []);
$node->setId($child);
if ($level < $this->levelMaximum) {
$children = $this->getChildrenOf($node, $level + 1);
Expand Down Expand Up @@ -386,7 +383,23 @@ protected function getChildrenUidsFromParentRelation(array $row): array
} elseif ($this->columnConfiguration['foreign_field'] ?? null) {
$relatedUids = $this->listFieldQuery($this->columnConfiguration['foreign_field'], $uid);
} else {
$relatedUids = $this->listFieldQuery($this->lookupField, $uid);
// Check available items
if ($this->availableItems !== [] && $this->columnConfiguration['type'] === 'category') {
// @todo: This is part of the category tree performance hack
$relatedUids = [];
foreach ($this->availableItems as $item) {
if ($item[$this->lookupField] === $uid) {
$relatedUids[$item['uid']] = $item['sorting'];
}
}
if ($relatedUids !== []) {
// Ensure sorting is kept
asort($relatedUids);
$relatedUids = array_keys($relatedUids);
}
} else {
$relatedUids = $this->listFieldQuery($this->lookupField, $uid);
}
}
} else {
$relatedUids = $this->listFieldQuery($this->lookupField, $uid);
Expand Down

0 comments on commit 971be90

Please sign in to comment.