fix: cache count() results for loops

[DanielRuf]

Description

This improves the performance of a few loops which were runningcount() in every iteration.

Fixed Issues (if relevant)

Manual testing scenarios

1. ...
2. ...

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[magento-engcom-team]

Hi @DanielRuf. 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 PR changes
• @magento-engcom-team give me $VERSION instance - deploy vanilla Magento instance

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

[orlangur]

@DanielRuf such change is not needed, see #16027 (comment) for more details.

[DanielRuf]

Not 100% true. Did you check the benchmarks?

[DanielRuf]

See https://3v4l.org/WBFuK

With this the compiler can apply AOT optimizations.
In general this adds up on large shops (over 20 attributes per product and over a few thousand products).

[DanielRuf]

Keep in mind that this saves many OP codes and utilizes the AOT benefits. Anything else is bad practice in general (cache immutable values).

[orlangur]

@DanielRuf,

function call by itself is negligible compared to loop body

Please provide relevant benchmarks, in the ones provided loop body consists of just one assignment and obviously there will be a difference.

[DanielRuf]

Please provide relevant benchmarks, in the ones provided loop body consists of just one assignment and obviously there will be a difference.

See the 3v4l link, the second example in the middle uses a cached count value and you can clearly see that it takes less than the two normal above and below it (to show that the cache alone brings much).

[orlangur]

@DanielRuf,

$is2 = microtime(true);
$ia2c = count($ia2);
for($i2 = 0; $i2 < $ia2c; $i2++){
    $iaf2 = $i2;
}

Loop body is too small and nonrealistic here, just compare to a real code you changed. What I'm trying to say is that such change does not give measurable boost in a real loop while obviously it makes some difference in a synthetic test.

[DanielRuf]

Loop body is too small and nonrealistic here, just compare to a real code you changed. What I'm trying to say is that such change does not give measurable boost in a real loop while obviously it makes some difference in a synthetic test.

I'll create a OP code report using VLD in the next days. And yes it bringt better performance in big shops.

[DanielRuf]

Loop body is too small and nonrealistic here

This is how small benchmarks are generally done on the lowest level.

[DanielRuf]

It's twice as fast in most cases without any further optimizations ;-)

[orlangur]

@DanielRuf thanks, will see then.

[magento-engcom-team]

Hi @sidolov, thank you for the review.
ENGCOM-3339 has been created to process this Pull Request

[DanielRuf]

cc @sidolov

[orlangur]

@DanielRuf please do not reopen without providing a proper benchmark.

[DanielRuf]

@DanielRuf please do not reopen without providing a proper benchmark.

There is a proper benchmark. With cached count it is 50% faster.

[DanielRuf]

There is a proper benchmark. With cached count it is 50% faster.

If readability is an issue, I can add the variable into the loop definition.
Would this be ok for you then?

If we would throwaway every improvement, even these which make bigger shops and specific loops faster, we do not gain much more performance.

[DanielRuf]

Also @sidolov is from the core tram and I think his review was correct.

[DanielRuf]

If this change makes no sense for you and you decide that this should not be in master / next releases, well then I will think about wasting more of my free time to improve little parts of Magento 2 as this is a very unsatisfying experience (totally different opinions and knowledge about the inner working of the Zend engine and AOT optimization for loops).

[orlangur]

@DanielRuf this change has nothing to do with performance as I explained from the very beginning. Compared to loop body one count call on each iteration is nothing and you didn't prove an opposite.

Whether a person from core or not is irrelevant to code review when both persons have enough information. I talked to @sidolov today in Slack an he agrees that such changes are useless from performance perspective.

I'm okay with changes if you introduce a variable named exactly $count in each case like

for ($i = 1, $count =  count($line); $i < $count; $i++) {

Still I'm insisting that you should understand this change has no measurable performance impact - not 0.1%, not even 0.01%, totally negligible. So, my only intention was to not waste time and other resources on changes not improving a product.

[orlangur]

I'm okay with changes if you introduce a variable named exactly $count in each case like

for ($i = 1, $count =  count($line); $i < $count; $i++) {

[orlangur]

Great! Thanks for collaboration.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3339 has been created to process this Pull Request

[magento-engcom-team]

Hi @DanielRuf. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.