15

Right now im reusing a lot of collections that are nested within foreach loops. Is it possible to move these things up a few levels? Currently Im forced to reload collections that have 51k + entities over and over again which slows things down tremendously. Specifically the kitinventory collections.

<?php
class Codespace_Module_Helper_Item extends other_one{

function functionOne($collection){
    ...
    $data = $collection->getData();
    foreach($data as $item){
        $this->_functionTwo($item);
    }
    ...
}

function _functionTwo($item){
    $model = Mage::getModel('catalog/product');
    $id = $model->getIdBySku($item['sku']);
    $inventoryStatus = Mage::getResourceSingleton('catalog/product')->getAttributeRawValue($id, 'product_inventory_status', 1);
    $invStatus = $model->getResource()->getAttribute('product_inventory_status')->getSource()->getOptionText($inventoryStatus);
    if ($invStatus && $id) {
        if ($invStatus !== 'Z') {
            $stockItem = Mage::getModel('cataloginventory/stock_item');
            $stockItem->setData(array());
            $stockItem->loadByProduct($id);
            if ($stockItem->getQty() != $item['quantity']) {
                $stockItem->setQty(item['quantity']);
                $stockItem->save();
                $this->functionThree($item['sku']);
            }
        }
    }
}

function functionThree($sku){
    $collectionOfKits = Mage::getModel('kitinventory/kitinventory')->getCollection()->addFieldToFilter('related_sku',$sku);
    if($collectionOfKits->getSize()){
        foreach($collectionOfKits as $kit){
            $kitSku = $kit->getSku();
            $kitCollection = Mage::getModel('kitinventory/kitinventory')->getCollection()->addFieldToFilter('kit_sku',$kitSku)->setOrder('related_sku','ASC');
            ...
            foreach($kitCollection as $component){
                $componentSkus[] = $component->getRelatedSku();
                $componentRequiredQuantity[] = $component->getRequiredQuantity();
            }
            $componentProductCollection = Mage::getModel('catalog/product')->getCollection();
            $componentProductCollection->joinField('qty',
                'cataloginventory/stock_item',
                'qty',
                'product_id=entity_id',
                '{{table}}.stock_id=1',
                'left');
            $componentProductCollection->addAttributeToFilter('sku', array('in' => $componentSkus));
            foreach($componentProductCollection as $component){
                $quantity = $component->getQty();
                ...
            }
            $kitId= Mage::getModel('catalog/product')->getIdBySku($kitSku)
            $kitStockItem = Mage::getModel('cataloginventory/stock_item')->loadByProduct($kitId);
            $this->functionFour($kitStockItem,$kitSku,$amountOfKitsPossible);
        }
    }
}

function functionFour($kitStockItem,$kitSku,$amountOfKitsPossible){
    ...
    $kitStockItem->setQty($quantity);
    $kitStockItem->save();
    ...
}

EDIT: this is the current functionality i've come up with, I still think there is a better way to handle these collections.

  • What kind of collection is passed to functionOne($collection)? In what order would it's size/count of items be? Is it necessary to loop over it to get the SKU's? – 7ochem Mar 23 '15 at 15:54
  • @7ochem Its a custom collection built from new inventory data which we get from our inventory management system. it contains the name of the item, the quantity of the item on hand, and the sku of the item. It can potentially contain 60k+ elements. – easymoden00b Mar 23 '15 at 15:59
9
+150

There are a few things you can work on;

  • not passing by reference, so using extra memory, you can pass objects, but arrays cannot be passed by reference by default. Or add a & in the function parameter declaration like function hello(array &$world)
  • invalid checks, if something isn't there return immediately. Do not try to find something that's not there
  • readability can be hard sometimes
    • add some doc (so you can understand if you see it in a few days, monhts, years)
    • smarter if statements to get less indentation
  • functions should have only one purpose, update stock or update related, not both, so maybe even cut some functions in even smaller functions. Try to create such logic in you mind, and rework from there.
  • Take a look at ->cleanModelCache()->clearInstance() from Mage_Core_Model_Model_Abstract to clear underlying data for some objects, can speed up things.
  • of coarse all other things which have already been said.

Added a updated version of your code with some inline recommendations on your current code, I could go on a bit, but it would currently not add more to it.

Function 1: Purpose is walking the collection

    /**
     * Walk collection
     * 
     * @param Mage_Core_Model_Resource_Db_Collection_Abstract $collection
     * @return void
     */
    public function functionOne($collection)
    {
        // ...

        // Walk collection, create references instead of passing array data
        foreach ($collection as $item) {

            // Update stock for product
            if (!$this->_functionTwo($item)) {
                // Not updated, continue next
                continue;
            }

            // Update related products
            $this->_functionThree($item); // Use same object again, no extra memory is used
        }

        // ...
    }

Function 2: Purpose is updating stock if changed

    /**
     * Update stock item if changed, returns true if updated
     * 
     * @param Mage_Core_Model_Model_Abstract $item
     * @return bool
     */
    function _functionTwo($item)
    {
        $model = Mage::getModel('catalog/product');
        /** @var $model Mage_Catalog_Model_Product */

        $id = $model->getIdBySku($item->getData('sku'));

        if (!$id) {
            // no id found, so stop looking nothing up
            return false;
        }

        // Get option value for store 1
        $inventoryStatus = $model->getResource()
                ->getAttributeRawValue($id, 'product_inventory_status', 1);

        if (!$inventoryStatus) {
            // No need for another lookup in db, because the status isn't set
            return false;
        }

        $invStatus = $model->getResource()
                ->getAttribute('product_inventory_status')
                ->setStoreId(0) // Get admin value
                ->getSource()
                ->getOptionText($inventoryStatus);

        if (!$invStatus) {
            // No need for another lookup in db, because the status has no text
            return false;
        }

        if ($invStatus === 'Z') {
            // Inventory status to not change something
            return false;
        }

        $stockItem = Mage::getModel('cataloginventory/stock_item');
        /** @var $stockItem Mage_CatalogInventory_Model_Stock_Item */

        // $stockItem->setData(array()); // unneeded piece of code
        $stockItem->loadByProduct($id);

        if ($stockItem->getQty() == $item->getData('quantity')) {
            // Valid stock
            return false;
        }

        // Update stock
        $stockItem->setQty($item->getData('quantity'));
        $stockItem->save();

        // End function and call function three separately, does something else
        return true;
    }

Function 3: Purpose updating related stock items

    /**
     * Update related stock items, return false if no related items are found
     * 
     * @param Mage_Core_Model_Model_Abstract $item
     * @return bool
     */
    function functionThree($item)
    {

        $collectionOfKits = Mage::getModel('kitinventory/kitinventory')
                ->getCollection()
                ->addFieldToFilter('related_sku', $item->getData('sku')); // Check if your indexes are set on these columns

        if (!$collectionOfKits->getSize()) {
            // Nothing found to relate to
            return false;
        }

        $connection = Mage::getSingleton('core/resource')
                ->getConnection('core_write');

        // Walk kits
        foreach ($collectionOfKits as $kit) {

            // getData is slightly faster then getSku(unless you've implemented it in your model)
            // getSku -> __call('getSku') -> get -> lowercase('sku') -> getData('sku') | note, Magento has some internal caching in this 
            $kitSku = $kit->getData('sku');

            $kitCollection = Mage::getModel('kitinventory/kitinventory')
                    ->getCollection()
                    ->addFieldToFilter('kit_sku', $kitSku)
                    ->setOrder('related_sku', 'ASC');

            // Use just a fetchAll to create a fast db query
            $select = $kitCollection->getSelect();

            $select->reset(Zend_Db_Select::COLUMNS)
                    ->distinct()
                    ->columns('related_sku')
                    ->columns('required_quantity');

            // Fetch component sku
            $componentSkus = $connection->fetchAll($select, 0);

            // Fetch required quantity
            $componentRequiredQuantity = $connection->fetchCol($select, 1);

            // ...

            $componentProductCollection = Mage::getModel('catalog/product')
                    ->getCollection()
                    ->joinField('qty',
                    'cataloginventory/stock_item',
                    'qty',
                    'product_id = entity_id',
                    '{{table}}.stock_id = 1',
                    'left');
            $componentProductCollection->addAttributeToFilter('sku', array('in' => $componentSkus));

            // Next line will invoke a load on the product collection
            foreach ($componentProductCollection as $component) {
                $quantity = $component->getQty();

                // ...

            }
            // You could choose to do a fetchAll here instead to get just the data you need
            $connection = $componentProductCollection->getConnection();

            foreach ($connection->fetchAll($componentProductCollection->getSelect()) as $row) {
                // Will have a array here
                $quantity = $row['quantity'];

                // ... -- do not not which funky magic happens here
            }


            $kitId = Mage::getModel('catalog/product')
                    ->getIdBySku($kitSku);
            if (!$kitId) {
                // No id
                continue;
            }

            // You could also take a look if you can sum the stock and do a single update instead
            $kitStockItem = Mage::getModel('cataloginventory/stock_item')
                    ->loadByProduct($kitId);
            $this->functionFour($kitStockItem, $kitSku, $amountOfKitsPossible);

            // Or something like this, update single field
            $connection->update($kitStockItem->getResource()->getMainTable(), array('qty' => $quantity), 'item_id = ' . $kitStockItem->getId());
        }

        return true;
    }

Function 4: Had to make some lucky(or unlucky) guesses, for now it is a useless function, could be added as is in Function 3.

    /**
     * Save stock item if changed and something else, rather not say ;-)
     * 
     * @param Mage_Catalog_Inventory_Model_Stock_Item $kitStockItem
     * @param string $kitSku
     * @param int $amountOfKitsPossible Guessed it
     */
    function functionFour($kitStockItem, $kitSku, $amountOfKitsPossible)
    {

        // ...

        // Do not know the rest of the code, so I wouldn't know which I could optimize here
        // If it isn't to serious, you could look at a single query and not hitting extra functions

        // Check if changed
        if ($quantity !=$kitStockItem->getData('qty')) {
            $kitStockItem->setQty($quantity);
            $kitStockItem->save();
        }        

        // ...

    }
}
  • You are the man. I'm relatively certain this will work, and if this shows a distinct improvement in processing time, may be my go-to reference for manipulating collections! – easymoden00b Mar 25 '15 at 15:09
  • A few small errors, but it's much better constructed than my own. – easymoden00b Mar 26 '15 at 15:46
5

I wanted to add this as a comment but I don't yet have enough rep. Take a look at how the Magento core grids join the product qty to the catalog/product collection here: https://github.com/OpenMage/magento-mirror/blob/magento-1.9/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Grid.php#L65

If you join the table to get the qty, you don't have to call this in a loop: Mage::getModel('cataloginventory/stock_item')->loadByProduct($product)->getQty();

$productCollection = Mage::getModel('catalog/product')->getCollection();
$productCollection->joinField('qty',
    'cataloginventory/stock_item',
    'qty',
    'product_id=entity_id',
    '{{table}}.stock_id=1',
    'left');
$productCollection->addAttributeToFilter('sku',array('in' => $relatedSkus));
foreach($productCollection as $product){
    $quantity = $product->getQty();
    ...// now you have your qty without having to load the product model.
}

The other alternative is to see if you can cache the results of this system intensive process. Maybe you could make a second database table to store the results, and have it refresh like a magento index would.

  • care to share how I could then call this information in a similar manner to the code above? Also, if I generate the product collections and assign them to a class variable would I be able to call that collection throughout the code? Would filtering (as shown in the code) effect the class variable or would it remain unchanged if I assign this filtered collection to another variable? – easymoden00b Mar 20 '15 at 13:05
  • I added an example of how to join the field, but this is only a small optimization. Yes: You can save the results as class variables and call them elsewhere. But: I think you will have to instantiate a new model each time you want to change filtering (may defeat the purpose.) – Eric Seastrand Mar 20 '15 at 13:23
  • Thank you, it appears as I have feared. Thanks for this example of optimization. Any others you can think of? I'll expound upon the uses of each collection a bit in the code example above. – easymoden00b Mar 20 '15 at 14:03
3

You don't need to reload the model over and over, Mage::getModel() with a reference is enough without knowing how your resource models are setup its difficult to say if its being reinitialized every time in memory and in those loops you end up leaking/running out of memory causing disk swap to occur possibly.

One collection to rule them all. Refactoring the functions to only reference the one collection. This is the same with standard SQL and procedural programming. Take a bit more time investigating your collections and resource models on how you can get all the data you need from SQL once, maybe twice and then have enough memory in place as well as referencing the data for looping over for display/manipulation. It is also easier to store one result in cache vs many, this is the same case for MySQL's built-in caching mechanisms also, as frequent requests that are large enough will cause the same disk swap issue.

Save the I/O

Vinai has a good example of implementing the same approach:

References:

protected by Amit Bera Mar 27 '15 at 20:07

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.