Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Precache Hierarchy::numChildren() for performance #8379

Closed
sminnee opened this issue Sep 17, 2018 · 5 comments
Closed

Precache Hierarchy::numChildren() for performance #8379

sminnee opened this issue Sep 17, 2018 · 5 comments
Labels
affects/v4 impact/critical type/enhancement

Comments

@sminnee
Copy link
Member

sminnee commented Sep 17, 2018 •
edited
Loading

The CMS treeview makes heavy use of Hierachy::numChildren() and its caching feature should be extended to allow precaching in a similar manner to Versioned::prepopulate_versionnumber_cache.

There is a call to Hierarchy::numChildren() for every tree-item shown in the CMS. This accounts for the bulk of time calling treeview / getsubtree URL requests, at least in cases where silverstripe/silverstripe-versioned#181 has been applied.

Approach

  • Replace the cache variable in Hierarchy::numChildren with a static cache by stage, class and ID, like Versioned
  • Introduce a prepopulate_versionnumber_cache method based on Versioned::prepopulate_versionnumber_cache
  • Use a query similar to "SELECT ParentID AS ID, COUNT(*) AS NumChildren FROM SiteTree WHERE ParentID != ID" to cache the number of children in bulk.
  • Incorporate the exclusion filters of Hierarchy::stageChildren() into this query

augmentStageChildren deprecation

It would be difficult to make this work with augmentStageChildren. I can't see many places where augmentStageChildren is used (it's not used in any of the supported modules, as far as I can see), and my recommendation is that we deprecate it in order to implement this performance features.

  • Detect if augmentStageChildren is used, If so, throw a warning that this disables the performance optimisation
  • Suppress the bulk prefetching of numChildren in this case

CMS update

Once this change has been made, CMSMain::SiteTreeAsUL can make calls to Hierarchy::prepopulate_numchildren_cache
@sminnee sminnee mentioned this issue Sep 17, 2018
Closed
1 task
@sminnee
Copy link
Member Author

sminnee commented Sep 17, 2018

I'd be keen to get @tractorcow's thoughts on this.

@sminnee sminnee mentioned this issue Sep 18, 2018
Merged
sminnee pushed a commit to sminnee/silverstripe-cms that referenced this issue Sep 18, 2018
FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation
d085d6d 
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
This was referenced Sep 18, 2018
Merged
Closed
@sminnee
Copy link
Member Author

sminnee commented Sep 19, 2018

Note that this might best be refactored as part of #8391.

sminnee pushed a commit to sminnee/silverstripe-cms that referenced this issue Sep 21, 2018
FIX: Use Hierarchy::prepopulate_numchildren_cache in tree-generation
95fae36 
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 pushed a commit that referenced this issue Sep 25, 2018
@maxime-rainville
NEW: Add Hierarchy::prepopulate_numchildren_cache() (#8380)
5b7a841 
* NEW: Add Hierarchy::prepopulate_numchildren_cache()
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

* NEW: Add Hierarchy::prepopulateTreeDataCache()

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

Fixes #8391
maxime-rainville pushed a commit to silverstripe/silverstripe-cms that referenced this issue Sep 25, 2018
@maxime-rainville
FIX: Use Hierarchy::prepopulateTreeDataCache() in CMS (#2266)
5bfc37f 
* 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
Copy link
Member Author

sminnee commented Oct 1, 2018

I believe that we can close this one now, @maxime-rainville @ScopeyNZ?

@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Oct 1, 2018

I'll leave @maxime-rainville to confirm but I'm happy to close it.

@maxime-rainville
Copy link
Contributor

Yes ... we're all done here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
affects/v4 impact/critical type/enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sminnee @maxime-rainville @ScopeyNZ