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.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 1632254-5.commerce_tax_component_performance.patch | 1.56 KB | rszrama |
| commerce_tax-dont_invoke_rules_when_unneeded.patch | 1.87 KB | pounard |
Comments
Comment #1
damien tournoud commentedIt seems to me that we should collect the
'rules_component'that we are looking for and then do aentity_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 invokingcommerce_tax_type_calculate_rates()repetitively.Comment #2
pounardJust 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.
Comment #3
damien tournoud commentedTried to clarified #1: I was not hinting this patch introduced any regression.
Comment #4
pounardOk, 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.
Comment #5
rszrama commentedDid as Damien recommended; letting test bot have a bite, though quick testing seemed to be ok.
Comment #6
rszrama commentedCommitted.
Comment #7
pounardIf you surround the foreach with an
if (!empty($component_names)) {it would be perfect. Very useful patch, thanks.Comment #8
rszrama commentedOk, committed that update.