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.

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

Allow caching and prepopulation for isLocalisedInStage #467

Closed
sminnee opened this issue Sep 19, 2018 · 14 comments
Closed

Allow caching and prepopulation for isLocalisedInStage #467

sminnee opened this issue Sep 19, 2018 · 14 comments

Comments

@sminnee
Copy link

sminnee commented Sep 19, 2018

FluentVersionedExtension::isLocalisedInStage() is heavily used in the CMS, e.g. when rendering tree items.

In order to cut down on the query count on large sites, this value should be able to be cached and the cache prepopulated, as we do for page versions, and as I have recommended for Hierarchy::numChildren() in silverstripe/silverstripe-framework#8379.

@sminnee
Copy link
Author

sminnee commented Sep 19, 2018

One small challenge with this is that there's no extension hook for prepopulating more items. CMSMain may need an onBeforeSiteTreeAsUL() extension hook, or possibly a SiteTree singleton should carry this responsibility.

@robbieaverill
Copy link
Contributor

Quickly noting that extensions are singletons in SS4 by default, so we'd need to include DataObject IDs and class names in internal cache field names

@tractorcow
Copy link
Collaborator

Yes that sounds about right @robbieaverill. I'd agree with such a cache addition.

We could have FluentSiteTree::prepopulate_localised_cache() as well.

@ScopeyNZ
Copy link
Collaborator

I'm working on a patch for this now.

@ScopeyNZ ScopeyNZ self-assigned this Sep 20, 2018
@sminnee
Copy link
Author

sminnee commented Sep 20, 2018

I've noted in silverstripe/silverstripe-framework#8391 a refactoring of these caches to make it easier for modules such as fluent to plug into the system.

@ScopeyNZ
Copy link
Collaborator

I've been working on this today. It's going okay but pre-populating the cache is a little intense due to the fact that not all pages will be checked, but we can't really know what IDs to filter by on pre-pop.

Anyway. I'm addressing this with some SELECT EXISTS magic and I'll have a PR ready soon.

@sminnee
Copy link
Author

sminnee commented Sep 20, 2018

Just get the results for all pages. If it's an optimised query it'll be a few ms even with 100,000 pages

@ScopeyNZ
Copy link
Collaborator

Getting the results is pretty quick. Looping the results is not so quick :( - Anyway. It reduces the number of queries by ~200 (for a full site tree), but doesn't really show any significant performance difference on local. I'm guessing the difference will be more measurable on SSP due to 200 less queries.

@ScopeyNZ
Copy link
Collaborator

Actually I didn't realise the incredible performance improvement of array_column. Wow.

@robbieaverill
Copy link
Contributor

@ScopeyNZ is this resolved with #468 ?

@ScopeyNZ
Copy link
Collaborator

Yep!

@ScopeyNZ ScopeyNZ reopened this Sep 25, 2018
@ScopeyNZ
Copy link
Collaborator

Oh I thought it was merged! Ha.

@robbieaverill
Copy link
Contributor

I've pushed some tests to #468, I think it's good to go

@robbieaverill
Copy link
Contributor

PR merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants