Optimization patch: product configurable attribute collection _loadPrices function #90

Closed
do-m-en opened this Issue Sep 28, 2012 · 9 comments

Projects

None yet

3 participants

@do-m-en
do-m-en commented Sep 28, 2012

Function app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php(132) Mage_Catalog_Model_Resource_Product_Type_Configurable_Attribute_Collection->_loadPrices()

has poor performance (I tested it in cart - magento 1.6.1.0 - but it hasn't changed consideraby from that version to current Magento 2 version) so I optimised it a bit.

I am not certain whether I made somekind of a mistake since the performance gain is fairly big so I'd appriciate a review of the patch before I apply it in production.

Major differences in calls to child functions (for 2 configurable and 1 simple product in cart) - output from xhprof:
Mage_Catalog_Model_Resource_Product_Type_Configurable_Attribute_Collection::getProduct: 11,036 before and 4 calls after

Mage_Catalog_Model_Product_Type_Configurable::getUsedProducts: 5,518 before and 2 calls after

Mage_Catalog_Model_Product::getTypeInstance: 5,518 before and 2 calls after

--- patch for magento 1.6.1.0 to 1.7.0.1 ---
see the comment below

--- patch for magento 2 (this patch wasn't tested) ---
see the comment below

Regards,
Domen Vrankar

@do-m-en
do-m-en commented Oct 1, 2012

I was really sloppy with the last patch so I deleted the comment... Even after reading my code through four times I haven't noticed the stupid mistakes that I've made...
This time I diffed the code with magento 2 and hopefully haven't made any more mistakes.
The performance gain for me was between 0.1 and 0.2 seconds (depending on which configurable product I viewed on magento 1.6.1.0) - the tests were made on product pages.
Once more I apologise for attaching a diff that I didn't check thoroughly.

Regards,
Domen

Patch for magento 2:

diff --git a/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php b/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php
index 29d59fa..6689555 100755
--- a/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php
+++ b/app/code/core/Mage/Catalog/Model/Resource/Product/Type/Configurable/Attribute/Collection.php
@@ -242,37 +242,39 @@ class Mage_Catalog_Model_Resource_Product_Type_Configurable_Attribute_Collection

             $values = array();

-            foreach ($this->_items as $item) {
-               $productAttribute = $item->getProductAttribute();
-               if (!($productAttribute instanceof Mage_Eav_Model_Entity_Attribute_Abstract)) {
-                   continue;
-               }
-               $options = $productAttribute->getFrontend()->getSelectOptions();
-               foreach ($options as $option) {
-                   $usedProducts = $this->getProduct()
-                       ->getTypeInstance()
-                       ->getUsedProducts($this->getProduct());
-                   foreach ($usedProducts as $associatedProduct) {
-                        if (!empty($option['value'])
-                            && $option['value'] == $associatedProduct->getData(
-                                                        $productAttribute->getAttributeCode())) {
-                            // If option available in associated product
-                            if (!isset($values[$item->getId() . ':' . $option['value']])) {
-                                // If option not added, we will add it.
-                                $values[$item->getId() . ':' . $option['value']] = array(
-                                    'product_super_attribute_id' => $item->getId(),
-                                    'value_index'                => $option['value'],
-                                    'label'                      => $option['label'],
-                                    'default_label'              => $option['label'],
-                                    'store_label'                => $option['label'],
-                                    'is_percent'                 => 0,
-                                    'pricing_value'              => null,
-                                    'use_default_value'          => true
-                                );
+            if($usedProducts = $this->getProduct()->getTypeInstance()->getUsedProducts($this->getProduct())) {
+                foreach ($this->_items as $item) {
+                    $productAttribute = $item->getProductAttribute();
+                    if (!($productAttribute instanceof Mage_Eav_Model_Entity_Attribute_Abstract)) {
+                        continue;
+                    }
+                    $itemId = $item->getId();
+                    $attributeCode = $productAttribute->getAttributeCode();
+                    $options = $productAttribute->getFrontend()->getSelectOptions();
+                    foreach ($usedProducts as $associatedProduct) {
+                        $attributeCodeValue = $associatedProduct->getData($attributeCode);
+                        foreach ($options as $key => $option) {
+                            if(!empty($option['value'])) {
+                                if ($option['value'] == $attributeCodeValue) {
+                                    // If option not added, we will add it.
+                                    $values[$itemId . ':' . $option['value']] = array(
+                                        'product_super_attribute_id' => $itemId,
+                                        'value_index'                => $option['value'],
+                                        'label'                      => $option['label'],
+                                        'default_label'              => $option['label'],
+                                        'store_label'                => $option['label'],
+                                        'is_percent'                 => 0,
+                                        'pricing_value'              => null,
+                                        'use_default_value'          => true
+                                    );
+                                    break;
+                                }
+                            } else {
+                                unset($options[ $key ]);
                             }
                         }
-                   }
-               }
+                    }
+                }
             }

             foreach ($pricings[0] as $pricing) {

Patch can be used on magento 1.6.1.0/1.7 with minor change to the first if statement:

            if($usedProducts = $this->getProduct()->getTypeInstance(true)->getUsedProducts(null, $this->getProduct())) {
@magento-team
Contributor

@do-m-en
Thanks for the patch. We have applied it partially -- moved out the getUsedProducts() out of the cycle. As for other suggested changes -- they are not applied:

  • swapping cycles of "used products" and "options" may lead to different order of the returned result, which would be unexpected change in behavior of this method
  • also breaking cycle and unsetting "options" adds more complexity to the code and indicates that "something is wrong" with the cycles.

Can you confirm that manipulation with the "options" cycle was the essentially the improvement? Because with the partial patch we applied, execution time doesn't seem improved.

@do-m-en
do-m-en commented Oct 23, 2012

I moved out the getUsedProducts() function call only to bypass a few calls - it doesn't change anything considerably regarding performance (Edit: actualy this changed performance a bit in my case - I forgot a bit about that... - not as mutch as the for loop change but this already improved performance for a little bit so I'm not certain why there is no gain in your case).

I put the unsetting part in the code because I didn't know whether that would happen often or not but couldn't see any difference in cycles on my test data so it's probbably useless.

The main gain came from changing the order of for loops. I haven't noticed a nigative impact on anything during the testing but it is possible that I was only lucky to test on products that didn't get wrong options order. I was tracing the changes and their performance hit all the time so I'm certain that this was the main performance gain.

I tested everything on a nexcess dedicated server with a test copy of a production store so the server was realy tuned for Magento. Also I was using APC cache (not as Magento cache but only as php cache) plus I had default Magento caching enabled and used redis for storing everything (I didn't use any full page caching modules). The execution of PHP code took the longest on that machine because of it's amount (database communication and reading from filesystem didn't have a big impact since everything was in memmory) and _loadPrices function was the only function with only 2 calls to it and a large execution time (I can't remember if it was 40 or 60% of the entire execution time) and the largest gain was changing the for loop order. I tested everything on Magento 1.6.1.0 so it is possible that something has changed in the newer versions. Also there are extensions present but I doubt that any of them change anything this deep in the code.

Since my laptop isn't tuned for performance (database and file system reads far exceed php execution time so almost anything I do to Magento has a performance impact that can't be seen on a tuned machine...) I can't test a newer vanilla Magento version with the same database content.

Server:
PHP 5.3.17
MySql 5.1.6.1
apache2 with PHP-FPM

Testing tools:
xhprof
http://www.magespeedtest.com/ (I wasn't able to run siege from my computer since the server is too far away for siege to be reliant so the test from this page are hopefully reliant)
http://www.webpagetest.org (was looking only at first bite without DNS lookup for this tests with ten runs and two consecutive runs set)

Content of the database:
~2500 configurable products
~20000 of all the products (simple products are all related to configurable products)
1 (most cases) or 2 configurable product options that define a simple product

It is possible that the amount of options compared to the amount of products is the reason for the performance gain so it'd probbably be a good idea to change the code so that the for loop order depends on the data in the database (statistics written somewhere in configuration for e. g.).

Wouldn't it be possible to implement somekind of sorting function that would sort the result after the data was already gathered?

Also if the change wouldn't impact core magento functionality it'd probbably be good enough to put a configuration flag in admin to enable/dissable the new version of the function so that the stores that are using modules that rely on the order would still be able to switch to the old implementation.

Regards,
Domen

@magento-team
Contributor

Hello,

Would you be able to provide us with a test database that you have used (with wiped customer/sales data if they are there)? It can be emailed to peter (at) magento.com

@do-m-en
do-m-en commented Oct 29, 2012

Hello,

Unfortunately I wasn't able to obtain the database.

@magento-team
Contributor

Thank you. We will evaluate this patch again and post an update.

@magento-team
Contributor

Hello @do-m-en ,

Thank you for your hard work. We tested the patch once again. See information bellow.

Environment:

  • Linux x86_b4
  • Apache 2.2
  • PHP 5.3.16 as a module
  • APC enabled
  • MySQL 5.5.27

Given:

  • Magento CE 1.7.0.1 with clear DB
  • 2500 configurable products with 2 simple products as options per each configurable product

Object of inspection:

  • processing time of a configurable product view page

Measurement tool:

  • Apache ab with 1000 loops and 1 user

Results received:

  • 0.00014s (0.04%) of improvement. Unfortunately such result can't be considered as significant and it is less than possible inaccuracy on our environment.

Most likely 0.2s of improvement can't depend on the environment, so it should be reproduced on our environment too. As you also mentioned, you made some additional code changes when performed the measurement. It would be nice, if you remeasure the performance impact with the same conditions as we had:

  • no additional code changes
  • Magento CE 1.7.0.1 with clear DB
  • 2500 configurable products with 2 simple products per each (see script for generation products)

If your result would be similar to ours, 0.2s performance improvement received by you before can be explained as reproducible on a specific DB only (more simple products or some other data has an impact on configurable product view page). Or maybe other code changes gave you some performance improvement during the measurement.

In case the result is considerably different from ours, it would be nice if you provide DB dump, version of Magento and the exact patch you used for measurements. The strategy of measurement would be helpful as well.

Thank you again for your contribution.

@magento-team magento-team added a commit that referenced this issue Nov 12, 2012
@magento-team magento-team 2.0.0.0-dev30
* Framework changes
  * Added dependency injection of framework capability
    * Adopted Zend\Di component of Zend Framework 2 library
    * Implemented object manager in Magento application
    * Refactored multiple base classes to dependency injection principle (dependencies are declared in constructor)
  * Themes/View
    * Implemented storing themes registry in database, basic CRUD of themes, automatic registration of themes in database from file system out of the box
    * Renamed `Mage_Core_Model_Layout_Update` into `Mage_Core_Model_Layout_Merge`, the former becomes an entity domain model. Similar changes with `Mage_Core_Model_Resource_Layout` -> `Mage_Core_Model_Resource_Layout_Update`, `Mage_Core_Model_Layout_Data` -> `Mage_Core_Model_Layout_Update`
* Performance tests
  * Improved indexers running script `dev/shell/indexer.php` to return appropriate exit code upon success/failure
  * Implemented running the same performance scenario file with different parameters
  * Slightly refactored framework class `Magento_Performance_Testsuite_Optimizer` for better visibility of algorithm
* Visual design editor
  * Added ability to remove elements in editor UI
  * Revised history of changes VDE toolbar and algorithm of "compacting" operations (moving, removing elements) as a layout update XML
  * Added selection of themes to VDE launcher page
* Refactored JavaScript of some UI elements to jQuery:
  * "Simple" and "configurable" product view pages
  * "Create Account" page
  * "Shopping Cart" page
  * CAPTCHA
  * Newsletter subscription
* Tax management UX improvements
  * Split Basic and Advanced Settings for Tax Rule Management UI
  * Moved the Import/Export functionality to Tax Rate page
  * Moved Tax menu to System from Sales
* Implemented the editable multiselect JavaScript component
* Added mentioning sitemap in `robots.txt` after generation
* Removed creation of DB backup in integration testing framework
* Fixed logic of order of loading ACL resources in backend
* Fixed JavaScript error during installation when one of files in `pub/media` is not writable
* Fixed structure of legacy test fixtures that allowed ambiguous keys in declaration
* Fixed inability to restore admin password when CAPTCHA is enabled
* Various minor UX fixes (labels, buttons, redirects, etc...)
* GitHub requests:
  * [#59](#59) -- implemented handling of unexpected situations in admin/dashboard/tunnel action
  * [#66](#66)
    * refactored ImageMagick adapter unit test to avoid system operation
    * simplified unit testing framework -- removed unused classes, simplified handling logic of directory `dev/tests/unit/tmp` and removed it from VCS
  * [#73](#73), [#74](#74) -- fixes in docblock tags
  * [#75](#75), [#96](#96) -- fixed translation module contexts in a few places
  * [#80](#80) -- fixed some runtime errors in import/export module
  * [#81](#81) -- removed usage of "remove" directive in places where it is overridden by setting root template anyway
  * [#87](#87) -- changed paths of files to include from relative into absolute in `dev/shell/indexer.php` and `log.php`
  * [#88](#88) -- provided comments for values that can be configured in `app/etc/local.xml` file
  * [#90](#90) -- slightly optimized logic of implementation of loading configurable product attributes
0f2900a
@magento-team
Contributor

Hello. We have not received any more information on this one and our tests cannot confirm the performance gain. We are closing this issue. Please post a comment to it if you can provide more details or the test DB and we will reopen. Thank you.

@fr0x
fr0x commented Jun 3, 2013

"2500 configurable products with 2 simple products as options per each configurable product"

What were the number of super attributes used in this test?

If it was 2 super attributes to cover the 5,000 simple products, this might not have fully showed the impact.

In our testing, the slowdown from this is directly related to the number of super attributes returned by:

$options = $productAttribute->getFrontend()->getSelectOptions();

This may need to be re-evaluated with a large super attribute count (5,000....one unique super attribute per simple product).

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