commerce_cart_commerce_product_calculate_sell_price_line_item_alter() calls commerce_cart_order_id() which explicitely calls some additional SQL queries, while commerce_cart_order_load() will trigger the same function but will statically cache the result: it should be used instead.

SVN patch (sorry, no GIT here) 2 liner patch attached, which saves 1 to 20 SQL queries on my site.

Files: 
CommentFileSizeAuthor
#8 1631836-8.cache_commerce_cart_order_id.patch4.49 KBrszrama
PASSED: [[SimpleTest]]: [MySQL] 3,587 pass(es).
[ View ]
#6 commerce_cart-refresh_line_static_cache-1631836-2.patch3.65 KBmarcin.wosinek
FAILED: [[SimpleTest]]: [MySQL] 3,128 pass(es), 0 fail(s), and 26 exception(s).
[ View ]
#3 commerce_cart-refresh_line_static_cache-1631836.patch673 bytesmarcin.wosinek
PASSED: [[SimpleTest]]: [MySQL] 3,567 pass(es).
[ View ]
commerce_cart-refresh_line_static_cache.patch499 bytespounard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_cart-refresh_line_static_cache.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Alternatively, couldn't we move the static cache from commerce_cart_order_load() to commerce_cart_order_id()?

That probably could do the trick too. Since the cart will be loaded anyway it doesn't really change a thing, but yes, it may be "cleaner" in some way.

EDIT: For the record, my patch is correct because it triggers PHP warnings when the user has no cart.

Status:Active» Needs review
StatusFileSize
new673 bytes
PASSED: [[SimpleTest]]: [MySQL] 3,567 pass(es).
[ View ]

I've analysed patch and both functions used and this patch makes sense for me. Here it is slightly changed.

#1
It would be more clean, but we have multiple returns in commerce_cart_order_id. Because of this, I think we have two ways to move the static cache:

  • Introduce some kind of $to_return variable in commerce_cart_order_id, and have just one return
  • Introduce _commerce_cart_order_id doing all logic, and leave commerce_cart_order_id do do only caching

What do you think is a better way for it?

I dont see the point of introducing a new function for this, but I'm definitely ok for using a variable instead of the two return statements. Nevertheless, the function is small enough to leave no ambiguity in what it does so I'm fine with keeping the #3 patch too.

StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [MySQL] 3,128 pass(es), 0 fail(s), and 26 exception(s).
[ View ]

Here is patch implementing idea from comment #2. For me, both solutions are equal; I would chose one which introduce pattern more common in commerce.

Status:Needs review» Needs work

The last submitted patch, commerce_cart-refresh_line_static_cache-1631836-2.patch, failed testing.

Title:commerce_cart_commerce_product_calculate_sell_price_line_item_alter() trigger excessive queriesMove the static cache from commerce_cart_order_load() to commerce_cart_order_id()
Version:7.x-1.3» 7.x-1.x-dev
Category:bug» task
Status:Needs work» Needs review
StatusFileSize
new4.49 KB
PASSED: [[SimpleTest]]: [MySQL] 3,587 pass(es).
[ View ]

Honestly, I observed no difference in query counts with or without the patch. However, it does make more sense to cache cart order IDs in the actual function that's loading them. The function commerce_cart_order_ids_reset() also needed to be updated to reference the changed static cache function ID. Let's see what test bot thinks; in practice, it seemed to work fine for me.

Status:Needs review» Fixed

Alrighty, committed.

You can observe differences only if the cart is being manipulated more than once on the page, which can happen very fast as soon as you play with Views, blocks, etc... In my use case, it removed 9 SQL queries.

Issue tags:+Performance

Ahh, ok - I was only viewing a few listing / product pages. Glad to know it's helping. : )

Ahh, ok - I was only viewing a few listing / product pages. Glad to know it's helping. : )

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