-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Stop $this->validColumnNames array from growing and growing #8928
Stop $this->validColumnNames array from growing and growing #8928
Conversation
…ue is recursively updated with its own data several times. Importing hundred of customers affect performance and even break due to memory limits
@jalogut thank you for your contribution. Could you please sign CLA? |
return array_unique( | ||
array_merge( | ||
$this->validColumnNames, | ||
$this->customerFields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such implementation is better than before but still has linear computational complexity.
If you flip the arrays fields will be unique by design and you will be able to perform main operations with O(1)
complexity.
So, use array_keys($array)
to get values and $array1 + $array2
to combine two arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I do not really understand. Where is the complexity here? in the array_unique
? If so, we could remove it as the main problem was simply that the resulting array was saved over and over again into $this->validColumnNames
. That way, just returning the array_merge will fix that.
Regarding your solution, it will be ok here but in the CustomerComposite.php
we have to flip 4 arrays. If I understand it correctly that would be:
return array_keys(
array_flip($this->validColumnNames) +
array_flip($this->_customerAttributes) +
array_flip($this->_addressAttributes) +
array_flip($this->_customerEntity->getValidColumnNames())
);
I find this a bit more complicated to understand than my current solution. Does it really make a difference in terms of performance? If so, I would not hesitate to edit my pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying to call array_flip
, I'm saying to flip arrays by themselves.
So, instead of ['column-name']
there will be ['column-name' => 1]
.
Where is the complexity here?
I'm saying about https://en.wikipedia.org/wiki/Time_complexity#Table_of_common_time_complexities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this wikipedia link is too complex for me. I understand what you mean about flipping the arrays themselves but that will make the code difficult to understand. It will also imply to make changes in several places where all this arrays are created/updated.
My goal was just to point out that saving recursively $this->validColumnNames
including itself, creates an array that is twice bigger in every iteration.
How to fix that? there are several valid solutions including the 2 we are discussing. I just went for the simplest and most understandable but of course other are also valid.
@jalogut Thanks for signing the CLA and very good catch! |
@jalogut Thank you, merged! |
…wing #8928 - fix static test
Optimise customer import getValidColumnNames() method