search

magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.47k stars 9.28k forks source link

possible memory leak in getUsedProducts function for configurables #37639

Open ioweb-gr opened 1 year ago

ioweb-gr commented 1 year ago

Preconditions and environment

Steps to reproduce

Create a lot of configurable products with at least 80 variations by color and size

Load the products via productRepository using searchCriteria

Then in a foreach loop

Try to use the function

$this->configurableType->getUsedProducts($product)

You will end up seeing a blackfire profile like this where each execution of the function runs more slowly over time.

This is an example of 50 products in my case,

The first execution of the loop took 300ms, image

the final one took 5+sec image

Example code

use Magento\Catalog\Api\ProductRepositoryInterface;
use Magento\ConfigurableProduct\Model\Product\Type\Configurable;
use Magento\Framework\Api\SearchCriteriaBuilder;

class ConfigurableProductProcessor
{
    protected $productRepository;
    protected $configurableType;
    protected $searchCriteriaBuilder;

    public function __construct(
        ProductRepositoryInterface $productRepository,
        Configurable $configurableType,
        SearchCriteriaBuilder $searchCriteriaBuilder
    ) {
        $this->productRepository = $productRepository;
        $this->configurableType = $configurableType;
        $this->searchCriteriaBuilder = $searchCriteriaBuilder;
    }

    public function processConfigurableProducts()
    {
        // Build the search criteria
        $searchCriteria = $this->searchCriteriaBuilder
            ->addFilter('type_id', 'configurable') // Filter by configurable products only
            // Add more filters, sorting, etc. if needed
            ->create();

        // Load configurable products based on the search criteria
        $configurableProducts = $this->productRepository->getList($searchCriteria)->getItems();

        foreach ($configurableProducts as $product) {
            $usedProducts = $this->configurableType->getUsedProducts($product);
            foreach ($usedProducts as $usedProduct) {
                // Process each used product
                // ...
            }
        }
    }
}

Expected result

Memory and time spent to fetch the products doesn't increase per iteration.

Actual result

Time execution increases steadily and so does memory consumption

Additional information

I'm developing an XML export plugin for Magento that aims to export a vast catalog of products. The catalog comprises approximately 1 million products, with 28,000 of them being configurable products with multiple variations. However, I am encountering challenges due to the sheer volume of products, leading to memory exhaustion and a significant increase in processing time due to the magento core functions.

Unfortunately as it is it's impossible to process the catalog properly.

My example was a limited example of 50 products. Imagine what happens with 28.000

I've provided the blackfire report to showcase that only magento core functions are taking up the majority of the slowness.

image

Release note

No response

Triage and priority

m2-assistant[bot] commented 1 year ago

Hi @ioweb-gr. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

ioweb-gr commented 1 year ago

@engcom-Bravo To give you more context, this is affecting all product export plugins I've seen so far. The consecutive loading of product data to export even with paginating over Collections or searchResults leads to this when configurable products are involved.

I've run a php-memprof profile on this where it stopped while out of memory at 10G !!!

I'd like to bring your attention to this kcachegrind view

Relative:

image

Absolute:

image

*sorted by self

Out of a total memorysize cost 10460310036

6220963464 were spent in PDOStatement::fetchAll and 2280736792 in Magento\Framework\Model\AbstractModel::setData

image

I've tried to dig into each function caller and it seems I keep going back to magento core functions that take up all the memory without ever freeing it.

For setData for example

By double clicking on kcachegrind I reach this point If I understand correctly using relative mode 14.37 out of the 21.80 total is spent on tier prices calculation

image

And 7.17 out of 21.80 is spent on _loadAttributes

image

For fetchAll

image

We have addmediaGalleryData and _loadAttributes

And by tracing the code execution graph everything points out to the getUsedProducts function and the aroundPlugin for it.

The weird thing in my case is there is a point where after 14-15k configurable products are processed there is sudden increase in memory usage which I can't quite pinpoint.

For example in my case at around 13k products I have used only 1272.5MB memory which rarely increases

image

And then at around 14.5k products I suddenly experience constant increasing of memory

image

But if I process that batch of products only e.g. from 14000 to 15000 the memory is not increasing like this. So it's not product specific.

Moreover there was one case which I forgot to screenshot but it show 80% of memory cost assigned to json_decode from the function

\Magento\ConfigurableProduct\Model\Plugin\Frontend\UsedProductsCache::aroundGetUsedProducts

I'd appreciate any help in deducing where the memory leak is

ioweb-gr commented 1 year ago

Here's an example profile where json_decode seems to be utilizing most of the memory usage constantly and the list of callers in sequence. That's in a smaller subset sample of course but the idea is the same

image

- image

- image

- image

Disabling the caching plugin saved 100MBs of memory only though in a subset of 2000 products out of 25000

MarksAfanasevics commented 11 months ago

Do you have some progress or a workaround for this? We have a lot of configurable, and looks like because of this bug site is really slow.

ioweb-gr commented 11 months ago

Nope unfortunately, there are memory leaks on all magento loops through Collections or ListInterfaces and this one is just the most extreme of them. I think that the Zend::PDO abstraction layer is just not good enough causing the memory leaks when fetching the data. I've never faced something similar with Doctrine for example.

MarksAfanasevics commented 11 months ago

For me it affects category page if swatches are enabled there. I see that problem is in salable check in plugin afterGetUsedProducts, page with 32 products and 170 swatches on them takes 54 sec to load. This https://github.com/magento/magento2/issues/26850#issuecomment-1311122767 helps a lot but issue is still there (30 sec to load). There is no performance issue at all if salable check is skipped (6.7sec to load): public function afterGetUsedProducts(Configurable $subject, array $products): array { // here was salable check return $products; } So problem for me is definitely in this plugin.

MarksAfanasevics commented 11 months ago

Looks like problem is in $this->productsSalableStatuses variable, I added logger to see how many values are stored there, and with each product object is growing, on first there are 370 values, on 40 there are 3603 values, possibly this is the memory leak we are looking for. Currently, I'm testing this as a replacement:

public function afterGetUsedProducts(Configurable $subject, array $products): array
    {
        $skusToCheck = [];
        $salableResults = [];

        $website = $this->storeManager->getWebsite();
        $stock = $this->stockResolver->execute(SalesChannelInterface::TYPE_WEBSITE, $website->getCode());

        foreach ($products as $product) {
            $sku = $product->getSku();
            $skusToCheck[] = $sku;
        }

        if (!empty($skusToCheck)) {
            $salableResults = $this->areProductsSalable->execute($skusToCheck, $stock->getStockId());
        }

        foreach ($products as $key => $product) {
            $sku = $product->getSku();
            $isSalableResult = $salableResults[$sku] ?? null;

            if ($isSalableResult && !$isSalableResult->isSalable()) {
                $product->setIsSalable(false);

                if (!$this->stockConfiguration->isShowOutOfStock()) {
                    unset($products[$key]);
                }
            }
        }

        return $products;
    }

Here is patch:

--- Plugin/Model/Product/Type/Configurable/IsSalableOptionPlugin.php
+++ Plugin/Model/Product/Type/Configurable/IsSalableOptionPlugin.php
@@ -73,29 +73,34 @@
      */
     public function afterGetUsedProducts(Configurable $subject, array $products): array
     {
-        $skus = [];
-        foreach ($products as $product) {
-            foreach ($this->productsSalableStatuses as $isProductSalableResult) {
-                if ($isProductSalableResult->getSku() === $product->getSku()) {
-                    continue 2;
-                }
-            }
-            $skus[] = $product->getSku();
-        }
-
-        if (empty($skus)) {
-            $this->filterProducts($products, $this->productsSalableStatuses);
-            return $products;
-        }
+        $skusToCheck = [];
+        $salableResults = [];

         $website = $this->storeManager->getWebsite();
         $stock = $this->stockResolver->execute(SalesChannelInterface::TYPE_WEBSITE, $website->getCode());

-        $this->productsSalableStatuses = array_merge(
-            $this->productsSalableStatuses,
-            $this->areProductsSalable->execute($skus, $stock->getStockId())
-        );
-        $this->filterProducts($products, $this->productsSalableStatuses);
+        foreach ($products as $product) {
+            $sku = $product->getSku();
+            $skusToCheck[] = $sku;
+        }
+
+        if (!empty($skusToCheck)) {
+            $salableResults = $this->areProductsSalable->execute($skusToCheck, $stock->getStockId());
+        }
+
+        foreach ($products as $key => $product) {
+            $sku = $product->getSku();
+            $isSalableResult = $salableResults[$sku] ?? null;
+
+            if ($isSalableResult && !$isSalableResult->isSalable()) {
+                $product->setIsSalable(false);
+
+                if (!$this->stockConfiguration->isShowOutOfStock()) {
+                    unset($products[$key]);
+                }
+            }
+        }
+
         return $products;
     }

I think this is a serious issue for stores with configurables that have a lot of variants, and I'm surprised that there is no answer from Magento team yet. It would be really great if someone could check solution I found.

m2-assistant[bot] commented 10 months ago

Hi @engcom-November. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-November commented 10 months ago

Hello @ioweb-gr,

Thank you for the report and collaboration! We tried to reproduce this on 2.4-develop, but we didn't see any major increase in time between the iterations. For profiling we have used SPX tool, please take a look at the screenshot: First iteration took 82ms image

Last iteration took 75ms image

We have used 50 configurable products with 80 variations, still we got the expected result. Please find the attached module used to reproduce this issue and let us know if we are missing anything. ProductVendor.zip

Thank you.

ioweb-gr commented 10 months ago

@engcom-November with 50 products you won't notice the issue. PHP buffer won't fill up with such a small dataset. Try with 50.000 configurables with 200 variations each (color / size)

The problem at some point is that because the memory is not properly released somewhere, PHP tries to find available object slot to clear and put the new one taking a tremendous increasing time to do so. With each pass, this slows down futher and further and further.

Your dataset is just too small.

My use case was with a site with 2.000.000 products of which around 50000 are configurables.

ioweb-gr commented 10 months ago

Check this video for example and notice the progressive increase of memory at each loop of configurable product batches

output.webm

m2-assistant[bot] commented 8 months ago

Hi @engcom-Hotel. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-Hotel commented 7 months ago

Hello @ioweb-gr,

Thanks for the contribution and collaboration!

We have tried to reproduce the issue and it seems the issue is reproducible in the Magento 2.4-develop branch. We have used memprof for issue reproduction.

Please find below the screenshot of qcachegrind UI for the reference:

image

Hence confirming the issue.

Thanks

github-jira-sync-bot commented 7 months ago

:white_check_mark: Jira issue https://jira.corp.adobe.com/browse/AC-10942 is successfully created for this GitHub issue.

m2-assistant[bot] commented 7 months ago

:white_check_mark: Confirmed by @engcom-Hotel. Thank you for verifying the issue.
Issue Available: @engcom-Hotel, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

kestraly commented 3 months ago

Looks like problem is in $this->productsSalableStatuses variable, I added logger to see how many values are stored there, and with each product object is growing, on first there are 370 values, on 40 there are 3603 values, possibly this is the memory leak we are looking for. Currently, I'm testing this as a replacement:

public function afterGetUsedProducts(Configurable $subject, array $products): array
    {
        $skusToCheck = [];
        $salableResults = [];

        $website = $this->storeManager->getWebsite();
        $stock = $this->stockResolver->execute(SalesChannelInterface::TYPE_WEBSITE, $website->getCode());

        foreach ($products as $product) {
            $sku = $product->getSku();
            $skusToCheck[] = $sku;
        }

        if (!empty($skusToCheck)) {
            $salableResults = $this->areProductsSalable->execute($skusToCheck, $stock->getStockId());
        }

        foreach ($products as $key => $product) {
            $sku = $product->getSku();
            $isSalableResult = $salableResults[$sku] ?? null;

            if ($isSalableResult && !$isSalableResult->isSalable()) {
                $product->setIsSalable(false);

                if (!$this->stockConfiguration->isShowOutOfStock()) {
                    unset($products[$key]);
                }
            }
        }

        return $products;
    }

Here is patch:

--- Plugin/Model/Product/Type/Configurable/IsSalableOptionPlugin.php
+++ Plugin/Model/Product/Type/Configurable/IsSalableOptionPlugin.php
@@ -73,29 +73,34 @@
      */
     public function afterGetUsedProducts(Configurable $subject, array $products): array
     {
-        $skus = [];
-        foreach ($products as $product) {
-            foreach ($this->productsSalableStatuses as $isProductSalableResult) {
-                if ($isProductSalableResult->getSku() === $product->getSku()) {
-                    continue 2;
-                }
-            }
-            $skus[] = $product->getSku();
-        }
-
-        if (empty($skus)) {
-            $this->filterProducts($products, $this->productsSalableStatuses);
-            return $products;
-        }
+        $skusToCheck = [];
+        $salableResults = [];

         $website = $this->storeManager->getWebsite();
         $stock = $this->stockResolver->execute(SalesChannelInterface::TYPE_WEBSITE, $website->getCode());

-        $this->productsSalableStatuses = array_merge(
-            $this->productsSalableStatuses,
-            $this->areProductsSalable->execute($skus, $stock->getStockId())
-        );
-        $this->filterProducts($products, $this->productsSalableStatuses);
+        foreach ($products as $product) {
+            $sku = $product->getSku();
+            $skusToCheck[] = $sku;
+        }
+
+        if (!empty($skusToCheck)) {
+            $salableResults = $this->areProductsSalable->execute($skusToCheck, $stock->getStockId());
+        }
+
+        foreach ($products as $key => $product) {
+            $sku = $product->getSku();
+            $isSalableResult = $salableResults[$sku] ?? null;
+
+            if ($isSalableResult && !$isSalableResult->isSalable()) {
+                $product->setIsSalable(false);
+
+                if (!$this->stockConfiguration->isShowOutOfStock()) {
+                    unset($products[$key]);
+                }
+            }
+        }
+
         return $products;
     }

I think this is a serious issue for stores with configurables that have a lot of variants, and I'm surprised that there is no answer from Magento team yet. It would be really great if someone could check solution I found.

@MarksAfanasevics - Did you make much progress on this. I think it works well apart from back orders. But I'll dig further