NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380

sminnee · 2018-09-18T01:22:14Z

Adds the ability to prepopulate the cache for Hierarchy::numChildren()
in a batch.

This is still a WIP as it will give bad results if augmentStageChildren
is used in a way that alters the number of records.

This is an useful part of optimising the query count on tree generation.
See #8379

Parent issue

/treeview endpoint is slow when a lot of nodes are in the tree silverstripe-cms#2250

Only relevant if silverstripe/silverstripe-framework#8380 is avialable, however coded defensively so it can be merged before that PR if needs be. See silverstripe/silverstripe-framework#8379

sminnee · 2018-09-18T01:26:04Z

Note: although this didn't have an order-of-magnitude impact on performance locally (reducing treeview execution from 3.5s to 2.5s), it did reduce the query count from ~140 to ~40. This should have a greater impact on deployments where the database is running on a separate server.

maxime-rainville

The change seems to be altering the results in some way. When I enable prepopulate_numchildren_cache(), I get a bunch of Too many pages ( show as list ) warnings in my treeview that shouldn't be there.

For what it's worth, it's noticeably faster.

sminnee · 2018-09-20T02:45:29Z

OK there's an issue with this: it conflicts with the lumberjack module because the lumberjack module overrides stageChildren. This causes the issue that Max found.

I'm going to see if I can amend the prepopulation logic to use a call to stageChildren as the basis of its query.

sminnee · 2018-09-20T03:59:52Z

Hrm, so I looked into this, and I wonder if we declare the problem to be with lumberjack: silverstripe/silverstripe-lumberjack#86

sminnee · 2018-09-20T04:02:40Z

You could for example do this instead:

maxime-rainville · 2018-09-20T08:11:05Z

Just tweaks Sam's logic to avoid counting the children of childless object when the cache is complete.

We're now getting a pretty solid 28% performance boost when rendering treeview. 😎

robbieaverill · 2018-09-20T08:41:39Z

You could for example do this instead:

The suspense is killing me

maxime-rainville · 2018-09-20T12:05:46Z

@robbieaverill Our key project will need to add this to their config:

SilverStripe\CMS\Model\SiteTree:
  hide_from_cms_tree:
  - App\Pages\RandomPageTypeOne
  - App\Pages\RandomPageTypeTwo
  - App\Pages\RandomPageTypeThree

I think that's what is meant to be in Sam's comment.

sminnee · 2018-09-20T22:05:38Z

Your amendment looks good, Maxime. I think that there's one thing we want to note in the changelog:

Take care with stageChildren() overrides. Hierarchy::numChildren() results will only make use of stageChildren() results that are applied to the base class and don't include record-specific behaviour.

This issue has been addressed.

sminnee · 2018-09-21T03:05:23Z

@silverstripe/core-team can we get a sanity check on whether the change described in my most recent comment is okay for semver?

chillu · 2018-09-21T04:27:55Z

@silverstripe/core-team can we get a sanity check on whether the change described in my most recent comment is okay for semver?

I've done a sanity check via Github for stageChildren, and it brings up a few use cases: https://github.com/search?l=PHP&q=%22function+stageChildren%22+dataobject&type=Code. Seems to be pretty much exclusively symbiote's various "connector" modules, and then a whole bunch of core code copied into other git repos.

The base module of this isn't even 4.x compatible (https://github.com/nyeholt/silverstripe-external-content), so I don't think it'll cause wider issues in practice - but looping in the maintainer @nyeholt in here as a courtesy either way.

This helper is necessary when you want to replace a parameter with a non-literal such as a column name. This arises when you are transforming queries such as we do in the prepopulation query for Hierarchy::numChildren()

API: Hierarchy::stageChildren() customisations must be applied to the base class and not include record-specific behaviour. Adds the ability to prepopulate the cache for Hierarchy::numChildren() in a batch. Note that this optimisation means that stageChildren() is not called on each record in order to calculate numChildren(). This means that the structure of the stageChildren() query must be the same for all records and the behaviour cannot be customised only for a subclass of the base data class. For example, apply your customisations to SiteTree and not a subclass. This is an useful part of optimising the query count on tree generation. See #8379

This will avoid trying to count the children of childless object.

sminnee · 2018-09-21T06:03:07Z

OK I've rebased this and added a note to the 4.3 changelog.

Only relevant if silverstripe/silverstripe-framework#8380 is avialable, however coded defensively so it can be merged before that PR if needs be. See silverstripe/silverstripe-framework#8379

maxime-rainville · 2018-09-23T12:15:11Z

I've got the unit tests written, but there's an edge case that's being tripped that I didn't quite figure out how to resolve yet. I'll have another look at in the morning.

prepopulate_numchildren_cache doesn't honor the hide_from_hierarchy config.

There's also a bit of an issue with unit testing the hide_from_cms_tree config flag because it only gets triggered when Hierachy::showingCMSTree returns true. This method is based on the current controller being LeftAndMain and the current action being one of "treeview", "listview", "getsubtree". FYI I'm very judgemental of this method!

src/ORM/Hierarchy/Hierarchy.php

tests/php/ORM/HierarchyCachingTest.php

sminnee · 2018-09-24T00:03:08Z

@ScopeyNZ I agree with the concerns about the DB::replace_parameter implementation, but a couple of notes:

The implementation was copied from DB::inline_parameters(), rather than written from scratch.
The "real fix" to its necessity is probably an SS5 topic such as SS5 RFC: More batch-oriented ORM #8397

sminnee · 2018-09-24T00:09:25Z

prepopulate_numchildren_cache doesn't honor the hide_from_hierarchy config.

This surprises me. Is the config applied to SiteTree or a subclass?

sminnee · 2018-09-24T00:10:19Z

tests/php/ORM/HierarchyCachingTest.php

+    public static function setUpBeforeClass()
+    {
+        parent::setUpBeforeClass();
+        HideTestObject::config()->update(


Can you apply this to SiteTree instead?

I'd rather not as it ought to work with this and that's how the other Hierarchy test are using.

sminnee · 2018-09-24T00:13:37Z

@maxime-rainville do you think I should merge the #8395 changes into this one?

The relevant changes are already in silverstripe/silverstripe-cms#2266

Simplify prepopulate_numchildren_cache by adding a $IDLess param to stageChildren.

maxime-rainville · 2018-09-24T03:09:31Z

Sorry, I bulked down and didn't see all the messages flying.

The problem was that some of the where statements in the loop where getting sent back as arrays and other were being sent back as DataQuery_SubGroup. The DataQuery_SubGroup were just being skipped altogether.

I just took some liberties with prepopulate_numchildren_cache logic. I added a flag to stageChildren that will suppress the ID/ParentID where clauses when set to true. I'm using that instead of looping through all the where clause.

I think it makes the logic a bit easier to follow and a bit cleaner and the generate SQL statement looks legit.

SELECT DISTINCT "ParentID", COUNT(DISTINCT "SiteTree"."ID") AS "NumChildren" FROM "SiteTree" LEFT JOIN "SiteTree_Localised" AS "SiteTree_Localised_en_NZ" ON "SiteTree"."ID" = "SiteTree_Localised_en_NZ"."RecordID" AND "SiteTree_Localised_en_NZ"."Locale" = ? WHERE (("SiteTree"."ClassName" NOT IN (?, ?, ?, ?) OR "SiteTree"."ClassName" IS NULL)) GROUP BY ParentID

However, it probably wouldn't hurt to have someone who actually knows what they're doing have another look at it.

On the topic #8395, it probably would be better to fold it into this one, as rebasing it at this stage would probably be more trouble than its worth.

maxime-rainville

I think it's in a good state now, but I ended up doing a big chunk of it, so it probably would be good for some one else to glance at it.

src/ORM/Hierarchy/Hierarchy.php

sminnee · 2018-09-24T04:44:28Z

src/ORM/Hierarchy/Hierarchy.php

-                }
-            }
-        }
+        $query = $dummyObject->stageChildren(true, true)->dataQuery()->query();


We can probably stop the implementation of DB::replace_parameter if we go with this, since it's a bit gross and was only added for this use-case.

We need to add "Where ID != ParentID" as a filter here.

I've added the Where ID != ParentID statement to stageChildren.

sminnee · 2018-09-24T04:46:23Z

src/ORM/Hierarchy/Hierarchy.php

-        $query->setGroupBy([
-            "ParentID
-        "]);
+        $query->setGroupBy(['"ParentID"']);


It's probably best to use Convert::symbol2sql("ParentID") here.

sminnee · 2018-09-24T04:47:14Z

@maxime-rainville I've left a few comments — do you want to address those, then I'll rebase/squash and maybe we can get Ingo to do the final merge?

This provides a more extensible way of preopulating caches for optimised tree generation. Fixes #8391

tests/php/ORM/HierarchyCachingTest.php

maxime-rainville · 2018-09-24T07:01:58Z

All done.

* FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation Only relevant if silverstripe/silverstripe-framework#8380 is avialable, however coded defensively so it can be merged before that PR if needs be. See silverstripe/silverstripe-framework#8379 * FIX: Use Hierarchy::prepopulateTreeDataCache() in CMS. Requires silverstripe/silverstripe-framework#8395 * Cache tree_class instead of assuming it will always be SiteTree.

sminnee mentioned this pull request Sep 18, 2018

FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation silverstripe/silverstripe-cms#2266

Merged

maxime-rainville self-assigned this Sep 18, 2018

maxime-rainville previously requested changes Sep 18, 2018

View reviewed changes

robbieaverill added the affects/v4 label Sep 20, 2018

Sam Minnee and others added 3 commits September 21, 2018 17:57

NEW: Add DB::replace_parameter()

1d81ecb

This helper is necessary when you want to replace a parameter with a non-literal such as a column name. This arises when you are transforming queries such as we do in the prepopulation query for Hierarchy::numChildren()

Add a _complete flag prepopulate_numchildren_cache

574918c

This will avoid trying to count the children of childless object.

sminnee mentioned this pull request Sep 21, 2018

Refactor tree-generation prepopulation #8391

Closed

4 tasks

Maxime Rainville added 2 commits September 21, 2018 23:06

Write some test for DB::replace_parameter

cc7769a

Write unit test for testing Hierarchy caching.

dd436f5

maxime-rainville reviewed Sep 23, 2018

View reviewed changes

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

ScopeyNZ suggested changes Sep 23, 2018

View reviewed changes

tests/php/ORM/HierarchyCachingTest.php Outdated Show resolved Hide resolved

maxime-rainville mentioned this pull request Sep 23, 2018

Refactor prepopulation #8395

Closed

Switch from preg_split to explode for DB::replace_parameter

07f9cdc

sminnee commented Sep 24, 2018

View reviewed changes

Simplify Hierarchy::prepopulate_numchildren_cache

700b5da

Simplify prepopulate_numchildren_cache by adding a $IDLess param to stageChildren.

Wrap GroupBy in double quotes for Postgresql

76f7017

maxime-rainville approved these changes Sep 24, 2018

View reviewed changes

sminnee commented Sep 24, 2018

View reviewed changes

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

sminnee commented Sep 24, 2018

View reviewed changes

NEW: Add Hierarchy::prepopulateTreeDataCache()

c352a26

This provides a more extensible way of preopulating caches for optimised tree generation. Fixes #8391

sminnee commented Sep 24, 2018

View reviewed changes

tests/php/ORM/HierarchyCachingTest.php Outdated Show resolved Hide resolved

Maxime Rainville added 3 commits September 24, 2018 18:46

Applying feedback from Peer review.

9ca1f75

Remove DB::replace_parameter

e4a7c43

Attach table name the stageChildren where ParentID <> ID clause

ff92963

ScopeyNZ approved these changes Sep 25, 2018

View reviewed changes

maxime-rainville merged commit 5b7a841 into silverstripe:4 Sep 25, 2018

robbieaverill mentioned this pull request Sep 25, 2018

FIX Opt-in performance fix for many consecutive lookups using isLocalisedInStage tractorcow-farm/silverstripe-fluent#468

Merged

maxime-rainville mentioned this pull request Sep 26, 2018

MINOR Add caching to the TreeDropdownField tree action #8409

Merged

kinglozzer mentioned this pull request Nov 13, 2018

CMS ^4.3 should require framework ^4.3 silverstripe/silverstripe-cms#2328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380

NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380

sminnee commented Sep 18, 2018 •

edited by maxime-rainville

Loading

sminnee commented Sep 18, 2018

maxime-rainville left a comment

sminnee commented Sep 20, 2018

sminnee commented Sep 20, 2018

sminnee commented Sep 20, 2018

maxime-rainville commented Sep 20, 2018

robbieaverill commented Sep 20, 2018

maxime-rainville commented Sep 20, 2018 •

edited

Loading

sminnee commented Sep 20, 2018

sminnee commented Sep 21, 2018

chillu commented Sep 21, 2018

sminnee commented Sep 21, 2018

maxime-rainville commented Sep 23, 2018

sminnee commented Sep 24, 2018

sminnee commented Sep 24, 2018

sminnee Sep 24, 2018

maxime-rainville Sep 25, 2018 •

edited

Loading

sminnee commented Sep 24, 2018 •

edited

Loading

maxime-rainville commented Sep 24, 2018

maxime-rainville left a comment

sminnee Sep 24, 2018

sminnee Sep 24, 2018

maxime-rainville Sep 24, 2018

sminnee Sep 24, 2018

maxime-rainville Sep 24, 2018

sminnee commented Sep 24, 2018

maxime-rainville commented Sep 24, 2018

NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380

NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380

Conversation

sminnee commented Sep 18, 2018 • edited by maxime-rainville Loading

Parent issue

sminnee commented Sep 18, 2018

maxime-rainville left a comment

Choose a reason for hiding this comment

sminnee commented Sep 20, 2018

sminnee commented Sep 20, 2018

sminnee commented Sep 20, 2018

maxime-rainville commented Sep 20, 2018

robbieaverill commented Sep 20, 2018

maxime-rainville commented Sep 20, 2018 • edited Loading

sminnee commented Sep 20, 2018

sminnee commented Sep 21, 2018

chillu commented Sep 21, 2018

sminnee commented Sep 21, 2018

maxime-rainville commented Sep 23, 2018

sminnee commented Sep 24, 2018

sminnee commented Sep 24, 2018

sminnee Sep 24, 2018

Choose a reason for hiding this comment

maxime-rainville Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

sminnee commented Sep 24, 2018 • edited Loading

maxime-rainville commented Sep 24, 2018

maxime-rainville left a comment

Choose a reason for hiding this comment

sminnee Sep 24, 2018

Choose a reason for hiding this comment

sminnee Sep 24, 2018

Choose a reason for hiding this comment

maxime-rainville Sep 24, 2018

Choose a reason for hiding this comment

sminnee Sep 24, 2018

Choose a reason for hiding this comment

maxime-rainville Sep 24, 2018

Choose a reason for hiding this comment

sminnee commented Sep 24, 2018

maxime-rainville commented Sep 24, 2018

sminnee commented Sep 18, 2018 •

edited by maxime-rainville

Loading

maxime-rainville commented Sep 20, 2018 •

edited

Loading

maxime-rainville Sep 25, 2018 •

edited

Loading

sminnee commented Sep 24, 2018 •

edited

Loading