Drupal has about 118 calls to is_array() in core. Many could be short-circuited and never called by checking if the variable is set. More could be eliminated if arguments were either arrays or were not.

Making these changes should enhance performance.

See http://www.php.lt/benchmark/phpbench.php.

is_array() is 3 to 7 times slower than isset().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chris Johnson’s picture

Title: is_array() slower than isset() or empty() » Initial patch
FileSize
14.03 KB

Here's a patch which speeds up all the is_array() calls where it was obvious to me that a short-circuit could be built for it.

Some is_array() calls were left untouched as they were ok. A few others were left untouched because I was unsure of all the circumstances that they might be called in.

Chris Johnson’s picture

Title: Initial patch » Setting status to patch (code need review)
Status: Active » Needs review
shouchen’s picture

Title: Setting status to patch (code need review) » is_array() slower than isset() or empty()
chx’s picture

Status: Needs review » Needs work

if ($terms['tags']) { // fixme I doubt this is needed :)

Also, in pushbutton page.tpl.php the is_array is not needed. In bluemarine it's if ($primary_links) this needs a look into menu_primary_links about its return values. That returns either NULL or an array. This tells us that both core themes needs an isset and the same for secondary.

chx’s picture

Also, in form_render the if is_array check is redundant. The whole function presumes it's an array.

Chris Johnson’s picture

Title: is_array() slower than isset() or empty() » Revised and corrected patch
FileSize
16.16 KB

Revised patch should handle all issues raised.

Chris Johnson’s picture

Title: Revised and corrected patch » is_array() slower than isset() or empty()
Status: Needs work » Needs review

Woops, accidentally changed the title. (and forgot to update the status).

chx’s picture

Status: Needs review » Reviewed & tested by the community

nice.

Dries’s picture

Did you do some benchmarks? Any benchmark results?

Chris Johnson’s picture

Dries: No, I haven't done any system-wide benchmarks on this. But on a head-to-head comparison of "isset($i) && is_array($i)" versus "is_array($i)" alone, where $i is not set, the former is 2 to 7 times faster.

Some ab benchmarks with and without the patch probably would seem the most direct way to do it.

Volunteers? I'm rather busy. :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks.

This patch had 2 parse errors; please try to be a tad more careful when marking this as 'ready to be committed'. Thanks.

Tobias Maier’s picture

Status: Fixed » Active

you forgot one isset() at chameleon.theme

@@ -55,12 +55,12 @@ function chameleon_page($content) {
[...]
-    if ($primary_links) {
+    if (isset($primary_links)) {
       $output .= '<div class="primary">'. $primary_links .'</div>';
     }
-    if ($secondary_links) {
+    if (($secondary_links)) {
       $output .= '<div class="secondary">'. $secondary_links .'</div>';
     }
[...]

at if (($secondary_links)) {
it has to be if (isset($secondary_links)) {

Chris Johnson’s picture

Status: Active » Reviewed & tested by the community
FileSize
825 bytes

Odd. The parens made it for the function call, but not the function name. Anyway, here's a patch that fixes that.

Cvbge’s picture

The patch probably won't apply because of changed $id header.

Cvbge’s picture

probably won't -> might not

Gábor Hojtsy’s picture

Ehem, this patch made the revisions update broken (the update never runs, the update process hands on forever). I offered a patch as part of my patch pack for the update process, which fixes several critical issues now.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)