Cache mailsystem_get_classes() between requests

This seems to be an issue related to mailsystem, not to Kickstart...

It seems slow on my system too so I've just added some caching into the mailsystem_get_classes function.

So now the top of the function looks like this:

$mailsystem_classes = &drupal_static(__FUNCTION__);
if (!isset($mailsystem_classes) && $cache = cache_get('mailsystem_get_classes')) {
    $mailsystem_classes = $cache->data;
}

And added this under the ksort:

cache_set('mailsystem_get_classes', $mailsystem_classes, 'cache', time() + 3600);

So the whole function looks like this: http://pastebin.com/DLHZr2eS

Same issue here. The patch attached introduces a persistent cache similar to what @dahousecat proposed.
Also added a cache clear in mailsystem_create_class().
Issue summary updated.

confirm this to work.

Working well here.

I can confirm the same diagnosis here. There's no reason for this to be called on every page load. I'm not even sure it needs to be called in hook_init() at all, but that's probably worth a separate issue.

Working +1

RTBC +1

Another high profile site that uses this in production, saves 100ms here ...

I think there's a bug in the patch. The bug does not keep the patch from working, or from being an improvement, but rather simply invalidates the point of the static cache.

I believe this line

  if (!isset($mailsystem_get_classes) && $cache = cache_get('mailsystem_get_classes')) {

should be like this:

  if (!isset($mailsystem_classes) && $cache = cache_get('mailsystem_get_classes')) {

$mailsystem_get_classes variable is never declared in the preceding function code so the first evaluation will always be TRUE. So this "works" but you don't get the real benefit of the statically cached variable.

@erikwebb

There's no reason for this to be called on every page load. I'm not even sure it needs to be called in hook_init() at all

I have to say I agree. I don't see why this has to happen on every _init(). Say you just want to run a drush command like drush sql-cli etc. Why should this mailsystem_get_classes() be fired for that? In general, my rule of thumb is if you think you have to use hook_init(), you probably shouldn't :).

Setting to needs work to address the misnamed variable.

Revised patch according to observations in comment 12.

Even though 7.x-3.x doesn't use the hook_init(), it might still benefit from this patch so it might want to be ported once approved and commited to the released version 7.x-2.x.

Back to RTBC

Deployed to hugely heavily loaded production server and saw an immediate and clear performance increase on the database side. The SELECT from registry query targeted by this patch used to represent 37% of our database server busyness. It is now down to 13%.

RTBC +1.

Attaching screenshot to show huge improvement due to this patch.

Patch works! +1 RTBC

Agreed, tested this. After clearing out some other issues this was regularly appearing as the slowest query on every page load.

What are the remaining steps to get this patch committed? Do maintainers have any concerns here?

Moving to critical as the performance suffering is real enough to make this critical.

Proposed solution should work, but why do we need mailsystem_get_classes() call from mailsystem_init() on every request in general? Class registry is re-built on every cache reset (database update, module list updates, etc) and Mailsystem module manually parses the file on new class creation. So, are there any reasons for this call here? Why not just remove this call and get speed-up even for websites with DB cache?

@Dmitriy.trt

Why not just remove this call and get speed-up even for websites with DB cache?

I agree. It should be removed. But doing so might affect other things. It's more important to fix the immediate issue, and then open a second issue for whether there should be any calls in hook_init().

This has been marked critical for 4 months and has a patch that is RTBC and ready to go. I see nothing holding this back and it would better to get this into the code and then look at the overall boostrapping picture with regards to this module.

Committed. Thank you for the patch, apotek!

#2558713: Mail system should not be using hook_init

Just wanted to mention this proposal here, since its related to performance as well, it could use a review and people following this issue would probably be interested:
#2708415: Move mailsystem persistent cache in cache_bootstrap