Performance related coding flaws

[insaini]

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

// 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

// 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

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

 

to this

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

 

and this

	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

	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]

Good work.

 

There are some works going on already;

http://www.oscommerce.com/forums/index.php?showtopic=110585

http://www.oscommerce.com/community/contributions,2417

[insaini]

Good work.

 

There are some works going on already;

http://www.oscommerce.com/forums/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 :P

[♥FWR Media]

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]

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]

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]

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)

// 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

// 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

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

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

 

 

change to this

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

 

 

Next Find

	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

	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]

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?