ENH More efficient hierarchy querying #11722

emteknetnz · 2025-05-07T00:00:18Z

Required for silverstripe/silverstripe-cms#3077

This will greatly reduce the number of queries when viewing the site tree

With 16006 pages testing on silverstripe/installer with https://github.com/emteknetnz/silverstripe-db-query-counter and https://github.com/emteknetnz/silverstripe-data-generator (and the silverstripe/cms PR)

I've pointed out the individual queries with the biggest reductions

Note that the before and after were both counted with https://github.com/silverstripe/silverstripe-cms/pull/3077/files#r2078854671 implemented, including on the "before" tests, so that queries from the redundant XHR request to /tree were not included

I ran flush before logging to ensure that caches were clear

/admin/pages:

Before
Total queries: 46

Count: 12 - Query: SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited" ... FROM "SiteTree" WHERE ("SiteTree"."ParentID" <> "SiteTree"."ID") AND ("SiteTree"."ParentID" = ?) ORDER BY "SiteTree"."Sort" ASC
+
Count: 12 - Query: SELECT DISTINCT "SiteTree_Live"."ClassName", "SiteTree_Live"."LastEdited"... WHERE ("SiteTree_Live"."ParentID" = ?) AND (("SiteTree_Live"."ID" != ? OR "SiteTree_Live"."ID" IS NULL)) AND ("SiteTree_Live"."ID" NOT IN (SELECT "ID" FROM "SiteTree")) ORDER BY "SiteTree_Live"."Sort" ASC FROM "SiteTree")) ORDER BY "SiteTree_Live"."Sort" ASC

After:
Total queries: 24

Count: 1 - Query: SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited"... FROM "SiteTree" WHERE ("SiteTree"."ParentID" IN (0)) ORDER BY "SiteTree"."Sort" ASC
+
Count: 1 - Query: SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited"... FROM "SiteTree" WHERE ("SiteTree"."ParentID" IN (7, 4112, 8217, 12322, 828, 4933, 9038, 13143, 1649, 5754, 9859, 13964, 2470, 6575, 10680, 14785, 3291, 7396, 11501, 15606, 1, 2, 3, 4, 5, 6)) ORDER BY "SiteTree"."Sort" ASC

/pages/edit/show/9 - page that is 3 levels deep

The difference is much more pronounced for nested pages

Before
Total queries: 386

Count: 95 - Query: SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited"... FROM "SiteTree" WHERE ("SiteTree"."ParentID" <> "SiteTree"."ID") AND ("SiteTree"."ParentID" = ?) ORDER BY "SiteTree"."Sort" ASC
+
Count: 95 - Query: SELECT DISTINCT "SiteTree_Live"."ClassName", "SiteTree_Live"."LastEdited"... FROM "SiteTree_Live" WHERE ("SiteTree_Live"."ParentID" = ?) AND (("SiteTree_Live"."ID" != ? OR "SiteTree_Live"."ID" IS NULL)) AND ("SiteTree_Live"."ID" NOT IN (SELECT "ID" FROM "SiteTree")) ORDER BY "SiteTree_Live"."Sort" ASC

After:
Total queries: 150

Count: 1 - Query: SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited"... FROM "SiteTree" WHERE ("SiteTree"."ParentID" IN (0)) ORDER BY "SiteTree"."Sort" ASC
+
Count: 1 - Query: SELECT DISTINCT "SiteTree"."ClassName", "SiteTree"."LastEdited"... FROM "SiteTree" WHERE ("SiteTree"."ParentID" IN (7, 4112, 8217, 12322, 828, 4933, 9038, 13143, 1649, 5754, 9859, 13964, 2470, 6575, 10680, 14785, 3291, 7396, 11501, 15606, 1, 2, 3, 4, 5, 6)) ORDER BY "SiteTree"."Sort" ASC

src/ORM/Hierarchy/Hierarchy.php

emteknetnz · 2025-05-07T00:01:53Z

tests/php/ORM/HierarchyTest.yml

@@ -43,7 +43,11 @@ SilverStripe\ORM\Tests\HierarchyTest\TestObject:
    Title: Obj 3bb
  obj3ca:
    Parent: =>SilverStripe\ORM\Tests\HierarchyTest\TestObject.obj3c
-    Title: Obj 3c
+    Title: Obj 3ca


This fixture had the wrong title before

emteknetnz · 2025-05-07T00:02:02Z

tests/php/ORM/HierarchyTest/TestObjectHideFromHierarchy.php.php

+use SilverStripe\ORM\Hierarchy\Hierarchy;
+use SilverStripe\Versioned\Versioned;
+
+class TestObjectHideFromHierarchy extends TestObject implements TestOnly


There is an existing test object HideTestObject used in different unit tests for testing $hide_from_hierarchy, however I couldn't use that class as it extends DataObject instead of TestObject

emteknetnz · 2025-05-07T00:02:14Z

src/Forms/TreeDropdownField.php

     *
     * @var string
     */
-    protected $childrenMethod = 'AllChildrenIncludingDeleted';
+    protected $childrenMethod = null;


There are existing getter/setters on this class for setting this property

emteknetnz · 2025-05-07T01:41:09Z

tests/php/ORM/MarkedSetTest.php

@@ -18,6 +18,8 @@ class MarkedSetTest extends SapphireTest

    protected static $extra_dataobjects = [
        HierarchyTest\TestObject::class,
+        HierarchyTest\HierarchyOnSubclassTestObject::class,
+        HierarchyTest\HierarchyOnSubclassTestSubObject::class,


These needed to be added or else we'd now start getting the following when running MarkedSetTest on local

SilverStripe\ORM\Connect\DatabaseException: Couldn't run query: INSERT INTO "HierarchyOnSubclassTest_Object" ("ClassName", "Title", "LastEdited", "Created") VALUES (?, ?, ?, ?) Table 'ss_tmpdb_1746582023_8776611.HierarchyOnSubclassTest_Object' doesn't exist

emteknetnz · 2025-05-08T04:51:44Z

src/ORM/Hierarchy/Hierarchy.php

+     * - Database queries will be larger `WHERE "ID" IN (?, ?) style queries rather than many `WHERE "ID" = ?
+     * - While Hierarchy.node_threshold_total has not been exceeded, it will cache descendant record as
+     *   well to save future queries
+     * - Does not included deleted records, which could be done via Versioned::updateListToAlsoIncludeDeleted($children)


I ended up removing the call to Versioned::updateListToAlsoIncludeDeleted($children);, and renaming the method to simply getAllChildren() (removing the "including deleted" bit) after a behat test failed

"Deleted" records (i.e. those that only exist in the _Versions table, not in either the draft or _Live tables, will display with a strikethrough line through it and with the 'archived' badge next to them

In the CMS UI, before and after this PR:

when you archive a record, it will immediately appear in the site tree as archived

when you immediately click on another node in the site tree, it will XHR load from /getsubtree another node, and the strikethrough line + archived badge remains in place. I believe this is done purely with javascript and not via XHR'ing in new data.

If you refresh the whole page, then the archived page will no longer display in the site tree and there is no way to show it

If we did include the call to Versioned::updateListToAlsoIncludeDeleted($children), then archived pages would persist in the site tree which we definitely do not want

SimulatedPanda · 2025-05-08T20:15:32Z

I approve the inclusion of this work in CMS v6.0 when all necessary reviews have been completed and any issues have been addressed.

emteknetnz · 2025-05-09T04:40:32Z

tests/php/ORM/MarkedSetTest.php

@@ -105,12 +107,14 @@ public function testMarkChildrenDoesntUnmarkPreviouslyMarked()
    </li>
    <li>Obj 3c
        <ul>
-            <li>Obj 3c
+            <li>Obj 3ca


This fixture had the wrong title before

GuySartorelli

This change needs a changelog entry and updates to relevant other documentation.

src/Forms/TreeDropdownField.php

src/ORM/Hierarchy/Hierarchy.php

src/ORM/Hierarchy/MarkedSet.php

src/ORM/Hierarchy/Hierarchy.php

src/Forms/TreeDropdownField.php

src/ORM/Hierarchy/MarkedSet.php

emteknetnz · 2025-05-15T03:14:10Z

src/ORM/Hierarchy/MarkedSet.php

@@ -47,12 +48,12 @@ class MarkedSet
    public $markingFilter;

    /**
-     * @var DataObject
+     * @var DataObject&Hierarchy


There's a check in the constructor that the DataObject that's assigned to $this->rootNode has the Hierarchy extension, and there's no other assignements to this property.

src/ORM/Hierarchy/Hierarchy.php

GuySartorelli · 2025-05-16T01:28:35Z

This feels a little risky post-beta after all the regression testing has already been done - there's a reasonable chance of regressions with this change.

That said, from my testing (without any database latency) the performance seems about the same (so no performance regression I can find), and try as I might I can't get it to behave differently from an installation without this PR, so it's probably safe.

emteknetnz mentioned this pull request May 7, 2025

Reduce the number of database queries part 1 #11703

Closed

3 tasks

emteknetnz marked this pull request as draft May 7, 2025 00:01

emteknetnz commented May 7, 2025

View reviewed changes

src/ORM/Hierarchy/Hierarchy.php Outdated Show resolved Hide resolved

emteknetnz commented May 7, 2025

View reviewed changes

emteknetnz mentioned this pull request May 7, 2025

ENH More efficient hierarchy querying #11720

Closed

emteknetnz force-pushed the pulls/6.0/db-num branch from d70c80e to 9d8d9cb Compare May 7, 2025 01:34

emteknetnz commented May 7, 2025

View reviewed changes

emteknetnz force-pushed the pulls/6.0/db-num branch 4 times, most recently from 2c35dd7 to bb4dd87 Compare May 8, 2025 02:22

emteknetnz commented May 8, 2025

View reviewed changes

emteknetnz commented May 9, 2025

View reviewed changes

emteknetnz marked this pull request as ready for review May 9, 2025 05:16

GuySartorelli requested changes May 14, 2025

View reviewed changes

emteknetnz force-pushed the pulls/6.0/db-num branch 4 times, most recently from cfc4208 to 281e646 Compare May 14, 2025 08:26

GuySartorelli requested changes May 15, 2025

View reviewed changes

emteknetnz force-pushed the pulls/6.0/db-num branch from 281e646 to 8ccd0b7 Compare May 15, 2025 03:12

emteknetnz commented May 15, 2025

View reviewed changes

GuySartorelli requested changes May 15, 2025

View reviewed changes

src/ORM/Hierarchy/Hierarchy.php Outdated Show resolved Hide resolved

src/ORM/Hierarchy/Hierarchy.php Outdated Show resolved Hide resolved

src/ORM/Hierarchy/Hierarchy.php Outdated Show resolved Hide resolved

emteknetnz force-pushed the pulls/6.0/db-num branch from 8ccd0b7 to 8e22deb Compare May 15, 2025 22:31

ENH More efficient hierarchy querying

17616c2

emteknetnz force-pushed the pulls/6.0/db-num branch from 8e22deb to 17616c2 Compare May 15, 2025 22:37

GuySartorelli approved these changes May 16, 2025

View reviewed changes

GuySartorelli merged commit 28e192f into silverstripe:6.0 May 16, 2025
12 checks passed

GuySartorelli deleted the pulls/6.0/db-num branch May 16, 2025 01:29

ENH More efficient hierarchy querying #11722

ENH More efficient hierarchy querying #11722

Uh oh!

Conversation

emteknetnz commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

emteknetnz May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emteknetnz May 7, 2025

Choose a reason for hiding this comment

Uh oh!

emteknetnz May 7, 2025

Choose a reason for hiding this comment

Uh oh!

emteknetnz May 7, 2025

Choose a reason for hiding this comment

Uh oh!

emteknetnz May 8, 2025

Choose a reason for hiding this comment

Uh oh!

SimulatedPanda commented May 8, 2025

Uh oh!

emteknetnz May 9, 2025

Choose a reason for hiding this comment

Uh oh!

GuySartorelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emteknetnz May 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuySartorelli commented May 16, 2025

Uh oh!

Uh oh!

Uh oh!

emteknetnz commented May 7, 2025 •

edited

Loading

emteknetnz May 7, 2025 •

edited

Loading