advagg_get_files_in_bundle runs a mysql query per bundle per page request. Couldn't that information be stored in a cache table (and that then be replaced with a memcached backend...)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Subscribing.

mikeytown2’s picture

It's not as bad as it seems because of the cache at the top of the advagg_css_js_file_builder() function. Bundler's hook runs in advagg_get_filename().

Reason I run it on a cache miss for the advagg_css_js_file_builder code is so it will pick up changes to the bundles sooner. If you still think it needs a cache let me know, I'll create an item in the cache table for it.

edit:
thinking about this & I'll add in a cache for it

killes@www.drop.org’s picture

Thanks for adding a cache item.

But just to confirm: you are saying this query should not run on a page that has been requested before? Or not?

In my case the queries are run for each page, regardless of how often I call it.

mikeytown2’s picture

Status: Active » Needs review
FileSize
4.09 KB

here's the patch

mikeytown2’s picture

FileSize
4.65 KB

forgot to add in the admin code for this patch

killes@www.drop.org’s picture

We may have been talking about different things...

You patch adds a cache to the bundle analysis part. What I wanted is cache for advagg_get_files_in_bundle. Should I prepare a patch?

mikeytown2’s picture

advagg_css_js_file_builder Cache hit. Above with or without above patch gives the same numbers.
47ms - advagg_processor
23ms - advagg_process_js
17ms - advagg_process_css
5ms - hook_advagg_disable_processor

advagg_css_js_file_builder Cache miss. With above patch
82ms - advagg_processor
41ms - advagg_process_js
34ms - advagg_process_css
5ms - hook_advagg_disable_processor

advagg_css_js_file_builder Cache miss. Without above patch
136ms - advagg_processor
95ms - advagg_process_js
34ms - advagg_process_css
5ms - hook_advagg_disable_processor

mikeytown2’s picture

FileSize
6.31 KB

Oh well; found more speed with a cache.

Looking at advagg_get_files_in_bundle(). We could use a cache here; but I didn't think this query is that slow so I didn't bother with one yet.

The queries your seeing have to do with cache verification. advagg_css_js_file_builder calls advagg_cached_bundle_get. This was added because of some issues I was having with the cache. I think I've solved all of them so I can move this check so it only runs if debugging is turned on.

This patch gives this; seems like all the time gets spent in drupal_alter now. Not really sure I want to cache at the advagg_processor level... thoughts?
39ms - advagg_processor
20ms - advagg_process_js
12ms - advagg_process_css
5ms - hook_advagg_disable_processor

killes@www.drop.org’s picture

The query isn't particularly slow, but if you can avoid them easily I see not reason to run them. The idea is to move load to parts of the infrastructure that can be more easily scaled. I now see cache_get queries instead, thanks!

I am unsure about the processor function, but if it is safe to cache, I'd avoid the array manipulations in there. Not sure about gain vs loss there, though.

Maybe catch has an opinion?

mikeytown2’s picture

caching at the advagg_processor level would probably bring total time down to sub 20ms range; most of that time being spent creating the cache id.

mikeytown2’s picture

speaking of the cache ID; advagg_bundler_analysis should use the advagg_bundle table count as part of the cache id.

catch’s picture

Agreed with killes on caching the query - if the same query is running on almost every page, then it almost always makes sense to cache it, not for raw speed but taking load off MySQL when there's a different cache backend (and a global cache won't add load when using MySQL caching, per page/user is a bit more iffy but this wouldn't be that).

On caching at the processor level, I'm not very familiar with the code here yet. If it's really a 20ms saving that sounds worth it if it's just moving the cache up a level. However if it's replacing PHP time with a cache request then not so much.

mikeytown2’s picture

FileSize
9.24 KB

patch above refactored the code so the query doesn't need to be run.

Added in a cache at the advagg_processor level
8.3ms - advagg_processor
This makes advagg faster then core, which is impressive because advagg does a lot more; just be aware that the only hook that gets ran 100% of the time is hook_advagg_disable_processor now.

mikeytown2’s picture

Status: Needs review » Fixed

went ahead and committed these changes.

mikeytown2’s picture

FileSize
3.3 KB

added in this as well

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.