Avoid calling function repetitively in loop conditionals for better performance

[lfluvisotto]

Description

Avoid calling function repetitively in loop conditionals for better performance
Function sizeof replaced by count

[magento-engcom-team]

Hi @lfluvisotto. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on Pull Request changes
• @magento-engcom-team give me new test instance - deploy NEW test instance based on Pull Request changes
• @magento-engcom-team give me {$VERSION} instance - deploy Vanilla Magento instance for Issue or Pull Request

For more details, please, review the Magento Contributor Assistant documentation

[lfluvisotto]

@orlangur, @osrecio and @ihor-sviziev

Let me know when this PR gets merged then I will create the forwardport (2.3-develop) and backport (2.1-develop)

[orlangur]

Hi @lfluvisotto, please check #16027 (comment) on this.

[lfluvisotto]

@osrecio what is your opinion? what do you think? it's relevant or not?

[lfluvisotto]

Also @sidolov what is your opinion? what do you think? it's relevant or not?

[lfluvisotto]

Also @okorshenko what is your opinion? what do you think? it's relevant or not?

[lfluvisotto]

@orlangur

sizeof is an alias for count, in the opcode will be a new line or memory address whatever.

Have a look at this benchmarch on Counting LoopsFor-loop test section:

http://phpbench.com/

[ihor-sviziev]

@orlangur @lfluvisotto specially I don't mind to accept such changes, but definitely these changes are manual and someone will add the same today or maybe tomorrow and no one will notice it.

We had few examples of tries to improve code in general, but as result they were rejected:

• [PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre #9367
• CS: reapply fixers #12192
• Fix code style issues with php-cs-fixer #13270

Here were few reasons:

1. these PRs are manual and not adding any checking that we didn't introduced such issues again, so root of the issue is not fixed
2. it covers a lot of changes in different places of magento, so it will be hard to do regression change in path release, it will be much better to add it to 2.3-develop branch, where full regression testing will be executed during preparing release
3. usually such PRs are really huge, it will take a lot of time to accept them and as result - unable to merge, because there are a lot of another PRs that getting merged every day that are conflicting with these changes

PS: remove function use might be automated, example: magento-engcom/php-7.2-support#31

If you have any questions - feel free to contact me in Slack

[orlangur]

Hi @lfluvisotto,

Have a look at this benchmarch on Counting LoopsFor-loop test section:
http://phpbench.com/

As I mentioned, please compare benefit of this change with the loop body execution time. You'll notice there is no measurable difference which would require such code complication. This fact won't change no matter how many people you will try to bother with review request :)

@ihor-sviziev my main concern here is that we should not waste time on processing useless changes, especially when they slightly bloat the code. #9367 falls out of the list, I'll complete is, it was not about php-cs-fixer application actually.

[lfluvisotto]

@orlangur

You can use for that in the CICD.

https://github.com/kalessil/phpinspectionsea/blob/master/RULES.md

Inspections Lists (Performance) > SlowArrayOperationsInLoopInspection > Slow array function used in loop

[orlangur]

@lfluvisotto this inspection is wrong as it is not a "slow" array function :)

How can we use PhpStorm plugins on CICD?

[lfluvisotto]

What is the tool used for CI/CD? Travis CI, right?

[orlangur]

Yep. And also Bamboo/Jenkins internally.

[lfluvisotto]

I will have a look tonight how to call phpinspectionsea in Travis CI.