Reduce alpha_invoke performance impact

Is the original alpha_invoke() be called when the standard Drupal cache is enabled?

Fubhy - can you review this for 3.1?

For @e-anima.

A database cache as proposed in #2 is most likely not want you want.

#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

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

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.

Yea that patch looks good. Let's commit it.

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?

#12: Yes, I have thought about that too. I will write some code and see how that goes later this evening.

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.

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.

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() .

This is what I would suggest:

/**
 * 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).

Mhmm. Looks like that works. And the footprint is 'zero' (even when actually rebuilding the theme registry).

we could actually do something similiar with theme_ functions (THEME_PATH/theme/pager_next.theme.inc). Works nicely too.

This would work I believe:

/**
 * 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;
          }
        }
      }
    }
  }
}

Yes, that might work. It would also deprecate alpha_invoke and the "inside" method.

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.

+++ 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, '-', '_').

subscribing

Do we need to scan recursively?

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

Should be $hook = strtr(substr, '-', '_').

You're right.

Here is the rerolled patch with those changes.

Forgot the "Recursively" in the comment.

Well, damn arguments order... Now this should work.

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.

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

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.

Has this been implemented in 4.x?

Omega 4.x does this completely different without any performance hit. And Omega 4.x doesn't have a "alpha" base anymore anyways. So... yeah... kinda

Did anybody ever try the patch in #30 in production?

The patch no longer apply. We need a reroll.

Found this issue when profiling CK2 and the hog this function is. I feel that the patch in #30 is a breaking change, isn't it? As it changes how the API for the theme was implemented. Anyone implementing alpha invokes will lose changes. What if, instead, we were able to write the known files into the cache table and just load that? On registry rebuild we just run the cache set.

It's hacky. But so is alpha_invoke. And it doesn't implement a breaking change.

Here is a patch which uses registryt alter to cache the possible hooks and their files so there isn't an insane amount of file checking. Down from ~230ms to ~35ms on the total calls.

Adding a patch which applies against 3.1 release tag. #39 only applies against -dev.

Sorry, I'm totally not a developer but I'm interested in a performance boost of all our omega 3 websites if possible.

The number of omega 3 installs is still rising as I can see (while omega 4 & 5 is reducing). We also use omega 3 as a starting point in a multisite setup that was initiated years ago. So I really hope there will be a (co)maintainer found for the immense amount of omega 3 users still using it today.

@mglaman do you have your patch running on a production site?

I will try to apply the latest patch locally, but would also love to hear what other developers/maintainers think of approach #39. Is it candidate for a 3.2 release perhaps?

Applied patch #40 against omega 3.1 successfully.