Author Topic: runHooks performance (Read 631 times)

jcasharpe · May 04, 2013, 04:54:13 am

On an average civicrm page I am seeing 57 calls to runHooks. This results in 10887 function_exists calls as there is apparently 191 modules in total on my Drupal site. Only 4 of these calls actually find a hook to execute. This seems like a perfect candidate to get cached as this actually takes around 25% of the page execution time.
Is there any existing code structure to support caching this sort of thing globally? I'd envision us having a table which lists modules that implement hooks that gets periodically refreshed via a cron task.

As the few hooks that currently run for me don't really add anything I need I've actually commented out the code that runs the hooks on my production site as the speed up is significant enough for me. (This is currently on 4.1 but I've observed similar figures for 4.3)

totten · May 04, 2013, 12:14:50 pm

10k is a lot, and it's an interesting candidate for optimization. I was curious how the overhead of 10k function_exists() calls compares to the overhead of fetching a cached listing from DB:

https://gist.github.com/totten/5518406

I only spent 5 minutes on that snippet, so it's still pretty inefficient, but it does show that there's room for improvement with some caching -- i.e. it improved from 11ms to 8ms. I think we should do another experiment with more realistic/efficient data-structure, but I'm guessing the floor is around 5ms (based on the time to load from DB).

Caching this would have the downside of hampering development slightly -- i.e. when a developer adds a new hook, he would need reset the cache. But maybe we could sort it out so that some systems enable caching while others disable caching?

Do you think the effort is worth it to trim ~5ms from each request?

jcasharpe · May 04, 2013, 03:58:15 pm

Ah the code doesn't quite hit the point of caching; you would only be retrieving a relatively small array that lists the functions (in my case 4) so you wouldn't have a 10k loop so this would bring the floor down to 2ms. Also if we make the assumption that we're likely always to call some hooks we can preload the array as part of the initialisation (and potentially amortize the cost alongside loading some other settings?)

totten · May 04, 2013, 05:29:33 pm

* Agree that a simple, affirmative list of hooks is a more realistic/efficient data-structure.
* That's a good point about loading alongside other settings to reduce the cost and could cut it further.
* This could also be an opportunity to rewrite the hook invocation stuff to make it cleaner and more testable.

xavier · May 04, 2013, 11:04:08 pm

So we would introduce a mode production|development?

It might be interesting as well to use that setting elsewhere. I suspect compile_check=false would be a good candidate to production mode to speed up things.

http://www.smarty.net/docsv2/en/variable.compile.check.tpl

Donald Lobo · May 05, 2013, 06:52:36 am

pretty sure drupal introduced caching for this in d7

we should check that code base and see how it is implemented there and potentially get a few ideas from there

lobo

totten · May 10, 2013, 02:12:41 pm

FWIW, Drupal's implementation does some of the things we would expect -- e.g. using both static & DB cache, using the prefetched "cache_bootstrap" cache tier. The key logic for managing the cache is in:

http://api.drupal.org/api/drupal/includes!module.inc/function/module_implements/7

A few interesting notes:
* In module_invoke_all(), they use module_implements() to provide a smaller candidate pool, but they *also* call function_exists() against each candidate. This probably helps in corner cases where the cache is stale (e.g. if the module files were manually removed from source tree but the cache was left alone, then module_implements provides unreliable data; the extra function_exists prevents fatal errors.)
* When updating the cache, the update is delayed until the end of the process (e.g. drupal_page_footer() calls module_implements_write_cache()).

jcasharpe · May 10, 2013, 03:25:02 pm

Its also worth noting that for Drupal 8 this has all been delegated to Symfony: http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/function/module_implements/8

So it might just be a case that we wait until CiviCRM uses Symfony to revisit the hooks?

CiviCRM Community Forums (archive)

News:

Author Topic: runHooks performance (Read 631 times)

jcasharpe

runHooks performance

totten

Re: runHooks performance

jcasharpe

Re: runHooks performance

totten

Re: runHooks performance

xavier

Re: runHooks performance

Donald Lobo

Re: runHooks performance

totten

Re: runHooks performance

jcasharpe

Re: runHooks performance