-
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
Optimization on Weee tax calculation #460
Comments
I would agree with your suggested optimization. I don't think the order matter here. |
Ok so I'm on it. |
Thank you for the FTP optimization - added/prioritized to backlog. |
I'm tracking this as our internal Jira ticket MAGETWO-32441; @Dorn- do you have a PR for this optimization? Thanks, |
@Dorn- do you have some updates? |
@Dorn-, internal ticket is resolved and all related changes are already available in develop branch of public repository. Please, spend some time to evaluate our solution and provide your feedback. |
Fixed Issues: - MAGETWO-58670 Unable to go to PayPal side from first attempt after applying discount on checkout - MAGETWO-58997 Copy past detector fails in Vault module - MAGETWO-59033 [Github] Authorize.net doesn't allow to use JCB and Diners Club credit cards - MAGETWO-59187 Magento\Checkout\Test\TestCase\OnePageCheckoutTest fails on variation OnePageCheckoutUspsTestVariation2
… for Customer #460 - Merge Pull Request magento/graphql-ce#460 from magento/graphql-ce:423-shippingmethod-test-coverage - Merged commits: 1. 198e1b3 2. ceace79 3. 8743bbb 4. 112f425 5. d9a5f4a 6. 2cbfb1f 7. 4571e8e 8. 8db3167 9. 1821c9d 10. 672526f 11. 1bd2068 12. d280bb0 13. d39e99b 14. 40061f8 15. f88f975 16. f8de00a 17. 7654d45 18. 06babc0 19. 7c216c5 20. 7826c4f
Hello,
I believe i found a misconception in the Tax Model used in the module Weee.
This issue is already there in Magento EE 1.12 and 1.13.
The relevant function is Mage_Weee_Model_Tax::getProductWeeeAttributes, the issue being at line 141 $productAttributes = $product->getTypeInstance()->getSetAttributes($product);
I realized something went wrong due to an unexpected long response time while trying to display my shopping cart when it is starting to be big (> 15 items).
As a reference, my website offers 25k products and approx. 1000 product attributes.
Using Xdebug, i realized that the FTP calcullation and more precisly the aforementionned function was the source of the issue.
Now if we start digging a bit, something really seems off:
Line 113: $allWeee = $this->getWeeeTaxAttributeCodes();
We gather all the attributes necessary in order to do the tax calculation.
Line 141: $productAttributes = $product->getTypeInstance()->getSetAttributes($product);
We gather all the existing attributes and sort them.
When, as it is the case for us, we have 1K attirbutes, sorting them is probably really hungry CPU wise (on top of the DB load to load them prior to that), is that really necessary ?
3)Line 142/143: foreach ($productAttributes as $code => $attribute) { if (in_array($code, $allWeee)) {
We in the end only deal with the Weee attributes.
Therefore, to sum it up:
I get the Weee attributes from the DB => Returns 1 attribute
I load all the attributes and sort them => 1k results with skyrocketing CPU load while sorting
I loop on the 1k attributes for in the end only dealing with one.
with a shopping cart containing 20 items, we can multiply this by 20.
My question is the following, why do we need to load and sort all the attributes while we only really need a few ?
Moreover, is sorting them really necessary ?
We already have the necessary attributes code thanks to line 113.
It should be possible to replace line 141 with something like:
$attrCollection = Mage::getModel('eav/entity_attribute') ->getResourceCollection() ->setCodeFilter($allWeee); foreach ($attrCollection as $attr) { $productAttributes[$attr->getAttributeCode()] = $attr; }
While i did not sort the attributes in this specific case, i already saved up 70% of CPU load.
I am currently working on a fix to just load the attributes which are necessary and to keep the sorting, please let me know if you think it's worth it and i will try to provide it.
Best Regards,
The text was updated successfully, but these errors were encountered: