Skip to:
Content

BuddyPress.org

Opened 14 years ago

Closed 13 years ago

#2985 closed enhancement (fixed)

Performance Tune Loops by Removing Functions from Their Declarations

Reported by: jeffsayre's profile jeffsayre Owned by:
Milestone: 1.5 Priority: normal
Severity: normal Version: 1.5
Component: Core Keywords: has-patch
Cc: jeffsayre

Description

This code construct is quite common throughout BuddyPress core:

for ( $i = 0; $i < count($array); $i++ ) {

     //stuff
}

This is bad form. Why? Because it sacrifices performance just to save a line of code.

Sure, the code may look prettier, but pretty code does not necessarily mean better code. We should focus on writing code for machine performance, not for humans to appreciate.

As a general rule, more times than not, it is a bad practice to put functions inside loop declarations.

Why is this an issue? With each iteration of the loop, the count() function gets executed. It gets executed again, and again, and again... This adds a lot of extra execution time which equals more processor usage, which equals more time before execution is complete.

How many times should the variable be counted? Once is all that's required. A single, additional line of code will turn these resource-hogging loops into more processor-friendly code blocks.

The easy, simple solution:

$count = count($array);
for ( $i = 0; $i < $count; $i++ ) {

     //stuff

}

Think this is not an important issue? As BuddyPress (v1.2.7) currently has 65 occurrences of this poor construct, there's more than likely a noticeable performance hit. With large, more active sites, the hit will be even higher. As a site's data grows with use, this performance issue increases as each array grows larger and larger, meaning that there are more elements through which to iterate and at each step trigger the count function yet again.

As there are too many places where this occurs throughout the BP code base (including the commandeered bbPress code), I'm not going to submit a patch. I do not have the time to volunteer. I just wanted to bring this up as an easy-to-address performance tuning measure.

Attachments (2)

2985.001.diff (26.6 KB) - added by cnorris23 14 years ago.
2985.002.patch (23.4 KB) - added by r-a-y 13 years ago.

Download all attachments as: .zip

Change History (13)

#1 @cnorris23
14 years ago

  • Keywords has-patch needs-testing added

Easy fix. Added some minor cleanup to the surrounding code. Everything should be good, as I went over it 3 or 4 times, but definitely needs testing.

#2 @kunalb
14 years ago

I believe that PHP caches the length of the array in PHP so it doesn't go over the array every time. However, there will some overhead in calling the function again and again, so it'll save some, though not that much, time per execution.

See this: http://www.tuxradar.com/practicalphp/18/1/7

#3 @jeffsayre
14 years ago

cnorris23 -

Thanks for your efforts. Did you upload your patch? There are no files currently attached.

kunalb -

That is correct. For a small site, it will offer little difference, but for a very large site with tens of thousands + of members--which we should assume some BuddyPress installs will have--it can make a noticeable difference. Seconds of difference are a big deal when you have a large network. This is by no means an issue with the vast majority of installs.

Remember that the amount of members a site has does not equal the number of loop iterations. It all depends on the complexity of a given array and the elements being iterated. Since many BuddyPress arrays are very complex, multi-nested structures, any fraction of time that can be shaved will improve the overall performance, and that is the goal here.

@cnorris23
14 years ago

#4 @cnorris23
14 years ago

Forgot to add the patch earlier :) It was late... I was tired...

#5 @DJPaul
14 years ago

  • Keywords dev-feedback added; performance removed

Has anyone run the numbers on this patch? Without proof that these particular count calls noticeably slow down execution, it looks like premature optimisation.

#6 @jeffsayre
14 years ago

Paul,

See the very end of the page of this resource http://www.phpbench.com/

It shows comparative results for both the count() and sizeof() functions using those functions within loop declarations and performing the calculations before the loop starts. The test was run with a loop processing only 1000 keys. This is well within range of even a small BuddyPress site.

The results are very clear.

#7 @r-a-y
13 years ago

You could also do this:

for ( $i = 0, $count = count($array); $i < $count; $i++ ) {

     //stuff

}

Read this article for reference:
http://php.net/manual/en/control-structures.for.php

#8 @DJPaul
13 years ago

  • Keywords needs-patch added; has-patch needs-testing dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Version 1.3 deleted

Patch has gone stale and needs re-doing for trunk. Would prefer to see r-a-y's variable naming suggestion as it is easier to read.

@r-a-y
13 years ago

#9 @r-a-y
13 years ago

  • Keywords has-patch added; needs-patch removed
  • Severity set to normal
  • Version set to 1.3

Please review the new patch as soon as you can to prevent needing a refresh.

Thanks!

#10 @DJPaul
13 years ago

  • Milestone changed from Future Release to 1.3

#11 @djpaul
13 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [4770]) Tune up 'for' loops. Fixes #2985, props cnorris23 and r-a-y

Note: See TracTickets for help on using tickets.