Currently _cache_actions_get_cache_bins() is used at several locations to get the available cache bins.
To do so the function calls module_invoke_all('flush_caches') which unfortunaltely already triggers some cache flushes.
This leads to an essential slow down of the site.
To avoid this I've introduced a variable to store the detected cache bins for a limited time.
The variable also contains an expiration timestamp after which the variable is repopulated.
Comments
Comment #1
fabsor commentedThis is a really nice catch - I haven't used this actual rule that much myself, which is probably why I missed this pretty obvious performance issue.
We discussed this solution on IRC, so I'm all on-board with the approach. There are some issues however:
You changed the implementation so that we only check which module implements hook_flush_caches, and then the name of the module is added to the list of cache bins. hook_flush_caches returns an array of cache bins however, and they do not correlate with the module name and there can be multiple cache bins. I changed this back to module_invoke_all. It shouldn't matter that much since we are caching it pretty hard.
Even though it's just a matter of deleting the variable, I'm still worried that users will be confused by errors that might occur if a cache bin is deleted (if the module is uninstalled) or that a new cache bin does not show up when you enable a module. This might be a little bit over the top, but I added a hook_modules_enabled and hook_modules_disabled implementation that resets the variable if any of the disabled modules implements hook_flush_caches(). This resulted in a bit more code, so I moved in the get_cache_bins function into the module file and removed it's private indicator (_), since it might be useful.
Comment #2
das-peter commentedNice! And sorry for the
module_implements()I simply was sloppy :|I like the idea of
hook_modules_enabled/hook_modules_disabled.And I had another idea, what about using
hook_updater_info_alterto make the expiration part of the variable superfluous.That way the variable is rebuild whenever modules are enabled, updated or disabled. Is there any other event which could change the available cache bins?
As compensation of my sloppiness, I've attached a second patch as follow up for the case the other makes it in. I've cleaned up the code to comply with the coding standards as good as possible.
Comment #3
fabsor commentedAlright, this should probably be as fool-proof as we can make it. I commited this, together with the coding standards fixes, much appreciated!
Comment #4
das-peter commentedOuch just discovered that something went wrong on cleanup. The condition to check if the cache bins are already stored checks for the wrong variable.
Attached patch fixes this.
Comment #5
fabsor commentedCommited, thanks.
http://drupal.org/commitlog/commit/14252/34c2fb97649e2b980130e2e98a6d0f4...