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

fabsor’s picture

This 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.

das-peter’s picture

Nice! 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_alter to 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.

fabsor’s picture

Status: Needs review » Fixed

Alright, this should probably be as fool-proof as we can make it. I commited this, together with the coding standards fixes, much appreciated!

das-peter’s picture

Status: Fixed » Needs review
StatusFileSize
new522 bytes

Ouch 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.

fabsor’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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