Reducing database queries. Developers please read

[bramp]

Hi,
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

[ruilong]

This is just the kind of improvements that are needed to get PrestaShop faster. I hope the devteam will look at this and implement these changes to the next version.

[Bruno Leveque]

Hi bramp,

Thanks for you help, we already deleted hundred of queries for v1.1.

Thanks to MySQL query cache, PrestaShop isn't "slow", however reducing queries greatly improved script execution.

Just wait n' see next release

[ruilong]

Do you have a date for next release?

[bramp]

It would be great if there was unsupported read-only SVN access, so we could follow development. There are half a dozen bugs on the bug tracker which have been reported as fixed, but I have to wait weeks/months until 1.1 is out to see if they are truly fixed for me.

Also if we are up to date, it would be easier for people to help contribute changes.

[Bruno Leveque]

Hi,

Yes we'll open SVN in read-only mode and write mode for most important community members

[intellierve]

I not sure if this a good solution but for that tax function above I did this.

static public function getApplicableTax($id_tax, $productTax)
{
global $cart, $defaultCountry;


global $cache_getApplicableTax;
if(isset($cache_getApplicableTax[$id_tax][$productTax])) {
return $cache_getApplicableTax[$id_tax][$productTax];
}


......

and at the bottom of the function

return $cache_getApplicableTax[$id_tax][$productTax] = $productTax * Tax::zoneHasTax(intval($id_tax), intval($defaultCountry->id_zone));
// return $productTax * Tax::zoneHasTax(intval($id_tax), intval($defaultCountry->id_zone));

[bramp]

I think lots of caches just like that will help. But what might be better is if the code is restructured so caches are needed less... If a cache is finally needed there perhaps should be a generic solution which can easily be applied to numerous classes/methods.

Also intelierve what you suggested is similar to what I did. However one interesting thing I have noted in the past, is using a 2D array like this:

$cache_getApplicableTax[$id_tax][$productTax]

is sometimes slower than doing this:

$cache_getApplicableTax[$id_tax . '_' . $productTax]

It might be worth seeing if this technique is faster. It certainly uses less ram.

Andrew

[niek]

Also when you turn on display errors with commenting config.inc.php there are a lot of errors, maybe these errors also slows down execution?!

//@ini_set('display_errors', 'off');

gr,
Niek