Refactor tree-generation prepopulation

[sminnee]

Problem

Right now CMSMain::SiteTreeAsUL() calls some Versioned prepopulation in order to generate the site tree without an egregious number of queries. This works okay, but has a couple of challenges:

• It has no extension point. Prepopulation of additional data would require manual adjustments to this method. This is a little awkward but workable for core changes (e.g. FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation silverstripe-cms#2266) but it's a showstopper for add-on modules that require prepopulation (e.g. https://github.com/tractorcow/silverstripe-fluent/issues/467)
• There's no way of repurposing it for other uses, such as treedropdownfield.

This relates to silverstripe/silverstripe-cms#2250.

Recommended fix

This logic should be wrapped into a prepopulateTreeDataCache method:

singleton('SiteTree')->prepopulateTreeDataCache(DataList $recordList = null, array $options)

• $recordList: An optional DataList of the records to include in the prepopulation. By default, all records will be included.
• $options: A map of metadata to assist. None of this should be assumed to be supplied, but it can be used to optimise any prepopulation, where appropriate.
  • 'childrenMethod': The name of the method on the record used to get children
  • 'numChildrenMethod': The name of the method on the record used to get number of children

Hierarchy can provide this method, since it relates to trees, and other extensions can hook into the behaviour by defining onPrepopulateTreeDataCache. The existing behaviour can be shifted to Versioned::onPrepopulateTreeDataCache, and the fluent ticket above could be implemented by providing this.

Pull requests

• NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380
• FIX Opt-in performance fix for many consecutive lookups using isLocalisedInStage tractorcow-farm/silverstripe-fluent#468
• FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation silverstripe-cms#2266
• FIX: Use Hierarchy::prepopulateTreeDataCache() in CMS. silverstripe-versioned#183

[sminnee]

Paging @silverstripe/core-team especially @tractorcow.

[chillu]

The existing behaviour can be shifted to Versioned::onPrepopulateTreeDataCache, and the fluent ticket above could be implemented by providing this.

Wouldn't fluent ideally replace the versioned implementation of this, to avoid running two expensive queries across potentially all records? That seems incompatible with the event hook approach (onPrepopulateTreeDataCache). Since it's implemented as a raw SQL query for performance reasons, there's no way to call augmentSQL through an extension. I guess it depends on how long a SELECT ... GROUP BY takes when you run it over 20k+ records. The GROUP BY criteria (ParentID, Locale) should already be indexed, so might not be worth the hassle in terms of performance gains.

[chillu]

Update: With the project we're testing against (17k records), SELECT ParentID, COUNT(*) FROM SiteTree GROUP BY 1 takes 5ms on my localhost through Vagrant. Should be roughly the same for a remote database, since it's just one call. The fluent version will take a bit longer, but looks like the impact of running both queries in negligible.

[sminnee]

Fluent is caching a different set of data: hierarchy is caching numChildren, fluent other is caching isLocalisedOnStage.

The numChildren caching would ideally incorporate any query modification that fluent necessitates, which my second implementation (that takes the stageChildren query of a single record and transforms it into a precaching query for all ParentIDs) does.

[sminnee]

OK this now blocks merge of Scopey's Fluent PR.

[sminnee]

OK the first cut of this is implemented, it affects a few modules:

• framework: NEW: Add Hierarchy::prepopulate_numchildren_cache() #8380 (merged)
• fluent: FIX Opt-in performance fix for many consecutive lookups using isLocalisedInStage tractorcow-farm/silverstripe-fluent#468
• cms: FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation silverstripe-cms#2266
• versioned: FIX: Use Hierarchy::prepopulateTreeDataCache() in CMS. silverstripe-versioned#183

Strictly speaking, the best merge order is probably fluent & versioned, then framework, then CMS

[maxime-rainville]

The only PR still open on this one is tractorcow-farm/silverstripe-fluent#468.

[robbieaverill]

@maxime-rainville the fluent PR has been merged, so I'll close this ticket - feel free to reopen if I've misunderstood your comment