[New Rule] Slow array SHOULD NOT be used in loop

[lenaorobei]

Rule

The use of slow array functions (array_merge) in loop is discouraged.

Reason

Merging arrays in a loop is slow and causes high CPU usage. Some benchmarking.

Bad example:

    $options = [];
    foreach ($configurationSources as $source) {
        // code here
        $options = array_merge($options, $source->getOptions());
    }

Good example:

    $options = [];
    foreach ($configurationSources as $source) {
        // code here
        $options[] = $source->getOptions();
    }

    // PHP 5.6+
    $options = array_merge([], ...$options);

Implementation

• Subscribe to T_STRING token and check if it's content is array_merge.
• Try to find outer loop.
• If loop is found rase a warning (need to discuss additional logic).

<severity>7</severity>
<type>warning</type>

[paliarush]

What is going to happen with core code for which error is generated instead of warning by default?

[lenaorobei]

We can throw a warning with incremental approach.

[Vinai]

Mixed feelings. I often use merge in a reduce function to avoid changing local state (and save a line of code).
Hope PHP optimizes for more functional patterns in future, now that FP is becoming mainstream.
Even Java now has optimized map/reduce/filter chains, and more and more languages support STM.
If PHP follows, this sniff would be outdated.
Maybe sniffs should focus on algorithmic complexity instead of Runtime implementation details.

[shochdoerfer]

Slow is a matter of definition :) In a loop with just a few iterations, you most likely won't notice that much of a difference.

[Vinai]

Exactly. That’s why focusing on big O algorithmic complexity seems like a good idea since that’s assumes scalability in the worst case scenario.
Stating this I feel stupid because I’m sure you are well aware.

[himanshu8dev]

For my point of view, This is good practice for developer who don't care about script execution it will also effect on performance and in the end it will slow down the site performance, This should be implemented..

[larsroettig]

Benmarks:

• https://gist.github.com/Ocramius/8399625#file-output-txt

• https://github.com/kalessil/phpinspectionsea/blob/master/docs/performance.md#slow-array-function-used-in-loop

• array_merge() being used on looping => docs kalessil/phpinspectionsea#138 (comment)

[lenaorobei]

@buskamuza @kandy

Need you input here.

See related PR #72

[kandy]

For me, it makes sense to have this rule

[larsroettig]

Hi @kandy,
sense yes but if you look to findings maybe there are some false positives

[kandy]

@larsroettig, Definitely, there are some false positives and the warning is a good signal to review such places.

[lenaorobei]

Implemented in #72