It's not a bug linked to a specific part of Commerce, nevertheless I will discuss it with a specific example: the commerce_tax_type_calculate_rates() function.

On the site I'm working on, I'm calculating the sell prices using the hook_commerce_product_calculate_sell_price_line_item_alter() hook instead of using rules (the business is a bit tricky and I don't want to create hundreds of default rules).

I managed to push taxes into the commerce_tax module tables, they per default inherit from a rules component identifier, but because I don't need it, I used the hook_commerce_tax_rate_info_alter() to remove it thus allowing me to avoid useless rules even trigger.

The problem, in this specific example, is that you call rules_get_components() before knowing if you are going to use it, and in my case, it's not being used, the result is 20 SQL queries from the rules entity controller (yes, 20 just for this example) because it attemps load on all rules components (20 of them) using $conditions arrays in the entity controller load method (it bypasses all caches).

If commerce did check in all cases if the rules really need to be triggered before loading the components, it could save a lot of SQL queries.

Attached a quick and dirty way to solve it in my use case as an example.

#5 1632254-5.commerce_tax_component_performance.patch1.56 KBrszrama
PASSED: [[SimpleTest]]: [MySQL] 3,587 pass(es).
[ View ]
commerce_tax-dont_invoke_rules_when_unneeded.patch1.87 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_tax-dont_invoke_rules_when_unneeded.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]


Damien Tournoud’s picture

Status:Active» Needs work

It seems to me that we should collect the 'rules_component' that we are looking for and then do a entity_load() (or equivalent) to load them all in one go. That would do the same thing as this patch but would in addition avoid loading *all* the rules actions defined on the website just for that. That would also leverage the entity cache better, so that we can avoid any loading when invoking commerce_tax_type_calculate_rates() repetitively.

pounard’s picture

Just to be clear, this patch does not add all the rules actions defined loading, the legacy code already did that. It seems a better plan than the actual code.

Damien Tournoud’s picture

Tried to clarified #1: I was not hinting this patch introduced any regression.

pounard’s picture

Ok, makes sense, anyway, this needs some fixing, I didn't look out over the full commerce suite code to see if other pieces were doing the same (I didn't find any other in my xdebug profiling) but it could worth to search just to be sure this has not been done anywhere else.

rszrama’s picture

Title:Do no load rules components when it is not needed» Only load the rules components we need to calculate taxes instead of the whole set
Version:7.x-1.3» 7.x-1.x-dev
Component:Other» Tax
Category:bug» task
Status:Needs work» Needs review
Issue tags:+Performance
new1.56 KB
PASSED: [[SimpleTest]]: [MySQL] 3,587 pass(es).
[ View ]

Did as Damien recommended; letting test bot have a bite, though quick testing seemed to be ok.

rszrama’s picture

Status:Needs review» Fixed


pounard’s picture

If you surround the foreach with an if (!empty($component_names)) { it would be perfect. Very useful patch, thanks.

rszrama’s picture

Ok, committed that update.

Automatically closed -- issue fixed for 2 weeks with no activity.