Skip to content

Remove array_merge#45

Merged
mnapoli merged 1 commit intomyclabs:masterfrom
theofidry:bugfix/array_merge
Sep 7, 2016
Merged

Remove array_merge#45
mnapoli merged 1 commit intomyclabs:masterfrom
theofidry:bugfix/array_merge

Conversation

@theofidry
Copy link
Collaborator

On the same token as #44. array_merge can be quite nasty as it creates a brand new array and copy the values of the two arrays being merged into it.

@mnapoli
Copy link
Member

mnapoli commented Sep 7, 2016

It's the first time I hear about array_merge not being "good", do you have a link that explains it?

@theofidry
Copy link
Collaborator Author

Hm I remember people like dunglas or ocramius mentioning it, verbally or on Twitter and I can't find a proper source on it.

However in good faith, here's a profiling: https://blackfire.io/profiles/compare/5e336e69-c114-4c83-9fe5-e360e777d39d/graph. Note that the benchmark I've made is not tweaked to highlight the issue (I didn't bother creating two or three parent classes with lots of properties).

@mnapoli
Copy link
Member

mnapoli commented Sep 7, 2016

👍 thanks for the profile link

@mnapoli mnapoli merged commit 6a21b7a into myclabs:master Sep 7, 2016
@Slamdunk
Copy link
Contributor

@theofidry this PR had to be reverted, see #50

@theofidry
Copy link
Collaborator Author

@Slamdunk see my comment on it, I don't think it needs to be reverted the fix is easy. Sorry to have broke things though, it's good to have a test for it now

@theofidry theofidry deleted the bugfix/array_merge branch September 16, 2016 13:51
@mnapoli
Copy link
Member

mnapoli commented Sep 16, 2016

👍 it's okay shit happens, I'm sorry too for not spotting that when reviewing the PR. I had a look at PHPUnit issues and there was no issue open about it so it doesn't seem like things broke for too many people.

Indeed the fix could have been different but as a first step it's a safe choice. I'm fine with another PR that restores the perf improvement with a few extra tests.

@theofidry
Copy link
Collaborator Author

@mnapoli yeah I'm gonna redo a PR making sure we have a test case for it as well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants