Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize FileLocator autoloader : use foreach instead of for with count when possible #3280

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

samsonasik
Copy link
Member

Checklist:

  • Securely signed commits

@michalsn
Copy link
Member

Sorry for the silly question but is foreach faster than for? Or this is just for readability purposes?

@samsonasik
Copy link
Member Author

previously, with for, it need to run count() first, while direct foreach is enough.

@michalsn
Copy link
Member

Yes, I understand this part, but isn't foreach slower than for?

@samsonasik
Copy link
Member Author

samsonasik commented Jul 12, 2020

direct foreach is faster. Example benchmark about for with count vs direct foreach : https://coderwall.com/p/il1tog/php-for-vs-foreach-benchmark

@samsonasik
Copy link
Member Author

➜  ~ curl https://gist.githubusercontent.com/nodesocket/3909074/raw/80e1bd3612b2b1680d6d519c8fd53bb79435e9de/gistfile1.php -o benchmark-for-vs-foreach.php

➜  ~ php benchmark-for-vs-foreach.php 
For took: 2.862ms
For with count() took: 3.643ms
Foreach took: 1.489ms

@michalsn
Copy link
Member

I get quite different results to be honest

For took: 0.128ms
For with count() took: 0.175ms
Foreach took: 0.162ms

Also checked it on sandbox: https://phpsandbox.io/n/red-king-pnbr

@samsonasik
Copy link
Member Author

The code that I updated previosly uses count+for, so foreach still win

@michalsn
Copy link
Member

Not really. "For with count()" is when we use count() inside for() loop, like:

for($i = 0; $i < count($elements); $i++)

and it's not the case here.

@samsonasik
Copy link
Member Author

The benchmark code need update to associative array and run the code inside loop, I updated the benchmark code at https://phpsandbox.io/n/super-fog-72g7 with place assign count() to variable first, but execute it. I got it:

For took: 0.066ms
For with count() took: 0.167ms
Foreach took: 0.099ms

foreach wins on this case.

@michalsn
Copy link
Member

Okay, now it looks good. Thanks.

@michalsn michalsn merged commit 0e6eac7 into codeigniter4:develop Jul 12, 2020
@samsonasik samsonasik deleted the file-locator-optimize branch July 12, 2020 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants