Posted by atlea on November 21, 2011 at 11:06pm
16 followers
| Project: | Omega |
| Version: | 7.x-3.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | fubhy |
| Status: | needs review |
Issue Summary
alpha_invoke is called on every THEME_preprocess and THEME_process, and if you have a few views on a page this can amount to quite a few function calls. Typically 150-500 in my tests. So this function, that runs quite a few function and file scans seems like good a good candidate for caching (esp. because of the files scans that can be slow on cloud hosting).
I was able to throw together some code that reduced the wall time for this function by about 600% on local storage. The code probably needs refactoring, so I am offering this as code for review rather than as a patch.
Atle
<?php
function alpha_invoke($type, $hook, &$vars) {
$cache_data = &drupal_static(__FUNCTION__);
if (!isset($cache_data)) {
$cache = cache_get('alpha:alpha:invoke');
$cache_data = isset($cache) ? $cache->data : array();
}
if (!isset($cache_data[$type][$hook])) {
$theme = alpha_get_theme();
// If one of the themes in the theme trail implements this hook
// include the corresponding .inc file and call the associated function.
foreach (alpha_theme_trail($theme->theme) as $key => $name) {
$function = $key . '_alpha_' . $type . '_' . $hook;
if (!function_exists($function)) {
$file = drupal_get_path('theme', $key) . '/' . $type . '/' . $type . '-' . str_replace('_', '-', $hook) . '.inc';
if (is_file($file)) {
$cache_data[$type][$hook]['files'][$key] = $file;
}
}
else {
$cache_data[$type][$hook]['functions'][$key] = $function;
}
}
$cache_data[$type][$hook]['processed'] = TRUE;
cache_set('alpha:alpha:invoke', $cache_data, 'cache');
}
if (isset($cache_data[$type][$hook]['files'])) {
foreach ($cache_data[$type][$hook]['files'] as $file) {
include $file;
}
}
if (isset($cache_data[$type][$hook]['functions'])) {
foreach ($cache_data[$type][$hook]['functions'] as $function) {
$function($vars);
}
}
}
?>
Comments
#1
Is the original alpha_invoke() be called when the standard Drupal cache is enabled?
#2
Hi,
I've attached this as a patch against 7.x-3.x. Try it, it helps with cpu, disk i/o and memory. :)
Atle
#3
Fubhy - can you review this for 3.1?
#4
For @e-anima.
A database cache as proposed in #2 is most likely not want you want.
#5
#1 yes alpha_invoke was called with all drupal core caches enabled.
i applied suns patch. now my page load dropped from around 2.1-2.5 to 1.6 - 2.0
#6
@sun: I actually only used static variable when I first did the optimization, and added caching after profiling and getting better results + I compare what is beeing done to the registry..witch is stored in cache.
Could you shine some lights as to why we would not want this stored in cache? Thanks. :)
Atle
#7
i tested suns patch on a bigger page having around 4000ms per page, all non cached, logged in as admin. it went down to 1700-2000. that is a major performance boost.
this maybe ist not obvious on a clean omega install with core only modules but on a bigger site the is_dir calls take a of of time/calls. if anyone is interested. here is an xhprof callgraph (all uncached, admin logged in).
#8
@e-anima: Hi, I am aware - I was profiling when I made the patch in #2 witch does the same as suns patch, but adds cache into the mix. By cache I don't mean page cache, like I suspect you are referring to when you say non-cached, but storing the results to the cache back-end (defaults to database, could be eg memcache) so that the is_dir-calls are not run on every page. It would be interesting to hear what results you get with the patch in #2..
As noted in the original post, this "reduced the wall time for this function by about 600% on local storage". If you are getting 2000ms, I suspect you are either not on local storage AND/OR are not using an PHP/op-code accelerator like APC (a must have for Drupal with all its files/includes).
I see the functions gets called 694 times. I have seen this reach 500 in my tests. I also notice you are using context. A (unrelated) tip: Make sure you are not using the "context for all page views", but rather path set to ~admin* or similar.
Atle
#9
the context thing is interesting, i changed that. i posted the callgraph not for you, but for reference and others.
my system runs on a SSD in a virtual box vm on a debian and i have apc enabled :). i am measuring page time with devel debug output.
as of which approach to use, i am not sure. i dont have the system skill to say which one is better. lets wait for some fubhy response or so.
#10
@e-anima: It would be interesting if you could try both patches. :) I also like to run apc.php and see witch include files are loaded the most... :) sometimes moving preprocess/process/theme-stuff out of files and into code can be beneficial.
#11
Yea that patch looks good. Let's commit it.
#12
I'm not very familiar with omega but it looks like #4 would still make quite many calls to is_file() for non-existent files. Instead of calling alpha_invoke() and resolving implementations on every page load, would it be possible to add these (hook_alpha_*) functions and includes into theme registry?
#13
#12: Yes, I have thought about that too. I will write some code and see how that goes later this evening.
#14
@olli: #2 use same approach as theme registry..
#15
We don't want to create something similiar as the theme registry. That would be wrong. We want to incorporate it into the existing theme registry.
#16
@fubhy: That would be better! ;) One could make hook_theme() scan files/functions to add them to the returned array (that is used to build the registry) to keep the dynamic nature. But the way .inc files are right now it would not work, as they would have to spesify the function/hook in them. So sites inserting just the "inside" php-code would have to wrap that code in the hook.
btw; if making something "similar" to how core does it is wrong, I don't think omega would exist. ;) But I agree on this one.. I don't even think this is needed. Sub themes can have their own hook_theme() and spesifiy their own include files in there if they wish to put it outside of template.php. The whole idea of process/preprocess directories is nice - but the added overhead that it creates + incompatibility with theme registry (and possibly _alter functions) is not.
#17
let's get this one wrapped up and a "final" patch ready to go so this can get out the door. If nothing else comes before 3.2 is ready, the patch from #4 will go in as it does help with performance based on tests.
If there's a better way to get it done, then I'm all ears.
#18
If 3.2 still supports the "inside" method atlea is referring to, then #4 needs work. If the method is no longer supported, then we could remove alpha_invoke() and use theme registry alone. As atlea pointed out it is currently not possible to use the ordinary 'includes' in theme registry. However, it would be possible to add our own element 'alpha invoke' into theme registry and use that in alpha_invoke() .
#19
This is what I would suggest:
<?php
/**
* Implements hook_theme_registry_alter().
*
* Allows subthemes to use preprocess / process code in separate files to keep
* the main template.php file clean. This is really fast because it uses the
* theme registry to cache the pathes to the files that it finds.
*/
function alpha_theme_registry_alter(&$registry) {
foreach (alpha_theme_trail($GLOBALS['theme_key']) as $key => $theme) {
foreach (array('preprocess', 'process') as $step) {
$path = drupal_get_path('theme', $key) . '/' . $step;
// Only look for files that match the 'something.preprocess.inc' pattern.
$mask = '/.' . $step . '.inc$/';
// This is the length of the suffix (e.g. '.preprocess') of the basename
// of a file.
$strlen = -(strlen($step) + 1);
// Recursively scan the folder for the current step for (pre-)process
// files and write them to the registry.
foreach (file_scan_directory($path, $mask) as $item) {
$hook = substr($item->name, 0, $strlen);
if (array_key_exists($hook, $registry)) {
// By adding this file to the 'includes' array we make sure that it is
// loaded before it is being executed.
$registry[$hook]['includes'][] = $path . '/' . $item->filename;
// Append the included preprocess hook to the functions array.
$registry[$hook][$step . ' functions'][] = $key . '_preprocess_' . $hook;
}
}
}
}
}
?>
Can't create a patch right now as I don't have my IDE with me (this was actually coded in the comment itself :P).
#20
Mhmm. Looks like that works. And the footprint is 'zero' (even when actually rebuilding the theme registry).
#21
we could actually do something similiar with theme_ functions (THEME_PATH/theme/pager_next.theme.inc). Works nicely too.
#22
This would work I believe:
<?php
/**
* Implements hook_theme_registry_alter().
*
* Allows subthemes to split preprocess / process / theme code across separate
* files to keep the main template.php file clean. This is really fast because
* it uses the theme registry to cache the pathes to the files that it finds.
*/
function alpha_theme_registry_alter(&$registry) {
foreach (alpha_theme_trail() as $key => $theme) {
foreach (array('preprocess', 'process', 'theme') as $type) {
$path = drupal_get_path('theme', $key);
// Only look for files that match the 'something.preprocess.inc' pattern.
$mask = '/.' . $type . '.inc$/';
// This is the length of the suffix (e.g. '.preprocess') of the basename
// of a file.
$strlen = -(strlen($type) + 1);
// Recursively scan the folder for the current step for (pre-)process
// files and write them to the registry.
foreach (file_scan_directory($path . '/' . $type, $mask) as $item) {
$hook = substr($item->name, 0, $strlen);
if (array_key_exists($hook, $registry)) {
// By adding this file to the 'includes' array we make sure that it is
// loaded before it is being executed.
$registry[$hook]['includes'][] = $path . '/' . $type . '/' . $item->filename;
if ($type == 'theme') {
// Remove the template file reference as this is now handled by a
// theme function.
unset($registry[$hook]['template']);
$registry[$hook]['type'] = 'theme_engine';
$registry[$hook]['theme path'] = $path;
$registry[$hook]['function'] = $key . '_' . $hook;
}
else {
// Append the included preprocess hook to the array of functions.
$registry[$hook][$type . ' functions'][] = $key . '_preprocess_' . $hook;
}
}
}
}
}
}
?>
#23
Yes, that might work. It would also deprecate alpha_invoke and the "inside" method.
#24
It's not possible to totally remove alpha_invoke without break compatibility (i.e. actually the functions theme_alpha_(pre)process_hook can live either in the hook-(pre)process.inc file or outside it. I choose to keep alpha_invoke but do not add files from it, keeping it lightweight.
This attached patch contains files inclusion by theme_registry from #22 while keep compatibility from actual release.
#25
+++ b/alpha/template.php@@ -53,6 +53,39 @@ function alpha_theme($existing, $type, $theme, $path) {
+ // Recursively scan the folder for the current step for (pre)process
+ // files and write them to the registry.
+ foreach (file_scan_directory($path . '/' . $type, $mask) as $item) {
Do we need to scan recursively?
+++ b/alpha/template.php@@ -53,6 +53,39 @@ function alpha_theme($existing, $type, $theme, $path) {
+ $hook = substr($item->name, $type_length);
+
+ if (array_key_exists($hook, $registry)) {
Should be $hook = strtr(substr, '-', '_').
#26
subscribing
#27
I don't think it hurts. But since I want to create a follow up patch for #1549792: Add support to preprocess.inc and process.inc that should scan directory for every request, I think it's better to don't be recursive there (for performance) and here (for consistency).
You're right.
Here is the rerolled patch with those changes.
#28
Forgot the "Recursively" in the comment.
#29
Well, damn arguments order... Now this should work.
#30
#31
Is this going to get commited? I have been doing some profiling on a cloud environment and this would be really helpful to reduce the load times. I did notice during my profiling that the Admin module slows down Omega. It is due to the way the Admin module builds it's menu. With Admin module enabled alpha_invoke is called hundreds of times with a type of "context_block_browser_item". Not sure you care but I did notice a thread I cannot dig up about Omega performance and noticed that whomever was complaining about performance had the admin module enabled.
Looking forward to a commit as I love Omega and would like to convince the higher ups that it is the best base theme for Drupal ever created.
#32
jepedo - I'm working on a 3.2 release that will include this alongside a bunch of other smaller fixes that are from the issue queue. There's a lot of larger omega sites popping up so I'm sure they'd love to have this bit of savings in the overhead
#33
Any details on when you plan to release the new version. Is there anything I can do to help the process along? I can help from a coding, profiling, benchmarking or testing perspective.