From 3c8bd95c6cd59bae28bfaf233d02d9ae12c786da Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Thu, 12 Sep 2024 13:22:26 +0200 Subject: [PATCH 1/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance with custom pimcore.admin.object.list.beforeListLoad listener --- .../AdminBundle/Resources/public/js/pimcore/overrides.js | 8 ++++++++ .../Resources/public/js/pimcore/treenodelocator.js | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/bundles/AdminBundle/Resources/public/js/pimcore/overrides.js b/bundles/AdminBundle/Resources/public/js/pimcore/overrides.js index f30ce1058d4..41691dd16b5 100644 --- a/bundles/AdminBundle/Resources/public/js/pimcore/overrides.js +++ b/bundles/AdminBundle/Resources/public/js/pimcore/overrides.js @@ -410,6 +410,14 @@ Ext.define('pimcore.data.PagingTreeStore', { me.superclass.onProxyLoad.call(this, operation); var proxy = this.getProxy(); proxy.setExtraParam("start", 0); + + // CANDO OPTIMIZATION START + // this line will reset the childNodeId to make sure it'll not interfere with other proxy requests + // childNodeId will be set in the treenodelocator.js and can be used within a custom listener + // to improve the performance on the client side + proxy.setExtraParam('childNodeId', null); + // CANDO OPTIMIZATION END + } catch (e) { console.log(e); } diff --git a/bundles/AdminBundle/Resources/public/js/pimcore/treenodelocator.js b/bundles/AdminBundle/Resources/public/js/pimcore/treenodelocator.js index e5ed82d3541..0fec1d85c54 100644 --- a/bundles/AdminBundle/Resources/public/js/pimcore/treenodelocator.js +++ b/bundles/AdminBundle/Resources/public/js/pimcore/treenodelocator.js @@ -421,6 +421,13 @@ pimcore.treenodelocator = function() proxy.setExtraParam("start", pagingState.offset); node.pagingData.offset = pagingState.offset; + // CANDO OPTIMIZATION START + // this line will set the childNodeId to add it to the backend request and can be used within + // a custom listener with the event pimcore.admin.object.list.beforeListLoad to improve the performance + // childNodeId will be reset in the treenodelocator.js + proxy.setExtraParam('childNodeId', pagingState.childNodeId); + // CANDO OPTIMIZATION END + store.load({ node: node, callback: self.processPaging From d056625a82ba4904e75e3054b9845c5f215a560f Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Fri, 13 Sep 2024 08:56:54 +0200 Subject: [PATCH 2/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance within the DataObject\DataObjectController.php --- .../Admin/DataObject/DataObjectController.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php index ee4d9a36194..4d23f17c4d2 100644 --- a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php +++ b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php @@ -147,6 +147,25 @@ public function treeGetChildsByIdAction(Request $request, EventDispatcherInterfa /** @var DataObject\Listing $childrenList */ $childrenList = $beforeListLoadEvent->getArgument('list'); + // CANDO OPTIMIZATION START + // do special logic to reduce amount of request by locating an object in a tree with huge amount of items + // basically it will clone the specified list and calculate the count of items to the current child + // based on that, the page and afterward, the offset can be calculated + $childNodeId = $request->get('childNodeId', null); + if (!empty($childNodeId)) { + // find offset + $offset = 0; + $clonedChildList = clone $childrenList; + $clonedChildList->setOffset($offset); + $clonedChildList->addConditionParam('o_key < ( SELECT o_key FROM objects WHERE o_id = ? )', $childNodeId); + $totalCountOfPreviousItems = $clonedChildList->getTotalCount(); + $page = (int) ceil($totalCountOfPreviousItems / $limit); + $offset = (int) ($page - 1) * $limit; + // reset offset + $childrenList->setOffset($offset); + } + // CANDO OPTIMIZATION END + $children = $childrenList->load(); $filteredTotalCount = $childrenList->getTotalCount(); From 3ac75bca5cb4c9879cb13166d5fe005311a07bdd Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Fri, 13 Sep 2024 10:33:51 +0200 Subject: [PATCH 3/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance within the DataObject\DataObjectController.php --- .../Admin/DataObject/DataObjectController.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php index 4d23f17c4d2..e792111d581 100644 --- a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php +++ b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php @@ -157,7 +157,17 @@ public function treeGetChildsByIdAction(Request $request, EventDispatcherInterfa $offset = 0; $clonedChildList = clone $childrenList; $clonedChildList->setOffset($offset); - $clonedChildList->addConditionParam('o_key < ( SELECT o_key FROM objects WHERE o_id = ? )', $childNodeId); + if ($object->getChildrenSortBy() === 'index') { + $clonedChildList->addConditionParam( + 'o_index < ( SELECT o_index FROM objects WHERE o_id = ? )', + $childNodeId + ); + } else { + $clonedChildList->addConditionParam( + 'o_key < ( SELECT o_key FROM objects WHERE o_id = ? )', + $childNodeId + ); + } $totalCountOfPreviousItems = $clonedChildList->getTotalCount(); $page = (int) ceil($totalCountOfPreviousItems / $limit); $offset = (int) ($page - 1) * $limit; From 7bd0174686c77261edfc13300d1fec15b9280e1d Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Fri, 13 Sep 2024 11:57:48 +0200 Subject: [PATCH 4/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance within the DataObject\DataObjectController.php --- .../Controller/Admin/DataObject/DataObjectController.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php index e792111d581..9deddb526d9 100644 --- a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php +++ b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php @@ -1189,9 +1189,14 @@ private function executeUpdateAction(DataObject $object, mixed $values): array $object->save(); - if ($isIndexUpdate) { + // CANDO OPTIMIZATION START + // do update indexes only if parents childrenSortBy is set to index + // sort by index cannot be set on nodes with paged children anyway + // it doesn't make sense to sort by index on large amount of data in one node + if ($isIndexUpdate && $parent->getChildrenSortBy() === 'index') { $this->updateIndexesOfObjectSiblings($object, $indexUpdate); } + // CANDO OPTIMIZATION END $success = true; } catch (\Exception $e) { From 3f567dc894f72984b54f3ed212bba9a2861b547f Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Wed, 25 Sep 2024 14:52:14 +0200 Subject: [PATCH 5/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance within the DataObject\DataObjectController.php --- .../Admin/DataObject/DataObjectController.php | 193 +++++++++++------- 1 file changed, 123 insertions(+), 70 deletions(-) diff --git a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php index 9deddb526d9..8d33266b2db 100644 --- a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php +++ b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php @@ -118,79 +118,32 @@ public function treeGetChildsByIdAction(Request $request, EventDispatcherInterfa $limit = 100; } - $childrenList = new DataObject\Listing(); - $childrenList->setCondition($this->buildChildrenCondition($object, $filter, $view)); - $childrenList->setLimit($limit); - $childrenList->setOffset($offset); - - if ($object->getChildrenSortBy() === 'index') { - $childrenList->setOrderKey('objects.o_index ASC', false); - } else { - $childrenList->setOrderKey( - sprintf( - 'CAST(objects.o_%s AS CHAR CHARACTER SET utf8) COLLATE utf8_general_ci %s', - $object->getChildrenSortBy(), $object->getChildrenSortOrder() - ), - false - ); - } - $childrenList->setObjectTypes($objectTypes); - - Element\Service::addTreeFilterJoins($cv, $childrenList); - - $beforeListLoadEvent = new GenericEvent($this, [ - 'list' => $childrenList, - 'context' => $allParams, - ]); - $eventDispatcher->dispatch($beforeListLoadEvent, AdminEvents::OBJECT_LIST_BEFORE_LIST_LOAD); - - /** @var DataObject\Listing $childrenList */ - $childrenList = $beforeListLoadEvent->getArgument('list'); - // CANDO OPTIMIZATION START - // do special logic to reduce amount of request by locating an object in a tree with huge amount of items - // basically it will clone the specified list and calculate the count of items to the current child - // based on that, the page and afterward, the offset can be calculated + // Because of the tree node locator is searching the tree with several pages per node with a binary search, + // which is triggered by the Admin UI JS. If there is a node with thousands of items, it makes a new request + // for every halving until the searched item was found between the offset and limit. + // To reduce the amount of requests this fix will get ID of the child node. If there are more items in the + // node as the limit it will recursive iterate through every page till the correct page is found. + // The original part of code is refactored into a separate function, which can be called recursively. $childNodeId = $request->get('childNodeId', null); - if (!empty($childNodeId)) { - // find offset - $offset = 0; - $clonedChildList = clone $childrenList; - $clonedChildList->setOffset($offset); - if ($object->getChildrenSortBy() === 'index') { - $clonedChildList->addConditionParam( - 'o_index < ( SELECT o_index FROM objects WHERE o_id = ? )', - $childNodeId - ); - } else { - $clonedChildList->addConditionParam( - 'o_key < ( SELECT o_key FROM objects WHERE o_id = ? )', - $childNodeId - ); - } - $totalCountOfPreviousItems = $clonedChildList->getTotalCount(); - $page = (int) ceil($totalCountOfPreviousItems / $limit); - $offset = (int) ($page - 1) * $limit; - // reset offset - $childrenList->setOffset($offset); - } + $result = $this->getChildrenList( + $eventDispatcher, + $object, + $objectTypes, + $allParams, + $limit, + $offset, + $filter, + $view, + $cv, + $childNodeId + ); + // get the children list data and map them to the correct variables + $objects = $result['objects']; + $filteredTotalCount = $result['filteredTotalCount']; + $offset = $result['offset']; + $total = $result['total']; // CANDO OPTIMIZATION END - - $children = $childrenList->load(); - $filteredTotalCount = $childrenList->getTotalCount(); - - foreach ($children as $child) { - $objectTreeNode = $this->getTreeNodeConfig($child); - // this if is obsolete since as long as the change with #11714 about list on line 175-179 are working fine, we already filter the list=1 there - if ($objectTreeNode['permissions']['list'] == 1) { - $objects[] = $objectTreeNode; - } - } - - //pagination for custom view - $total = $cv - ? $filteredTotalCount - : $object->getChildAmount(null, $this->getAdminUser()); } //Hook for modifying return value - e.g. for changing permissions based on object data @@ -218,6 +171,106 @@ public function treeGetChildsByIdAction(Request $request, EventDispatcherInterfa return $this->adminJson($objects); } + // CANDO OPTIMIZATION START + private function getChildrenList( + EventDispatcherInterface $eventDispatcher, + DataObject $object, + array $objectTypes, + array $allParams, + int $limit, + int $offset, + ?string $filter, + ?string $view, + ?array $cv, + ?string $childNodeId + ): array { + $objects = []; + $childrenList = new DataObject\Listing(); + $childrenList->setCondition($this->buildChildrenCondition($object, $filter, $view)); + $childrenList->setLimit($limit); + $childrenList->setOffset($offset); + + if ($object->getChildrenSortBy() === 'index') { + $childrenList->setOrderKey('objects.o_index ASC', false); + } else { + $childrenList->setOrderKey( + sprintf( + 'CAST(objects.o_%s AS CHAR CHARACTER SET utf8) COLLATE utf8_general_ci %s', + $object->getChildrenSortBy(), $object->getChildrenSortOrder() + ), + false + ); + } + $childrenList->setObjectTypes($objectTypes); + + Element\Service::addTreeFilterJoins($cv, $childrenList); + + $beforeListLoadEvent = new GenericEvent($this, [ + 'list' => $childrenList, + 'context' => $allParams, + ]); + $eventDispatcher->dispatch($beforeListLoadEvent, AdminEvents::OBJECT_LIST_BEFORE_LIST_LOAD); + + /** @var DataObject\Listing $childrenList */ + $childrenList = $beforeListLoadEvent->getArgument('list'); + + $children = $childrenList->load(); + $filteredTotalCount = $childrenList->getTotalCount(); + + // only execute the recursive call if the childNodeId is set + if (!empty($childNodeId)) { + // check if childNodeId is in current list, if not do it again by recalling this function + $found = false; + foreach ($children as $child) { + if ($child->getId() === $childNodeId) { + $found = true; + } + } + + // if the child node is not found, then call this function again + if (!$found) { + $nextOffset = $offset + $limit; + // if next offset is higher than the filtered total count, then the item was not found at all + if ($filteredTotalCount > $nextOffset) { + return $this->getChildrenList( + $eventDispatcher, + $object, + $objectTypes, + $allParams, + $limit, + $nextOffset, + $filter, + $view, + $cv, + $childNodeId + ); + } + } + } + + foreach ($children as $child) { + $objectTreeNode = $this->getTreeNodeConfig($child); + // this if is obsolete since as long as the change with #11714 about list on line 175-179 are working fine, we already filter the list=1 there + if ($objectTreeNode['permissions']['list'] == 1) { + $objects[] = $objectTreeNode; + } + } + + //pagination for custom view + $total = $cv + ? $filteredTotalCount + : $object->getChildAmount(null, $this->getAdminUser()); + + // return all calculated values to the calling function as an array + return [ + 'objects' => $objects, + 'filteredTotalCount' => $filteredTotalCount, + 'offset' => $offset, + 'total' => $total, + ]; + } + // CANDO OPTIMIZATION END + /** * @param DataObject\AbstractObject $object * @param string|null $filter From f26e07d040b744942cdba8f386e76b4763c092b5 Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Wed, 25 Sep 2024 16:04:17 +0200 Subject: [PATCH 6/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance within the DataObject\DataObjectController.php --- .../Controller/Admin/DataObject/DataObjectController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php index 8d33266b2db..a09b7ca801f 100644 --- a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php +++ b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php @@ -222,8 +222,9 @@ private function getChildrenList( // check if childNodeId is in current list, if not do it again by recalling this function $found = false; foreach ($children as $child) { - if ($child->getId() === $childNodeId) { + if ($child->getId() === (int) $childNodeId) { $found = true; + break; } } From 9e9818b27a5d02f80a640bad9b4f7966e4998d24 Mon Sep 17 00:00:00 2001 From: Armin Pleisch Date: Thu, 26 Sep 2024 08:17:20 +0200 Subject: [PATCH 7/7] 10.6 Fix to set/reset additional parameter on the tree node locator to make the possibility to improve the performance within the DataObject\DataObjectController.php --- .../Controller/Admin/DataObject/DataObjectController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php index a09b7ca801f..6f083883be7 100644 --- a/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php +++ b/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php @@ -181,7 +181,7 @@ private function getChildrenList( int $offset, ?string $filter, ?string $view, - ?array $cv, + array|bool|null $cv, ?string $childNodeId ): array { $objects = [];