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

[5.8] Arr::collapse better performance #28662

Merged
merged 4 commits into from May 30, 2019

Conversation

Projects
None yet
4 participants
@awais-vteams
Copy link
Contributor

commented May 30, 2019

For large array to collapse() get better performance.

Ubuntu 14.04 Core™ i3-4130 CPU @ 3.40GHz × 4, 8GB

$arr = [];
for ($i = 0; $i < 20000; $i++) {
    $arr[] = [1, 2, 3];
}
Arr::collapse($arr);

Before: 6.88 s
After: 0.03 ms

awais-vteams added some commits May 30, 2019

@awais-vteams awais-vteams reopened this May 30, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I don't get it... what is the purpose of merging two empty arrays at the end.

@awais-vteams

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

I don't get it... what is the purpose of merging two empty arrays at the end.

without empty array, PHP 7.3 gives notice to array_merge() expect one parameter to be an array

@taylorotwell

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Is this actually the exact same behavior as the earlier array_merge logic?

@awais-vteams

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

I don't get it But I believe this really time breaking performance in Arr::collapse function You can test run it.

@awais-vteams

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Actually, array_merge calling in loop previously having O(n) complexity but it now its O(1) 6x faster

@taylorotwell taylorotwell merged commit a5b8f74 into laravel:5.8 May 30, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Do we need two empty arrays? array_merge([], ...$results) works for me.

@tantana5

This comment has been minimized.

Copy link

commented Jun 14, 2019

awesome, exactly i need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.