In includes/theme.inc/drupal_theme_initialize(), finding the ancestors is done that way, accessing arrays and objects many times:

  // Find all our ancestor themes and put them in an array.
  $base_theme = array();
  $ancestor = $theme;
  while ($ancestor && isset($themes[$ancestor]->base_theme)) {
    $ancestor = $themes[$ancestor]->base_theme;
    $base_theme[] = $themes[$ancestor];
  }

I think it can optimized (and made clearer/cleaner) thus:

  // Find all our ancestor themes and put them in an array.
  $base_theme = array();
  $ancestor = $themes[$theme]->base_theme;
  while ($ancestor) {
    $base_theme[] = $themes[$ancestor];
    $ancestor = $themes[$ancestor]->base_theme;
  }

I tested it (see attached file):
- from 0 to 100 ancestors
- repeating the while from 1 to 1 million times in a row.

It constantly gave an improvement of around... 55 % !

Note: this patch also solves issue #832624 (http://drupal.org/node/832624)

Comments

LaurentAjdnik’s picture

LaurentAjdnik’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, core-theme-drupal_theme_initialize-926784.patch, failed testing.

twod’s picture

This patch assumes all themes have base themes (or at least the base_theme property).

There are several problems with the benchmarking code in the original post. I tried to run it on my AMD Phenom II X4 system with 16GB RAM and it failed miserably. It ate all RAM in a matter of seconds and was killled by the system after it was out of swap space as well, even when only using 10 base themes and 10 loops.

  • $base_theme is reset several times, but the base themes are actually added to $base_theme1 and $base_theme2, which are never reset.
  • $base_theme must be reset after each test iteration to not consume memory and affect the next iteration by being larger than before.
  • The suggested test case does not reset the $ancestor variable before each iteration, meaning the while loop is only executed once, giving that case a huge bias.
  • isset() is needed around $themes[$theme]->base_theme in the suggested method or it throws a warning for each theme.
  • This one is minor, but microtime() returns the time in seconds, not milliseconds.

I fixed the points above and added another test with the isset() calls.
I also added another test identical to the first to see if there's an error margin depending on when the test is run.
Then I ran it a couple of times and always got results similar to this.

MAX = 100;
LOOPS = 1000000;
Original method : 65.147700071335 s

Suggested method: 50.697026014328 s
Ratio = 1.285039876953
Gain  = 22% (+ warnings)

Fixed method: 73.557250976562 s
Ratio = 0.88567339326062
Gain  = -12%

Verification: 67.754120111465 s
Ratio = 0.96153119491711
Gain  = -4%

The suggested method is slightly faster, but throws the warnings.
With the added isset() calls in the "Fixed method", there is no gain compared to the original code.

Test code:

  header('Content-Type: text/plain');
  // Avoid a ton of error output.
  error_reporting('E_NONE');
  // Setup fake theme hierarchy
  class clsTheme {
    public $base_theme = NULL;
  }
  define('MAX',100); // Set to 1 for no ancestor
  define('LOOPS',1000000);
  for ($i = MAX ; $i > 0 ; $i--) {
    $theme = 'theme' . $i;
    $base_theme = 'theme' . ($i+1);
    $temp_theme = new clsTheme;
    $temp_theme->base_theme = $base_theme;
    $themes[$theme] = $temp_theme;
  }
  unset($themes['theme' . MAX]->base_theme);
  $theme = 'theme1';

  // Original method
  $base_theme = array();
  $t1 = microtime(true);
  for ($i=0 ; $i<LOOPS ; $i++) {
    $base_theme = array();
    $ancestor = $theme;
    while ($ancestor && isset($themes[$ancestor]->base_theme)) {
      $ancestor = $themes[$ancestor]->base_theme;
      $base_theme[] = $themes[$ancestor];
    }
  }
  $t1 = microtime(true) - $t1;

  // Suggested method
  $base_theme = array();
  $t2 = microtime(true);
  for ($i=0 ; $i<LOOPS ; $i++) {
    $base_theme = array();
    $ancestor = $themes[$theme]->base_theme;
    while ($ancestor) {
      $base_theme[] = $themes[$ancestor];
      $ancestor = $themes[$ancestor]->base_theme;
    }
  }
  $t2 = microtime(true) - $t2;

  // Fixed method
  $base_theme = array();
  $t3 = microtime(true);
  for ($i=0 ; $i<LOOPS ; $i++) {
    $base_theme = array();
    $ancestor = isset($themes[$theme]->base_theme) ? $themes[$theme]->base_theme : FALSE;
    while ($ancestor) {
      $base_theme[] = $themes[$ancestor];
      $ancestor = isset($themes[$ancestor]->base_theme) ? $themes[$ancestor]->base_theme : FALSE;    }
  }

  $t3 = microtime(true) - $t3;

  // Original method again for verification 
  $base_theme = array();
  $t4 = microtime(true);
  for ($i=0 ; $i<LOOPS ; $i++) {
    $base_theme = array();
    $ancestor = $theme;
    while ($ancestor && isset($themes[$ancestor]->base_theme)) {
      $ancestor = $themes[$ancestor]->base_theme;
      $base_theme[] = $themes[$ancestor];
    }
  }
  $t4 = microtime(true) - $t4;
  
  echo "Original method : " . $t1 . " s\n\n";

  echo "Suggested method: " . $t2 . " s\n";
  echo "Ratio = " . ($t1 / $t2) . "\n";
  echo "Gain  = " . (int)((100 * ($t1 - $t2)) / $t1) . "%\n\n";

  echo "Fixed method: " . $t3 . " s\n";
  echo "Ratio = " . ($t1 / $t3) . "\n";
  echo "Gain  = " . (int)((100 * ($t1 - $t3)) / $t1) . "%\n\n";

  echo "Verification: " . $t4 . " s\n";
  echo "Ratio = " . ($t1 / $t4) . "\n";
  echo "Gain  = " . (int)((100 * ($t1 - $t4)) / $t1) . "%\n\n";

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.