Opened 14 years ago
Closed 13 years ago
#2985 closed enhancement (fixed)
Performance Tune Loops by Removing Functions from Their Declarations
Reported by: | 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)
Change History (13)
#2
@
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.
#3
@
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.
#5
@
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
@
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
@
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
@
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.
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.