[New rule] Do not use in_array

[ihor-sviziev]

Rule

in_array creating performance issue in many places when it used a lot of times.

Reason

Performance diff: https://stackoverflow.com/questions/13483219/what-is-faster-in-array-or-isset

It causes performance issues:

• Optimize performance by using isset() instead of in_array() magento2#24677
• Elastic Search 5 Indexing Performance Issue magento2#20966
• Fix newsletter queue subscribers adding performance magento2#4088

Implementation

[lenaorobei]

I would say even: "Do not use in_array in the loop".
ForeachArrayMergeSniff can be extended and renamed in order to perform this check
https://github.com/magento/magento-coding-standard/blob/develop/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.md

[ihor-sviziev]

I would say even: "Do not use in_array in the loop".
ForeachArrayMergeSniff can be extended and renamed in order to perform this check
https://github.com/magento/magento-coding-standard/blob/develop/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.md

We could start from this.

But actually in most cases it’s quite obvious that it could cause performance issues in a loop, while real performance issues becoming on integration phases, when one method that contains in_array for few elements just called few thousands times. Example related to magento tedious/JShrink#90

[lenaorobei]

The issue was discussed during the magento/architecture#290 and it was decided to not to introduce rule that may cause false-positive findings because its static nature (cannot measure actual performance).

[andrey-legayev]

Sure, in_array() is a general practice in PHP and typical answer to question "how to kill performance"
Should I revoke my pull requests?

[lenaorobei]

@andrey-legayev you are very welcome to join next public architecture discussion and share your thoughts. We are open to any improvements in case they provide a real value.

[andrey-legayev]

How to optimize magento2 performance:

• run profiler
• look for in_array
• fix it

This is what I do during last 2 weeks.
How to prevent people not to use in_array() for 2000 elements? sometimes in loop...

[lenaorobei]

I do understand you point and the reason that stays behind the use of in_array in loop.
Coding standard rules perform static file analysis and know nothing about array size. The reason why we closed this issue is because PHP CodeSniffer in not appropriate tool to perform such kind of check.

[buskamuza]

@andrey-legayev , I'd better promote better practices of using loops in general. There are many things that can kill performance, and in_array() is not the only one. IMO, a real human (developer) should think about performant implementation of loops. A general rule is usually:

• remove anything repeatable outside of a loop

This is not about in_array() calls, and it's hard to cover all use cases with static checks. For example, retrieving value from another service. Anything that doesn't rely on the array item we are iterating, should be move outside of the loop.

In the same time if you have an array of two items you're iterating through, and you do in_array() for another array of three items, most likely you don't care about performance here. And if in_array() adds readability, I'd rather keep it there.

[andrey-legayev]

I measured in_array() performance for 1 element and isset() is still be faster.
Sure, I had to run code 100k times to see time difference, but if you use it in some base class of framework for.. readability? and then call it through 20 lines of call stack 200k times. Does it make sense? IMO not.
But feel free to use it for readability.

[andrey-legayev]

For example:
Magento\Framework\Module\ModuleList::sortBySequence()
I'm sure in_array() is used inside double loop for readability. Performance isn't needed there, readability is better.

[lenaorobei]

@andrey-legayev I'm afraid, we are talking about different things there. Let me try to explain. There are two statements we are trying to mix:

• in_array in loop causes performance issues. Indeed, issues that you provided need to be fixed, there is no discussion regarding this and no objections. Hope, we are on the same page regarding this.
• Static check for the in_array in loop detection. Current issue proposes to implement PHP CodeSniffer rule for in_array in loop detection. And here we have contra-versional thoughts, because static detection uses tokens and cannot detect performance issue.

That's why it was proposed to investigate and find more appropriate tool for such check.