Enclavely / tailor

Build beautiful page layouts quickly and easily using your favourite WordPress theme.
https://www.tailorwp.com
GNU General Public License v3.0
1.05k stars 102 forks source link

Tweak to fix performance of calling tailor_get_terms() #120

Open codeeverything opened 7 years ago

codeeverything commented 7 years ago

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

codeeverything commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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)?