0
So I was astonished to see that a simple page with a handful of products produces over 1500 database queries! This seems over the top!.
I've been using xdebug to find the slowest sections of your code, and there are quite a few places caches could be used.
Take for example the Cart class. It has a method getOrderTotal, which is called 14 times (when I display a page). Inside that method is this code:
if ($this->isVirtualCart() AND $type == 5)
return 0;
if ($this->isVirtualCart() AND $type == 3)
$type = 4;
Now, isVirtualCart() may be called twice each time getOrderTotal is called. isVirtualCart looks like this:
public function isVirtualCart()
{
if (!intval(self::getNbProducts($this->id)))
return false;
$allVirtual = true;
foreach ($this->getProducts() AS $product)
{
$allVirtual &= (Validate::isUnsignedInt(ProductDownload::getIdFromIdProduct($product['id_product'])) ? true : false);
}
return $allVirtual;
}
There is one call to getNbProducts which queries the database for the count of items in the cart.
Then if that is greater than zero, a call to getProducts() is made, which in turn issues a couple of heavy queries. Then it loops over each product checking if it is a virtual product which in itself issues another query for each product!
Now I can see many problems with all this. The first problem is why call isVirtualCart twice? Just store the boolean results at the very beginning and you have halfed the number of queries. Second in isVirtualCart, why do you loop over all the products? Why not stop after the first non-virtual one is found?
I think the "better" solution to all this, is for the cart object to cache the products in it. As long as the database relating to this cart is only changed within the Cart object, then caching shouldn't cause any "lost update" issues as long as any operation modifying the cart clears the cache. Also if the products in the cart are cached, you can also cache values such as isVirtual. Also bare in mind the cache would only exist while the page is rendering (less than a second at most). So even if the cart is updated by some external means, the update will be reflected on the next page.
The main offender I found however was Tax::getApplicableTax(), which created an Address object 250 times with the exact same arguments each time. I made a simple pool of Address objects, where when a new Address object is required, it first checks inside the pool to see if it has already been created. If it is in the pool it is return, otherwise the database is queried to create a new Address object. This 4-5 line hack changed the script's execution time from 327ms to 263ms.
Anyway this long ranty post was for two reasons. One, I am frustrated with how inefficient some of the PrestaShop code is. Two, I wanted to highlight some of the flaws, so the main devs, or anyone else could go out and fix them. Now I'm not just moaning, I am willing to help make changes. For example, I want to change the cart so everything is cached. However I don't want to make many changes if there was a good reason to not cache everything, or if the main devs just thought this was out right stupid.
I would love to hear feedback from the devs, or anyone else.
thanks
Andrew
Back to top
