osCommerce Community Support Forums > Performance related coding flaws

insaini

Apr 7 2008, 12:25 PM

Ok so I finally have my store set as I want it.. And I have now dived into the performance problems of the site. There arent many categories and I dont show the product counts of each category .. but when I started looking at the code I found possibly one of the biggest problems in the way osCommerce is coded. Im pretty sure there is an optimization trick to enhance this but as far as I am concerned from a coding standpoint it wouldnt have been needed had it been coded properly in the first place..

function tep_get_tax_rate()

this function is called everywhere.. which is absolutely pointless and is possibly one of the biggest bottlenecks in the site if you have a lot of products per category.. the reason why it is pointless is if you dont show the prices with taxes added on then there is no reason to call this function yet it is called for each product for each category. Even if you cache the results for the product.. the function is called for absolutely no reason.. this has lead to other coding flaws in contributions for example 'PriceFormatter.php' which also calls this function for no good reason.

So how do you fix it.. a little time is all you need. Instead of calling this function with the tax_class_id BEFORE sending it to the currency formatter methods (ie $currencies->format_price etc..) just pass the tax_class_id into it.. let currencies methods pass the tax_class_id further until it finally reaches its main useful method .. tep_add_tax ..

tep_add_tax does its only useful function.. IF you are adding the tax to the price.. CALCULATE IT THEN!!! .. let tep_add_tax call tep_get_tax_rate() with the tax_class_id which you have now passed through.. INSTEAD OF .. passing in an absolutely pointless products_tax value which will NEVER BE USED!! ..

examples..

in /catalog/includes/functions/general.php

change

CODE

// Add tax to a products price
  function tep_add_tax($price, $tax) {
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) ) {
// BOF Separate Pricing Per Customer, show_tax modification
// next line was original code
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) ) {
    global $sppc_customer_group_show_tax;
    global $sppc_customer_group_tax_exempt;
     if(!tep_session_is_registered('sppc_customer_group_show_tax')) {
     $customer_group_show_tax = '1';
     } else {
     $customer_group_show_tax = $sppc_customer_group_show_tax;
     }

    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) && ($customer_group_show_tax == '1')) {
// EOF Separate Pricing Per Customer, show_tax modification
      return $price + tep_calculate_tax($price, $tax);
    } else {
      return $price;
    }
  }

to this

CODE

// Add tax to a products price
  function tep_add_tax($price, $tax_class) {
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) ) {
// BOF Separate Pricing Per Customer, show_tax modification
// next line was original code
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) ) {
    global $sppc_customer_group_show_tax;
    global $sppc_customer_group_tax_exempt;
     if(!tep_session_is_registered('sppc_customer_group_show_tax')) {
     $customer_group_show_tax = '1';
     } else {
     $customer_group_show_tax = $sppc_customer_group_show_tax;
     }

    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) && ($customer_group_show_tax == '1')) {
// EOF Separate Pricing Per Customer, show_tax modification
      return $price + tep_calculate_tax($price, tep_get_tax_rate($tax_class));
    } else {
      return $price;
    }
  }

note I am using SPPC so your methods will be different if you are not.. regardless this is exactly how this function should have been coded to begin with.

next change whatever calls that function .. not many unless you are using PriceFormatter.php but for example the currencies class

change this

CODE

    function display_price($products_price, $products_tax, $quantity = 1) {
      return $this->format($this->calculate_price($products_price, $products_tax, $quantity));
    }

to this

CODE

    function display_price($products_price, $products_tax_class, $quantity = 1) {
      return $this->format($this->calculate_price($products_price, $products_tax_class, $quantity));
    }

and this

CODE

    function calculate_price($products_price, $products_tax, $quantity = 1) {
      global $currency;

      return tep_round(tep_add_tax($products_price, $products_tax), $this->currencies[$currency]['decimal_places']) * $quantity;
    }

to this

CODE

    function calculate_price($products_price, $products_tax_class, $quantity = 1) {
      global $currency;

      return tep_round(tep_add_tax($products_price, $products_tax_class), $this->currencies[$currency]['decimal_places']) * $quantity;
    }

now I mean come on this has to make perfect sense... it does to me anyhow.. ive already made the changes.. noticed a nice drop in the number of queries.. and no need for caching something that shouldnt need to be in the first place.. (not unless you are showing these prices with taxes included that is..)

J

burt

Apr 7 2008, 12:29 PM

Good work.

There are some works going on already;
http://forums.oscommerce.com/index.php?showtopic=110585
http://www.oscommerce.com/community/contributions,2417

insaini

Apr 7 2008, 12:39 PM

QUOTE (burt @ Apr 7 2008, 08:29 AM)

Good work.

There are some works going on already;
http://forums.oscommerce.com/index.php?showtopic=110585
http://www.oscommerce.com/community/contributions,2417

Right.. I know there is an optimization contribution for this tep_get_tax_rate method.. what im trying to say is there is no need for it..

if you do the coding changes required.. these are the files that need to be modified on a stock osC

general.php
currencies.php
specials.php
whats_new.php
shopping_cart.php
new_products.php
product_info.php

and if you have them..

pad_base.php
PriceFormatter.php

modifying these files as I stated above will reduce those function calls for good..

J

I noticed a drop from 6 second process time to 0.5 .. like i said.. dramatic

FWR Media

Apr 7 2008, 12:47 PM

I'm glad that you got it sorted for yourself but the best solution is Chemos tep_get_tax.

Two replaced functions in includes/functions/general.php.

Upload the class tax.php to includes/classes.php.

Done.

insaini

Apr 7 2008, 12:54 PM

QUOTE (FWR Media @ Apr 7 2008, 08:47 AM)

I'm glad that you got it sorted for yourself but the best solution is Chemos tep_get_tax.

Two replaced functions in includes/functions/general.php.

Upload the class tax.php to includes/classes.php.

Done.

I realize Chemo's solution works.. it does so differently.. by caching the tax class ids in an array .. whereas what I pointed out.. needs no use of caching and may take a couple more minutes to implement.. but regardless.. works the same.. with a lot queries but ALSO a lot less function calls and the same time showing that it just makes more sense to have coded it that way in the first place.. why cache when you dont need to..

desiredin

Apr 14 2008, 01:14 PM

What versions of OSC does this apply to? I am using OSC2.2 RC2 and I'm not finding those functions in
specials.php
whats_new.php
shopping_cart.php
new_products.php
product_info.php

insaini

Apr 14 2008, 05:40 PM

QUOTE (desiredin @ Apr 14 2008, 09:14 AM)

What versions of OSC does this apply to? I am using OSC2.2 RC2 and I'm not finding those functions in
specials.php
whats_new.php
shopping_cart.php
new_products.php
product_info.php

This applies to osC 2.2 all releases..

If you want to make the changes I made..

you need to follow these steps

First- Open /catalog/includes/functions/general.php

Find the function: (Now I have SPPC installed so this function will be slightly different if you dont)

CODE

// Add tax to a products price
  function tep_add_tax($price, $tax) {
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) ) {
// BOF Separate Pricing Per Customer, show_tax modification
// next line was original code
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) ) {
    global $sppc_customer_group_show_tax;
    global $sppc_customer_group_tax_exempt;
     if(!tep_session_is_registered('sppc_customer_group_show_tax')) {
     $customer_group_show_tax = '1';
     } else {
     $customer_group_show_tax = $sppc_customer_group_show_tax;
     }

    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax > 0) && ($customer_group_show_tax == '1')) {
// EOF Separate Pricing Per Customer, show_tax modification
      return $price + tep_calculate_tax($price, $tax);
    } else {
      return $price;
    }
  }

and change it to this

CODE

// Add tax to a products price
  function tep_add_tax($price, $tax_class) {
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax_class > 0) ) {
// BOF Separate Pricing Per Customer, show_tax modification
// next line was original code
//    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax_class > 0) ) {
    global $sppc_customer_group_show_tax;
    global $sppc_customer_group_tax_exempt;
     if(!tep_session_is_registered('sppc_customer_group_show_tax')) {
     $customer_group_show_tax = '1';
     } else {
     $customer_group_show_tax = $sppc_customer_group_show_tax;
     }

    if ( (DISPLAY_PRICE_WITH_TAX == 'true') && ($tax_class > 0) && ($customer_group_show_tax == '1')) {
// EOF Separate Pricing Per Customer, show_tax modification
      return $price + tep_calculate_tax($price, tep_get_tax_rate($tax_class));
    } else {
      return $price;
    }
  }

The difference is the call to the 'tep_get_tax_rate' function being added where it says

CODE

return $price + tep_calculate_tax($price, tep_get_tax_rate($tax_class));

and the var $tax being changed to $tax_class

Second - Open /catalog/includes/classes/currencies.php

Find

CODE

    function display_price($products_price, $products_tax, $quantity = 1) {
      return $this->format($this->calculate_price($products_price, $products_tax, $quantity));
    }

change to this

CODE

    function display_price($products_price, $products_tax_class, $quantity = 1) {
      return $this->format($this->calculate_price($products_price, $products_tax_class, $quantity));
    }

Next Find

CODE

    function calculate_price($products_price, $products_tax, $quantity = 1) {
      global $currency;

      return tep_round(tep_add_tax($products_price, $products_tax), $this->currencies[$currency]['decimal_places']) * $quantity;
    }

and change to this

CODE

    function calculate_price($products_price, $products_tax_class, $quantity = 1) {
      global $currency;

      return tep_round(tep_add_tax($products_price, $products_tax_class), $this->currencies[$currency]['decimal_places']) * $quantity;
    }

Finally - Do a site-wide search

Do a site wide search for the function call 'tep_get_tax_rate('

by doing a search for this (it will come in several files) .. what you need to do is remove the call ONLY where the calls are within calls to currency class functions.. they are no longer necessary.. just get rid of the function call and leave what is inside it.. (the tax class id) ..

basically if you see something like $currencies->format_number(blah , blah, tep_get_tax_rate(blah_func(something))) change it to $currencies->format_number(blah , blah, blah_func(something))

thats it.. let me know if you are missing something..

sashaben

May 17 2008, 03:21 AM

Thanks for the tip.
Would I be wrong in saying you can use both your tip and Chemos tep_get_tax together or is that kind of pointless?

OSCOMMERCE SUPPORT CALL 702-453-3332