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

loop optimization #757

Merged
merged 1 commit into from Jan 31, 2022
Merged

loop optimization #757

merged 1 commit into from Jan 31, 2022

Conversation

ging-dev
Copy link
Contributor

@ging-dev ging-dev commented Nov 6, 2021

Idea from #747

@staabm
Copy link
Contributor

staabm commented Nov 6, 2021

Could you profile whether its faster after the change?

@ging-dev
Copy link
Contributor Author

ging-dev commented Nov 6, 2021

Could you profile whether its faster after the change?


https://blackfire.io/profiles/compare/9764ff37-5b57-46a6-bff3-479fba5db29a/graph

if ($arraysToProcess[$j]->isKeysSupersetOf($arraysToProcess[$i])) {
$arraysToProcess[$j] = $arraysToProcess[$j]->mergeWith($arraysToProcess[$i]);
array_splice($arraysToProcess, $i--, 1);
$arraysToProcessCount--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does change the loop limit boundary, if it desired, why is that correct, if not, I belive this can not improve performance

Copy link
Contributor Author

@ging-dev ging-dev Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part can be reverted as I'm not sure it needs to do multiple iterations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is really faster than? Please provide microbenchmark which shows count(array) is really slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is not with the complexity O(1) of count(), the point is that referencing to where the array's length is stored tends to be slower than accessing data from a variable. This makes them meaningful in the loop.

https://3v4l.org/VCvdQ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it seems a little bit faster https://3v4l.org/i2dqh


// transform A & (B & C) to A & B & C
for ($i = 0; $i < count($types); $i++) {
for ($i = 0; $i < $typesCount; $i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously it was called in every loop iteration which would make it O(n) then I guess? AFAIK this one of the micro optimizations that also the EA Extended plugin for phpstorm suggests and it definitely makes sense IMO.

@ging-dev ging-dev closed this Nov 6, 2021
@ging-dev ging-dev reopened this Nov 6, 2021
@ging-dev
Copy link
Contributor Author

ging-dev commented Nov 6, 2021

well i accidentally clicked the wrong ;)

@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants