Epic: CMS performance improvements

[mfendeksilverstripe]

Current CMS admin application doesn't perform well with large websites (using SS 4.1, but these issues were present in SS 3 as well). I identified 3 areas that largely contribute to the performance issues.

My test setup:

• 6013 - TaxonomyTerm objects in a well-structured hierarchy (individual nodes shouldn't have too many children as they represent geographical regions)
• 3009 - Page objects in not so well structured hierarchy (some nodes can have many children)
• using Fluent module on both objects (although the performance issues may not necessarily be Fluent related)

I tested page load with individual requests broken down.

1 - EditForm

Request admin/pages/edit/show/<page_id>

CMSMain::getEditForm() calls a function getArchiveWarningMessage(). I suspect the purpose of this function is to populate the warning message in the case user decided to archive the page as the action may have effect on children pages. This warning message generation is very costly though:

// Get all page's descendants
$record->collateDescendants(true, $descendants);
.
.
.
foreach ($descendants as $page) {
.
.
.

This loops through the all the page descendants which gives me a solid 70 seconds load time just for the server response as we are looping though all the pages if I select the root page. The issue is less bad when a page further down the hierarchy is selected. It still has an impact though and most importantly this is called every time EditForm is rendered.

Summary:

Issue occurs once per every CMS page load and perfomance hit depends on the position of the current page in site tree.

2 - TreeView

Request admin/pages/edit/treeview

CMSMain::treeview() takes 12 to 18 seconds to generate response. This function just populates a template:

$this->renderWith($this->getTemplatesWithSuffix('_TreeView'))

Further investigation is needed to figure out which part of the template is slow. This issue is present on every CMS page which shows the site tree, but at least is triggered only once.

Summary:

Issue occurs once per every CMS page load.

3 - TreeDropdown

Request admin/pages/edit/EditForm/<page_id>/field/<field_name>/tree?format=json

This request takes 10 to 12 seconds per one TreeDropdownField (or a subclass). It's the most tricky one as well. Obvious place to start investigating is the TreeDropdownField::tree() however the slowness isn't coming from here. I disabled this action complety by removing tree from the $allowed_actions in the TreeDropdownField. After this change you will get 404 as a response, but it will still take almost the same amount of time to get the response.

I measured the tree() which took about 0.5 seconds and also the constructor which tok 1 to 1.5 seconds. The slowness must be coming from somewhere else.

Summary:

Issue occurs once per every TreeDropdownField present on the CMS page load.

PRs

• FIX Improve performance of CMSMain::getArchiveWarningMessage silverstripe-cms#2231
• MINOR: Add CMS specific hierarchy config silverstripe-cms#2248

Note: Please install zenhub.com to see tickets associated to this epic.

[krismcgnz]

Note: The supporting snapshot for this issue, is available to SilverStripe staff at this url https://drive.google.com/drive/u/0/folders/1txqgXIxBGWe7iCM_StA6zeYS598uRXnQ

[maxime-rainville]

1 - EditForm

For this action takes about 9.5 sec. As mentioned this is entirely taken by getArchiveWarningMessage.

The I can't be bothered solution

getArchiveWarningMessage essentially displays a slightly different message based on whatever the page has children and whatever the children are part of campaigns:

• Warning: This page and all of its child pages will be unpublished and automatically removed from their associated {NumCampaigns} before being sent to the archive.
• Warning: This page and all of its child pages will be unpublished before being sent to the archive.
• Warning: This page will be unpublished and automatically removed from their associated {NumCampaigns} before being sent to the archive.
• This page will be unpublished before being sent to the archive.

We could just use a more generic message. That would bring down the request time to a fraction of a sec.

Functionally equivalent solution

I found a way to speed up this method by using lower level instructions. Instead of fetching an array containing all the other pages, it fetches an array of all page IDs.

I takes about 2.5 sec on average for me and should be functionally equivalent. I can probably shave more time of this if I re-implement get_for_object_by_id to accept an array of ID.

    /**
     * Build an archive warning message based on the page's children
     *
     * @param SiteTree $record
     * @return string
     */
    protected function getArchiveWarningMessage($record)
    {
        // Get all page's descendants
        $descendants = [];
//        $record->collateDescendants(true, $descendants);
        $this->collateDescendants([$record->ID], $descendants);
        if (!$descendants) {
            $descendants = [];
        }

        // Get all campaigns that the page and its descendants belong to
//        $inChangeSetIDs = ChangeSetItem::get_for_object($record)->column('ChangeSetID');
        $inChangeSetIDs = ChangeSetItem::get_for_object_by_id($record->ID, SiteTree::class)->column('ChangeSetID');

        foreach ($descendants as $page) {
            $inChangeSetIDs = array_merge(
                $inChangeSetIDs,
//                ChangeSetItem::get_for_object($page)->column('ChangeSetID')
                ChangeSetItem::get_for_object_by_id($page, SiteTree::class)->column('ChangeSetID')
            );
        }

        if (count($inChangeSetIDs) > 0) {
            $inChangeSets = ChangeSet::get()->filter(['ID' => $inChangeSetIDs, 'State' => ChangeSet::STATE_OPEN]);
        } else {
            $inChangeSets = new ArrayList();
        }

        $numCampaigns = ChangeSet::singleton()->i18n_pluralise($inChangeSets->count());
        $numCampaigns = mb_strtolower($numCampaigns);

        if (count($descendants) > 0 && $inChangeSets->count() > 0) {
            $archiveWarningMsg = _t('SilverStripe\\CMS\\Controllers\\CMSMain.ArchiveWarningWithChildrenAndCampaigns', 'Warning: This page and all of its child pages will be unpublished and automatically removed from their associated {NumCampaigns} before being sent to the archive.\n\nAre you sure you want to proceed?', [ 'NumCampaigns' => $numCampaigns ]);
        } elseif (count($descendants) > 0) {
            $archiveWarningMsg = _t('SilverStripe\\CMS\\Controllers\\CMSMain.ArchiveWarningWithChildren', 'Warning: This page and all of its child pages will be unpublished before being sent to the archive.\n\nAre you sure you want to proceed?');
        } elseif ($inChangeSets->count() > 0) {
            $archiveWarningMsg = _t('SilverStripe\\CMS\\Controllers\\CMSMain.ArchiveWarningWithCampaigns', 'Warning: This page will be unpublished and automatically removed from their associated {NumCampaigns} before being sent to the archive.\n\nAre you sure you want to proceed?', [ 'NumCampaigns' => $numCampaigns ]);
        } else {
            $archiveWarningMsg = _t('SilverStripe\\CMS\\Controllers\\CMSMain.ArchiveWarning', 'Warning: This page will be unpublished before being sent to the archive.\n\nAre you sure you want to proceed?');
        }

        return $archiveWarningMsg;
    }

    private function collateDescendants($recordIDs, &$collator) {

        $children = SiteTree::get()->filter(['ParentID' => $recordIDs])->column();
        if ($children) {
            foreach ($children as $item) {
                $collator[] = $item;

            }
            $this->collateDescendants($children, $collator);
            return true;
        }
        return false;
    }

[maxime-rainville]

2 - TreeView

The part that seems to be slowing this down quite a bit is CMSMain::getSiteTreeFor(). Most of the heavy lifting is handled by SilverStripe\ORM\Hierarchy\MarkedSet. There's not much room for improvement in MarkedSet, because it's coded in a generic way so it can be applied to any Hierarchy.

So short of coding a SiteTree specific MarkedSet, there's not much improvement that can be done here.

Specific project can try tweaking the node_threshold_total. Basically, the treeview tries to preload subnodes so they are available right away when you try to expand them in UI.

If you use a smaller node_threshold_total, the initial treeview rendering will be quicker, but you'll need more AJAX request when navigating the tree view. This cut the rendering time of treeview for me by about 50%. It might be something worth experimenting with.

We'll need to move private static $node_threshold_total = 50; out of Hierarchy and into the YML config to make it easier for people to alter the value.

[maxime-rainville]

3 - TreeDropdown

The TreeDropDownField uses the same logic for building it's underlying list. The same node_threshold_total trick should work there as well ... it will have the same side effect.

SilverStripe\Forms\TreeDropdownField:
  node_threshold_total: 5

[maxime-rainville]

@mfendeksilverstripe Can you try using the CMS PR above and adding the bits below in your config?

It makes a bit of difference for me, although I wouldn't say it's dramatic.

SilverStripe\CMS\Model\SiteTree:
  node_threshold_total: 1

SilverStripe\Forms\TreeDropdownField:
  node_threshold_total: 1

[mfendeksilverstripe]

@maxime-rainville Thanks for the performance improvements. Here is my feedback:

1 - the initial Request admin/pages/edit/show/<page_id> went down to 25s from 30s on my test page (I hardcoded the warning message). Interesting thing is that when you measure time spent in EditForm function (which is bound to the request handler) it's only a few seconds. I'm not sure where does the large overhead come from. Please do more investigation in this area.

2 & 3 - applied new configuration and it seems to help slightly. It saves few seconds per field, which is not bad.

[chillu]

Sorry, a bit late in this convo. Regarding archive warnings, that's been a lazy way to implement the original feature - it's computing this warning regardless whether the author actually wants to trigger an associated action. We can speed that up, or we can do it smarter. I've proposed an alternative which doesn't slow down every fricken CMS page load request in every CMS instance ;) But it's a lot more work to do generically, so not a quick fix. @mfendeksilverstripe Would you be open to running a fork that simply removes the line of code which computes the archive warning message in the first place, until we have this sorted? See #621 (impact/high in our backlog).

[chillu]

Just for future reference, I've tracked this down to silverstripe/silverstripe-cms#1774 (comment)

[frankmullenger]

@chillu this has been implemented already (getArchiveWarningMessage() returns a hard coded string) but doesn't appear to have had an appreciable impact on performance for us so far.

[maxime-rainville]

So I've been playing with TopicPage ID 23. I've been removing FormFields to see how this effects the time it takes to render the page. That's render time I got with various fields combination:

• All fields in : ~ 20.0 sec
• All fields removed: ~ 0.4 sec
• ElementalArea only: ~ 14.0 sec
• PrimaryImage only: ~ 5.0 sec
• DependentPages only: ~ 1.3 sec
• All fields included except ElementalArea, PrimaryImage and DependentPages: ~ 0.5 secs

It's weird that PrimaryImage is taking 5 sec, because that's just a plain old UploadField. I'll look into that a bit more.

The ElementalArea field might be affected by that custom template the customer project is using. I'll play around with that to see if it can be improved it. But I might need more detail about your use case for having a custom template there.

I'll leave DependentPage for last.

[maxime-rainville]

I think I've identified why the UploadField is so slow. UploadField attaches a bunch of MetaData about each file by calling AssetAdmin::getObjectFromData(). One of that metadata field is inUseCount which contains the number of DataObject referencing this file. To generate that number it recursively tries to identify each owner of the file.

The main purpose of this field seems to populate a This file is currently used in %s place(s), are you sure you want to delete it? when deleting the file. This is especially useless in this case because the UploadField doesn't allow you to delete files.

A quick fix could be to cut down the number of meta data fields that we attached to files in our Upload Field. That should shave 5 sec off that specific request.

I suspect however we'll have a similar problem when accessing the files through Asset-Admin. That's probably worth more investigation.

[chillu]

The main purpose of this field seems to populate a This file is currently used in %s place(s), are you sure you want to delete it? when deleting the file.

That sounds like a very similar type of thinking which lead to #621. We proactively compute things we only need in maybe 1% of the usage flows (assuming it's 100x more likely that you view a file than deleting it). Which is wasteful, we should do it on demand. Which is why I'm pushing to implement #621 in a generic way (cms actions can ask for confirmation before proceeding)

[maxime-rainville]

I've got partial fix with silverstripe/silverstripe-asset-admin#829. This updates the data attached to UploadField items.

But we should look at at fetching that info only when the user is actively deleting the file as @chillu suggest. Otherwise, this will bite us in other places.

This should save 5 sec in the specific case I'm looking at.

[chrispenny]

Hi team, Just a note that a side effect of using node_threshold_total seems to be that you cannot search for results outside of that threshold (maybe that's expected?).

EG, with no threshold set, I can find the TaxonomyTerm for "Auckland".

Same page, but with threshold now set, no result for "Auckland".

We're using:

SilverStripe\Forms\TreeDropdownField:
  node_threshold_total: 1

[mfendeksilverstripe]

Another area to look at is the Elemental area gridfield in case you have roughly 30 content blocks (60 blocks in total when you also count layout blocks). It takes around 2 minutes to load such page.

It seems that the performance is scaling very poorly when lot of blocks are present. I did some basic benchmarking:

35s - load page without recursive publish state feature and without ElementalArea

extra 5s - add recursive publish state feature (custom code)

extra 79s - add a vanilla display for Layout block (editor template), but keep custom gridfield actions

extra 35s - add a vanilla display for Layout block (editor template), no custom gridfield actions

Areas to look at:

Nested blocks render in editor template
custom gridfield actions
vanilla elemental area setup with roughly 60 blocks in total

[mfendeksilverstripe]

Raised a separate issue on Elemental

silverstripe/silverstripe-elemental#368

[unclecheese]

TreeView

If you use a smaller node_threshold_total, the initial treeview rendering will be quicker, but you'll need more AJAX request when navigating the tree view.

A setting of 50 for node_threshold_total is still brutally slow for me -- around 10s. It becomes tolerable when set to 10 (3s).

Would recommend adding a limit for node_threshold_leaf as well. Because you'll be relying more on AJAX to expose subtrees, you're still going to run into issues when you go to expose a tree with lots of children. "Business listings bucket" for instance, took ~9s to expose. There's no good case that I can see for rending a node of that size in the SiteTree, and we should be pushing content authors to use the list view for those types of sections. The node_threshold_leaf setting will force a "Too many pages (view as list)" link into the tree. The list view is much more performant, as it's paginated.

Action Item:

• Add node_threshold_total and node_threshold_leaf config to CMS config.yml

[unclecheese]

Have done the above in silverstripe/silverstripe-cms#2248

[unclecheese]

Here are some of the benchmarks I've come up with. This is based on renders of CMSMain_Content.ss only, with $Tools and $Menu stripped out.

• All fields: 17s (1097 queries)
• No BlockPage fields (ElementalArea customisations): 12s (762 queries)
• No BlockPage or, Page fields (TreeDropdowns): 10s (646 queries)
• No BlockPage, Page or ElementalArea: 3s (69 queries)
• Empty FieldList: 1.57s (53 queries)
• getEditForm() returns EmptyForm(): 1s (13 queries)

Lots of things to look at, but clearly the biggest drop comes when ElementalArea is removed, which imposes a 7-second, 600 query cost.

[robbieaverill]

cc @brynwhyman, might need to schedule some performance work for elemental if the OSSers don’t get to it first

Edit: cross reference: silverstripe/silverstripe-elemental#368

[unclecheese]

Further breakdown of ElementalArea:

• As is: 10s (646 queries)
• Limited to one record: 4s (134 queries)
• Limited to three records: 5s (262 queries)
• Limited to five records: 7.7s (390 queries)
• Limited to seven records: 9.5s (518 queries)

Pretty deterministic results, here. Each row costs 128 queries and roughly two seconds.

[guci0]

Number of queries is so high (anyway).

[sminnee]

@unclecheese @maxime-rainville have we considered that this is exacerbated by the treeview reloading every time you change a page? That seems significant and unnecessary.

[sminnee]

Related silverstripe/silverstripe-versioned#181

[unclecheese]

Have merged silverstripe/silverstripe-asset-admin#829, which improves the edit form speed remarkably. (at least 50%). That said, Elemental still continues to be a huge problem. Here are some benchmarks (testing with page 666)

Remove ElementalArea	Apply UploadField fix	Time to render EditForm
-	-	48s
X	-	23s
-	X	19s
X	X	2s

The road to that sweet 2 second load time goes through Elemental.