Smarty is slowing down our civicrm

[ojkelly]

Hi,

Over the last few months we've noticed some problems with Civicrm, that I suspect are all related to Smarty.

Symptoms
- uncached pages templated by smarty take longer to render than on the demo site.
- sometimes a page will load missing a field
- sometimes a form will not validate complaining about a required field that is not on the rendered form

I've also found a bunch of these errors in our logs. The template changes each time, and there's no pattern to the template, indicating it's not likely the template itself failing.

Code: [Select]

Warning: include(): Failed opening ‘/srv/bindings/REDACTED/files/civicrm/templates_c/en_US/%%DF/DF4/DF4B6A64%%PremiumBlock.tpl.php’ for inclusion
(include_path=‘/srv/bindings/REDACTED/code/sites/all/modules/custom/gift_memberships/:/srv/bindings/REDACTED/code/sites/all/civicrm_custom/:
/srv/bindings/REDACTED/code/sites/all/modules/custom/gift_memberships/:
/srv/bindings/REDACTED/code/sites/all/civicrm_custom/:
/srv/bindings/REDACTED/code/sites/all/modules/contrib/civicrm/:
/srv/bindings/REDACTED/code/sites/all/modules/contrib/civicrm//packages:
/usr/share/pear:/usr/share/php’) in Smarty::_smarty_include() (line 1909 of /srv/bindings/REDACTED/code/sites/all/modules/contrib/civicrm/packages/Smarty/Smarty.class.php).
The error still occurs when wrapped in code to check if the file exists and is readable.

I've run through this issue with Pantheon, and wrote a test script, to confirm it's not their platform. Our civicrm site runs across 6 servers, and uses a shared filesystem. I've very confident the filesystem itself is not the problem, however the way smarty uses the filesystem is still something I'm exploring.

While our live site runs on 6 servers, I can replicate this exact bug on a single server site.

I've dug a bit into the smarty code, and it's not the greatest. They're suppressing errors everywhere, I've rewritten some of it, but haven't come up with any solutions.

Any ideas on where to look to try and solve this?


Our setup:
Drupal + Civicrm 4.4.3
MariaDB
6 web servers
Redis
Valhalla filesystem, shared filesystem between servers.
Varnish
Cloudflare

Custom code:
 - custom gift_memberships module
 - membership webhook between our sites (sends webhooks after membershipo change)
 - custom templating on our front end forms

Also, having built stuff to interact with civi without using smarty I can safely say civi is awesome, smarty is painful. So any move away from smarty would be strongly supported by us.

[totten]

This problem sounds like a difficult one.

While our live site runs on 6 servers, I can replicate this exact bug on a single server site.

Does that mean either (a) you can replicate the problem using the same mix of software/configuration on the same hosting infrastructure (Pantheon) but with a smaller #servers or (b) you can replicate with a vanilla/out-of-the-box Ubuntu+Drupal+Civi dedicated server? My guess is (a) -- but that' based on trying to read-between-the-lines.

I'[m] very confident the filesystem itself is not the problem, however the way smarty uses the filesystem is still something I'm exploring.

I don't know anything about Pantheon's filesystem (Valhalla), but it does seem like a reasonable theory that there's some negative interaction between Smarty and Valhalla. At a high-level, they focus on different file usage-patterns. Valhalla seems to be focused on Drupal file-uploads (URL-1) and is described by its author as "very NoPOSIX" (URL-2). Smarty uses the filesystem as the backing store for a cache, and (compared to Drupal file-uploads) it's probably more sensitive to nuances of FS behavior (locking, timestamps, IO sequencing) which are ordinarily defined by POSIX.

How did you become very confident that the FS is not the problem? Have you reproduced the problem while running templates_c on a different filesystem (e.g. tmpfs or ext2/3/4)? If not, then that would be a good experiemnt to pursue. (AFAIK, templates_c can be put on local/non-distributed FS -- e.g. /tmp/templates_c or /var/cache/templates_c -- even if you have multiple web-servers. The only drawback is that you may need to manually flush templates_c when upgrading.)

Some other random thoughts/questions:

- If we focus on that failed include() statement, there's a third component which might merit investigation -- the opcode cache (APC/xcache/etc). All three parts (Smarty's automatic compiler, the filesystem, and the opcode cache) could be culprits. If you're able to reproduce the problem, it would be interesting to reproduce under different combinations of FS+opcode-cache.

- It would be possible to change Smarty's compilation process -- instead of auto-compiling files on-demand, one could pre-compile the files. This idea comes up from time-to-time because it could improve security, but it could also simplify Smarty's runtime (post-deployment) mechanics. It requires some combination of patching/upgrading Smarty+Civi, and (AFAIK) no one has put in the resources needed to make that happen.

(URL-1) https://www.getpantheon.com/news/inside-pantheon-valhalla-filesystem ("What Drupal actually needs")
(URL-2) http://pl.atyp.us/wordpress/index.php/2012/01/scaling-filesystems-vs-other-things/#comment-246082

[xavier]

Hi,

Chris investigated the compile_check option on smarty, did that improve things?
https://issues.civicrm.org/jira/browse/CRM-14458

You might want to see if another cache storage than the filesystem does help:
http://www.smarty.net/docs/en/caching.custom.tpl

And yes, probably not a lot of the conversations about civicrm 5.0 included any proposal to keep smarty

[ojkelly]

This problem sounds like a difficult one.

Little bit..

Does that mean either (a) you can replicate the problem using the same mix of software/configuration on the same hosting infrastructure (Pantheon) but with a smaller #servers or (b) you can replicate with a vanilla/out-of-the-box Ubuntu+Drupal+Civi dedicated server? My guess is (a) -- but that' based on trying to read-between-the-lines.

A. Though I'm in the process of testing it on a standalone vanilla ubuntu.
I know that it's not the multiple servers + load balancer causing the issue, as we can replicate it on a single server environment (albeit identical to our live environment in just about every other way).

I don't know anything about Pantheon's filesystem (Valhalla), but it does seem like a reasonable theory that there's some negative interaction between Smarty and Valhalla. At a high-level, they focus on different file usage-patterns. Valhalla seems to be focused on Drupal file-uploads (URL-1) and is described by its author as "very NoPOSIX" (URL-2). Smarty uses the filesystem as the backing store for a cache, and (compared to Drupal file-uploads) it's probably more sensitive to nuances of FS behavior (locking, timestamps, IO sequencing) which are ordinarily defined by POSIX.

How did you become very confident that the FS is not the problem? Have you reproduced the problem while running templates_c on a different filesystem (e.g. tmpfs or ext2/3/4)? If not, then that would be a good experiemnt to pursue. (AFAIK, templates_c can be put on local/non-distributed FS -- e.g. /tmp/templates_c or /var/cache/templates_c -- even if you have multiple web-servers. The only drawback is that you may need to manually flush templates_c when upgrading.)

I've chatted to Pantheon about this, and we're both now reasonably confident it's not Valhalla. The latest version they run is (I'm told) POSIX compliant, and it seems to behave in a manner consistent with that (given bug excepted).

I also wrote a script to generate php files to the filesystem and read them back based on http requests, then threw siege at it. Under 100 concurrents it could write and read the file no problem at all. The hypothesis with this was that, smarty might be writing a file to the filesystem then trying to read it again and failing.

I've also been down this rabbit hole: http://stackoverflow.com/questions/1867609/why-is-php-not-including-my-file-when-it-clearly-exists

It does seem possible to get this error in php where you cannot include a file that passes both file_exists() and is_readable() et al.

I should say, confident but not ruled out.

Some other random thoughts/questions:

- If we focus on that failed include() statement, there's a third component which might merit investigation -- the opcode cache (APC/xcache/etc). All three parts (Smarty's automatic compiler, the filesystem, and the opcode cache) could be culprits. If you're able to reproduce the problem, it would be interesting to reproduce under different combinations of FS+opcode-cache.

Opcode is one factor I haven't looked at yet, I'll let you know what I find.

- It would be possible to change Smarty's compilation process -- instead of auto-compiling files on-demand, one could pre-compile the files. This idea comes up from time-to-time because it could improve security, but it could also simplify Smarty's runtime (post-deployment) mechanics. It requires some combination of patching/upgrading Smarty+Civi, and (AFAIK) no one has put in the resources needed to make that happen.

I've tried messing with it's compilation, not sure about how to get pre-compiling going but I'll look into it. Any idea where to start?

Hi,

Chris investigated the compile_check option on smarty, did that improve things?
https://issues.civicrm.org/jira/browse/CRM-14458

Ah, that's not from when he was working for us, so I'll have to give it a shot.

You might want to see if another cache storage than the filesystem does help:
http://www.smarty.net/docs/en/caching.custom.tpl

I've partially gone down this route. Either I'm missing something about how the smarty build process works or it's always going to dump files to the filesystem. Looking through smarty it's include()'n files from templates.

And yes, probably not a lot of the conversations about civicrm 5.0 included any proposal to keep smarty

[totten]

- It would be possible to change Smarty's compilation process -- instead of auto-compiling files on-demand, one could pre-compile the files. This idea comes up from time-to-time because it could improve security, but it could also simplify Smarty's runtime (post-deployment) mechanics. It requires some combination of patching/upgrading Smarty+Civi, and (AFAIK) no one has put in the resources needed to make that happen.

I've tried messing with it's compilation, not sure about how to get pre-compiling going but I'll look into it. Any idea where to start?

Chris investigated the compile_check option on smarty, did that improve things?
https://issues.civicrm.org/jira/browse/CRM-14458

To get to a proof-of-concept, I think one would:

• In Smarty.class.php, create a new function pre_compile($resource_name). To write this, copy the code from fetch($resource_name) and trim it down -- roughly speaking, do everything up until include($_smarty_compile_path). (In the past, I've scratched my head trying to find a way to do this with the existing Smarty v2 API -- I don't think it can be done without patching. But most of the logic is already written, so it shouldn't be too hard to hack a proof-of-concept.)
• Write a scanner which calls pre_compile(). In Unix CLI, one might say: find -name '*.tpl' -exec "drush eval 'civicrm_initialize(); CRM_Core_Smarty::singleton()->pre_compile(\"{}\");'" \;
• Disable runtime compilation in Smarty (perhaps using Chris C's patch from CRM-14458).
• Deploy the pre-compiled content from templates_c to your servers.

(I'm not too worried about the maintainability of this -- if we stay on Smarty v2, then we don't have to worry about conflicting updates b/c v2 is basically EOL upstream. If we upgrade to Smarty v3 in the future, then we get comparable functionality from compileAllTemplates(). If we ditch Smarty in the future, then it won't matter.)

(A release-able version would, of course, require a bit more work+polish -- e.g. to scan extensions for *.tpl files.)

You might want to see if another cache storage than the filesystem does help:
http://www.smarty.net/docs/en/caching.custom.tpl

I've partially gone down this route. Either I'm missing something about how the smarty build process works or it's always going to dump files to the filesystem. Looking through smarty it's include()'n files from templates.

Neat idea. FWIW, Civi uses Smarty v2, so the relevant interface would be http://www.smarty.net/docsv2/en/section.template.cache.handler.func.tpl

And yes, probably not a lot of the conversations about civicrm 5.0 included any proposal to keep smarty

Yup. In the newer UIs, we've been moving towards JS+HTML+CiviAPI (without Smarty or QuickForm) - and we're hoping to accelerate that in 5.0.

[ojkelly]

UPDATE

I've modified Smarty core to set and get from the drupal cache (which in our case runs on Redis on Pantheon). We've noticed significant speed improvements - to such a degree that I think it would be very beneficial to get this into core.

This involved editing a few of the core smarty files, and removing the compile dir check from the smarty/civi integration.

I'm almost finished for the year, so I'll get the code up start of next year.

I'm not sure 100% how we could put this in core, but for those on Pantheon it's incredibly faster.

[totten]

Nice. Look forward to seeing it.

We can figure out if it needs adjustments for core when you post the code.

[ojkelly]

I've got two commits on github showing the change.

The gist is anywhere the files wrote or read from the filesystem they were changed to cache_set and cache_get. to make this non-drupal specific we could probably make a wrapper function that calls the drupal/wordpress/joomla cache function.

Change to Smarty core (unfortunately some style changes might make it harder to see what changed.
https://github.com/ojkelly/civicrm-packages/commit/4e88b13ef5263ac874c9922c2dd6c1051a59f1f0

Change to Smarty-civi integration
https://github.com/ojkelly/civicrm-core/commit/85e04b67a662c86b27811e8850f94a9daccd0a0a

[totten]

Thanks for sharing! I've done an automated cleanup and rerun the diff so that we can get a more concise view of the changes ( https://gist.github.com/totten/cf6a22150ca7944b14ee ).

In trying to read/understand the patch, a couple questions:

• There are a few new functions -- precompile(), smarty_eval(), get_file_owner(). Can you tell where these are called? Or are these orphans from prior (deleted) work?
• Some of the previous comments pointed to a Smarty interface ( http://www.smarty.net/docsv2/en/section.template.cache.handler.func.tpl or http://www.smarty.net/docs/en/caching.custom.tpl for v2 or v3, respectively) which (purportedly) allows you to hook into Smarty and change the cache behavior without a patching Smarty.class.php. The patch uses a different approach (directly replacing file I/O in Smarty.class.php with cache I/O). Did you come across some trade-off/issue -- or did this just feel like the easier way to get to a working proof-of-concept?
• At a policy level, I wonder about the merits of (a) replacing the Smarty's normal cache with a DB-backed cache vs (b) allowing the the DB-backed cache as an option.

[ojkelly]

Ah yup, precompile() is left over from building the patch, it's not needed. Smarty will regen as needed. I used it in a drupal module to regen all the templates.

Code: [Select]

function civicrm_schwartz_admin_recompile() {


    if (!empty($_SERVER['PRESSFLOW_SETTINGS'])) {
        $env = json_decode($_SERVER['PRESSFLOW_SETTINGS'], TRUE);
        $pantheon_conf = $env['conf'];
//        $dirPath = '/srv/bindings/' . $pantheon_conf['pantheon_binding'] . '/files/civicrm/templates_c';
//        foreach(new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dirPath, FilesystemIterator::SKIP_DOTS), RecursiveIteratorIterator::CHILD_FIRST) as $path) {
//            $path->isDir() && !$path->isLink() ? rmdir($path->getPathname()) : unlink($path->getPathname());
//        }
//        rmdir($dirPath);
//
        civicrm_initialize();

        $i = 0;
        $filenames = array();

        $smarty = CRM_Core_Smarty::singleton();
        $smarty->force_compile = true;

        $dir = new RecursiveDirectoryIterator('/srv/bindings/' . $pantheon_conf['pantheon_binding'] . '/code/');
        $ite = new RecursiveIteratorIterator($dir);
        $files = new RegexIterator($ite, '/.*\.tpl$/', RegexIterator::GET_MATCH);
        foreach ($files as $file) {
            watchdog('tpl', print_r($file[0], TRUE) );
            $smarty->precompile($file[0]);
            $i++;
            $filenames[] = $file[0];
        }


        return $i . "  /  " . print_r($filenames, TRUE);
    }
}
smarty_eval() is the sad piece of code that makes this work. We need to run the smarty templates through php's eval(). The wrapper function is from a snippet that makes eval run reliably. Normally running eval() slows down php, but in this instance it's significantly faster due to the bottleneck of smarty interacting with the filesystem on some hosts.

get_file_owner() is orphaned.

--

Yeah I dug into cache.handler.func and got absolutely no where. It's for a different thing. Problem with this, is that we're not actually changing the cache. I don't think you can cache smarty when using civicrm, otherwise when you update a contact their details change in the DB, but not when you reload the page.

So we're not modifying the cache at all, what we are actually doing is changing the compiled template storage from filesystem to redis/DB in this instance.

--

It's hard to say. For users with one big server running on the filesystem is likely sufficient. However as soon as you scale horizontally the filesytem lets civi down significantly.

This is not caching, it's smarty compiled template storage.

You can store it in any volatile storage you have (smarty regens if not found - you only suffer a performance hit). So redis/memcache is perfect. DB is good too if you can't use a key/value store. Though I suspect DB might be slower.

I'd suggest this should be only redis/memcache OR filesystem. As if your on one box, you're filesystem will work, and if you're scaling horizontally you will have a key/value store of some kind (you're missing out if you don't).

[ojkelly]

Also just in case, there's about 5 files changed to make this work.

[Chris Grewe]

I've got two commits on github showing the change.

The gist is anywhere the files wrote or read from the filesystem they were changed to cache_set and cache_get. to make this non-drupal specific we could probably make a wrapper function that calls the drupal/wordpress/joomla cache function.

Change to Smarty core (unfortunately some style changes might make it harder to see what changed.
https://github.com/ojkelly/civicrm-packages/commit/4e88b13ef5263ac874c9922c2dd6c1051a59f1f0

Change to Smarty-civi integration
https://github.com/ojkelly/civicrm-core/commit/85e04b67a662c86b27811e8850f94a9daccd0a0a

OJ,

First and foremost, thank you for your work on this. I'm a fellow Pantheon/Civi developer, and this has been driving me (to say nothing of our end users) up a wall for a while.

I'd like to suggest one minor tweak to your Civi patch. With where you have the commented out section starting in CRM\Core\Smarty.php, you exclude the possibility of Smarty loading any custom templates from anything other than the core template dirctories. If you move it down to between lines 99 and 100 in the original version (i.e. right above $this->compile_dir = $config->templateDir;), you get the best of both worlds: custom templates load, and they load from Redis memory.

The speed increases I'm seeing in my own testing are quite amazing. Thank you very much for sharing this!

[ojkelly]

Ah bummer I thought that fix was in that patch. I noticed the same thing with the comments so that you can still have custom directories for templates.

Great to hear it's working well for you.

I should add this patch didn't resolve our original problem that is: sometimes templates compile missing bits (like say an address field). It seems to happen 1/1000 times at most. Is it something you've come across at all?