Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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...)
Comment | File | Size | Author |
---|---|---|---|
#15 | advagg-1209680-15.patch | 3.3 KB | mikeytown2 |
#13 | advagg-1209680-13.patch | 9.24 KB | mikeytown2 |
#8 | advagg-1209680-8.patch | 6.31 KB | mikeytown2 |
#5 | advagg-1209680-5.patch | 4.65 KB | mikeytown2 |
#4 | advagg-1209680-3.patch | 4.09 KB | mikeytown2 |
Comments
Comment #1
catchSubscribing.
Comment #2
mikeytown2 CreditAttribution: mikeytown2 commentedIt'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
Comment #3
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThanks 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.
Comment #4
mikeytown2 CreditAttribution: mikeytown2 commentedhere's the patch
Comment #5
mikeytown2 CreditAttribution: mikeytown2 commentedforgot to add in the admin code for this patch
Comment #6
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedWe 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?
Comment #7
mikeytown2 CreditAttribution: mikeytown2 commentedadvagg_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
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedOh 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
Comment #9
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedThe 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?
Comment #10
mikeytown2 CreditAttribution: mikeytown2 commentedcaching 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.
Comment #11
mikeytown2 CreditAttribution: mikeytown2 commentedspeaking of the cache ID; advagg_bundler_analysis should use the advagg_bundle table count as part of the cache id.
Comment #12
catchAgreed 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.
Comment #13
mikeytown2 CreditAttribution: mikeytown2 commentedpatch 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.
Comment #14
mikeytown2 CreditAttribution: mikeytown2 commentedwent ahead and committed these changes.
Comment #15
mikeytown2 CreditAttribution: mikeytown2 commentedadded in this as well