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

array_merge() being used on looping => docs #138

Closed
rentalhost opened this issue Feb 13, 2017 · 16 comments
Closed

array_merge() being used on looping => docs #138

rentalhost opened this issue Feb 13, 2017 · 16 comments

Comments

@rentalhost
Copy link
Collaborator

@rentalhost rentalhost commented Feb 13, 2017

EDIT: needs to be documented here


I'm getting the reporting about using of array_merge() in loopings. My question is: what is the best way to not do that?

For instance: the code below stores some data from $user, that is an array of int.

$dupesIds = [];

foreach ($users as $user) {
    // ...
    $dupesIds = array_merge($dupesIds, $user->some_ids_array);
}
@kalessil
Copy link
Owner

@kalessil kalessil commented Feb 13, 2017

Itshould be like this (saves looooots of memory allocation rounds, what leads to better performace as well)

$dupesIds = [];
foreach ($users as $user) {
    $dupesIds []= $user->some_ids_array;
}
$dupesIds = call_user_func_array('array_merge', $dupesIds)

@rentalhost
Copy link
Collaborator Author

@rentalhost rentalhost commented Feb 13, 2017

I do some tests, and the problem is not the memory (in this case, inside looping is better), but the CPU usage (it's a looooooooot worse).

Can you put your example on plugin (description)? It can help to users to follow it. Currently description is very unclear about other options. 😄

From plugin: Analyzes loops and reports if any resource-greedy array functions are being used (e.g. 'array_merge(...)').


Some benchmarking (7.000 iterations):

Case 1: with array_merge() on looping (http://pastebin.com/acEdVLSE).
Note: over 7.000 iterations is impossible run this code on my PC. haha

Time: 953.20ms
Memory: 8.00Mb
Mem. Peak: 12.00Mb

Case 2: with array_merge() outside the looping with call_user_func_array() - following your example (http://pastebin.com/QAqSDfEK).

Time: 3.00ms (-99.68% compared to case 1)
Memory: 2.00Mb (-75.00%)
Mem. Peak: 6.00Mb (-50.00%)


Extra case: with array_merge() outside the looping with call_user_func_array() - 250.000 iterations.
You can note that the memory usage is high, because you are storing a big array, instead of merging it in each looping, but it is more fast than first case.

Time: 112.80ms
Memory: 12.00Mb
Mem. Peak: 82.00Mb

@kalessil
Copy link
Owner

@kalessil kalessil commented Feb 13, 2017

You also can try prefixing call_user_func_array with root NS: '\call_user_func_array(...)' ;)

@kalessil
Copy link
Owner

@kalessil kalessil commented Feb 13, 2017

It can gain some extra execution time benefits in PHP7+

@rentalhost
Copy link
Collaborator Author

@rentalhost rentalhost commented Feb 13, 2017

No changes. I guess that it's a micro-micro optimization haha. Running on PHP 7.1.

\count(\call_user_func_array('array_merge', $idsArray));

Time: 112.80ms (same time)
Memory: 12.00Mb (same memory)
Mem. Peak: 82.00Mb (same memory peak)

@kalessil
Copy link
Owner

@kalessil kalessil commented Feb 13, 2017

Okay, I think opcache is not used during the benchmarking. In the case opcache does pretty neat optimizations )

@kalessil kalessil changed the title array_merge() being used on looping array_merge() being used on looping => docs Feb 14, 2017
@kalessil kalessil added this to the 2.3.3 milestone Feb 14, 2017
@kalessil kalessil removed their assignment Feb 17, 2017
@kalessil kalessil added this to the 2.3.4 milestone Feb 20, 2017
@kalessil kalessil removed this from the 2.3.3 milestone Feb 20, 2017
@kalessil
Copy link
Owner

@kalessil kalessil commented Feb 23, 2017

Documented here, feedback is welcome.

@kalessil kalessil closed this Feb 23, 2017
@rieschl
Copy link

@rieschl rieschl commented May 23, 2017

Sorry for posting in a closed issue but I think it's worth mentioning that PHP will throw a Fatal error (ArgumentCountError) if the foreach loop isn't executed at least once:

array_merge() expects at least 1 parameter, 0 given

something like

if (!empty($options) {
    $options = array_merge(...$options);
}

could be inserted

@kalessil
Copy link
Owner

@kalessil kalessil commented May 23, 2017

No problems, it's pretty handy actually as the context already here - I'll update the docs.

    $options = [[]]; // the inner array prevents errors when invoking the array_merge after loop
    ...

kalessil added a commit that referenced this issue May 23, 2017
@kalessil
Copy link
Owner

@kalessil kalessil commented May 23, 2017

@rieschl : I updated the documentation, thanks for reporting =)

@akrz
Copy link

@akrz akrz commented Jun 12, 2019

How about rewriting something like this without array_merge?

foreach ($results as $key => $value) {
    $results[$key]['breakdown'] = \array_merge($results[$key]['breakdown'], $extraAccessorials);
}

Basically, merging $extraAccessorials to all elements of $results

@rentalhost
Copy link
Collaborator Author

@rentalhost rentalhost commented Jun 12, 2019

@akrz depending of your PHP version (requires 5.6+), you could do something like that:

$breakdownRaw = [];

foreach ($results as $key => $value) {
    // $extraAccessorials should be assigned at some point before.
    $breakdownRaw[] = $extraAccessorials;
}

$results[$key]['breakdown'] = \array_merge($results[$key]['breakdown'], ... $breakdownRaw);

Example: https://ideone.com/rmZ5jW (will reproduces [1, 2, 3, 4, 5, 6, 7, 8, 9]).

@akrz
Copy link

@akrz akrz commented Jun 12, 2019

@rentalhost Thanks for responding.
That will merge only to the last element of $results. I want to merge to ALL elements.

Example of what I want: https://ideone.com/jgWMWo

With your suggestion, $extraAccessorials will end up in the last element of $results:
https://ideone.com/f8yF5k

@rentalhost
Copy link
Collaborator Author

@rentalhost rentalhost commented Jun 12, 2019

Oh! Now I understand your case.

So I think that it should be just ignored by @noinspection. Except if we could find a pattern where it will not kill this inspection. (Maybe ignore if array_merge() is merging an most external scope variable?)

@kalessil
Copy link
Owner

@kalessil kalessil commented Jun 29, 2019

@akrz : this is definitely a false-positive, I created #1348 bug-report.

@fialhoFabio
Copy link

@fialhoFabio fialhoFabio commented Nov 11, 2020

Spread comes to save the world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants