PrestaShop Forums: Reducing database queries. Developers please read - PrestaShop Forums

Jump to content


Welcome to the PrestaShop Forum! We hope you'll share your comments and suggestions with us. We ask that you please post in English to the main sections of the PrestaShop Forum. If you want to write in another language, please post in the corresponding PrestaShop Community section below.

Please note that PrestaShop Community sections are largely self-moderated. PrestaShop team members may or may not participate in non-English sections. To improve the chances of receiving feedback to your question or comment, please post it in English to the main sections of our Forum.

NYC

Vous parlez français ? par ici !


Reducing database queries. Developers please read


Reducing database queries. Developers please read

#1 bramp

    PrestaShop Apprentice

  • 15 Aug 2008
  • Members
  • PipPip
  • 38 posts

Posted 24 August 2008 - 09:00 PM

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

#2

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 24 August 2008 - 10:52 PM

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.

#3

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 25 August 2008 - 04:13 AM

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 ;)

#4

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 25 August 2008 - 04:19 AM

Do you have a date for next release?

#5

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 25 August 2008 - 11:32 AM

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.

#6

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 25 August 2008 - 11:37 AM

Hi,

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

#7

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 26 August 2008 - 09:56 AM

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));

#8

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 26 August 2008 - 11:09 AM

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

#9

    PrestaShop Newbie

  • 15 Dec 2011
  • Members
  • Pip
  • 0 posts

Posted 13 July 2009 - 12:24 PM

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





1 user(s) are reading this topic

0 members, 1 guests, 0 anonymous users