[RFC|Rejected] Do not use sizeof to check for empty arrays a

[brunoais]

I'm requesting 2 changes in the coding guidelines.
The changes are simple. They are supposed to improve phpbb's performance without changing its functionality.
No1:
Instead of using:

Code: Select all

if (sizeof($some_array))

Use:

Code: Select all

if (empty($some_array))

The reason behind this is that sizeofis O(n) and requires more testing and working. empty() is optimized to do just the simple test of "is array? If it is, is it empty?"

Some small tests using 2 while loops (one for each one of this situations) and 5000 cycles shows that:
0.21661281585693
vs
0.055058002471924

And a test without while loop show (this is the most favorable result to sizeof):
9.9897384643555E-5
vs
2.8133392333984E-5 <- May I say... This number is very stable between tests.

No2.

Code: Select all

if (strpos($string, '?') === 0)

Where '?' is any string composed by 1 character.
Use:

Code: Select all

if ($string[0] === '?')

The reason behind this is that strpos is O(n) and is a function (2 penalties). Accessing the substring of 1 character in a string where we know where the caracter is, is faster if you use the string as an array instead of using the strpos().

Some small tests using 2 while loops (one for each one of this situations) and 5000 cycles shows that:
0.30220890045166
vs
0.12246012687683

And a test without while loop show (this is the most favorable result to strpos):
8.0108642578125E-5
vs
2.6941299438477E-5 <- May I say... This number is very stable between tests.


These two tests show that what I'm suggesting is a faster way of executing the code than the way it appears in many places in the current phpbb's source code.

The changes I'm requesting is based on these sentences:
- sizeof() is a function used to check the size of an array or a string, it's not supposed to be used to check if an array has elements. There are better ways of doing that at the moment.
E.g.
These are wrong:

Code: Select all

if (sizeof($array))
if (sizeof($array) > 0)

This is right:

Code: Select all

if (!empty($array))

This is also right:

Code: Select all

if (sizeof($array) > 1)

same as this:

Code: Select all

if (sizeof($array) > 30)

- strpos is a function to find where the start of a substring exists in a string. If the string you are comparing has, at least, the size of the position of the character where you want to test and you know that the string given is not supposed to be the empty string then use the equality of two strings. E.g
These are all wrong:
If you know the string has, at least, 1 character

Code: Select all

if (strpos($string, '!') === 0)
if (strpos($string, 'g') === 0)

If you know the string has, at least, 5 characters

Code: Select all

if (strpos($string, '6') === 5)
if (strpos($string, 'j') === 5)

These are right:

Code: Select all

if (strpos($string, 'abc') === 0)
if (strpos($string, 'atl') === 3)

These are the replacements for the wrong ones (in order):
If you know the string has, at least, 1 character

Code: Select all

if ($string[0] === '!')
if ($string[0] === 'g')

If you know the string has, at least, 5 characters

Code: Select all

if ($string[5] === '6')
if ($string[5] === 'j')

What do you say about this? Comments are welcome (well... this is an RFC, anyway)!
Edit: After some attention calls, I made some extra stuff.

[naderman]

This is a micro optimisation that is going to save so little time, it's not even worth the amount of time you spent writing this RFC, please focus on meaningful changes, that users will be able to benefit from.

[bantu]

I do think that !empty() should be used over sizeof() when checking for an empty array. However, I agree with naderman that this is not worth it for the existing codebase. Also, sizeof() on arrays is O(1), because arrays store their size.

[igorw]

What Nils and Andreas said.

[callumacrae]

This is a micro optimisation that is going to save so little time, it's not even worth the amount of time you spent writing this RFC, please focus on meaningful changes, that users will be able to benefit from.

+1

[MichaelC]

This is a micro optimisation that is going to save so little time, it's not even worth the amount of time you spent writing this RFC, please focus on meaningful changes, that users will be able to benefit from.

+1. Its like the difference between the speed of echo and print.