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

Tweak to fix performance of calling tailor_get_terms() #120

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

codeeverything
Copy link

@codeeverything codeeverything commented Jun 9, 2017

Tweak to fix performance when large numbers of categories/tags exist, and/or large numbers of elements are used on a Tailored page.

  • Cache categories and post tags once on element registration, rather than for each element on the page
  • Cache data used in tailor_modify_colorpicker to reduce calls to get_theme_mod()
  • Cache $control_definitions in tailor_control_presets() to keep calls to __() sky-rocketing

…nt. Instead call it once on registration and cache the result in static variables
@codeeverything
Copy link
Author

I'd closed this PR as I hadn't originally intended to make it public, only to share with my colleagues. However, we're now using this change internally to fix the performance issue when you have a large number of terms (10,000s in our case).

I'm not a Wordpress developer so I'm not sure how "Wordpressy" the fix is, but the notion of caching early in the process rather than reading each term for each element seems sound.

Would be great to hear your thoughts :)

@andrew-worsfold
Copy link
Contributor

Thanks, @codeeverything. I wonder whether WordPress transients would be a better approach to solving this problem; have you considered those? If not, I can investigate further.

@codeeverything
Copy link
Author

codeeverything commented Jun 26, 2017

@andrew-worsfold Thanks for the reply :) Not sure about transients - after a quick read up it seems like these would persist across requests. That would lighten the load, but IMHO I think each request should be served on it's own merits, so I'd go with wp_cache_add() and wp_cache_get() I think. I'll try to get some time this week to rewrite the code in this PR to use these instead of class statics.

I also have a couple of other problem areas I'm looking at (similar caching solution), which I may push into this PR or add as new ones. Just got to find some time to look more properly this week.

These few performance issues aside - nice plugin :)

@codeeverything
Copy link
Author

codeeverything commented Jun 26, 2017

@andrew-worsfold As discussed I've rewritten my original change to use Wordpress's object cache rather than rely on class static variables. This doesn't perform quite as well (more logic overhead), but it's probably a safer bet.

I've also included two other performance fixes in the same vein.

The tailor_modify_colorpicker function in includes/helpers/helpers-color.php now caches the result of the $control_args variable. For our setup this was incurring significant penalties by calling get_theme_mod() for each colour on each call to this function, which I think in turn was calling get_option many times. Bit of a snowball effect :)

The tailor_control_presets function in includes/helpers/helpers-elements.php now caches the $choices and $control_definitions values. The latter is particularly important as it calls the __() function many times. As far as I could see there was no reason to generate this array more than once, where it was doing it for every Tailor element on the page. __() seems to eventually call translate() (I think this was in l10n.php in the WP core), to provide multi lingual support. Prior to this little tweak the number of calls we saw for this shot up from ~1,000 to ~11,000 per page load! This change reduces this to a more sensible ~1,800.

@codeeverything
Copy link
Author

@andrew-worsfold As a final note - the above changes leave us with around a 30% penalty when enabling Tailor, a better result but would be nice if it was less :)

I think we're starting to hit diminishing returns, but I had one last thought. It looks as though Tailor elements are registered and call tailor_control_presets regardless of whether they are used on the page or not? I may be wrong here. If that is the case I wonder if this could be reworked to analyse the page first and only initialise the elements that have actually been used (or are used as sub elements of those in use)?

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

Successfully merging this pull request may close these issues.

3 participants