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.

CommentFileSizeAuthor
#8 1631836-8.cache_commerce_cart_order_id.patch4.49 KBrszrama
#6 commerce_cart-refresh_line_static_cache-1631836-2.patch3.65 KBmarcin.wosinek
#3 commerce_cart-refresh_line_static_cache-1631836.patch673 bytesmarcin.wosinek
commerce_cart-refresh_line_static_cache.patch499 bytespounard
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

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

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

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.

marcin.wosinek’s picture

marcin.wosinek CreditAttribution: marcin.wosinek commented
Status: Active » Needs review
FileSize
commerce_cart-refresh_line_static_cache-1631836.patch673 bytes

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

marcin.wosinek’s picture

marcin.wosinek CreditAttribution: marcin.wosinek commented

#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?

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

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.

marcin.wosinek’s picture

marcin.wosinek CreditAttribution: marcin.wosinek commented
FileSize
commerce_cart-refresh_line_static_cache-1631836-2.patch3.65 KB

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.

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Title: commerce_cart_commerce_product_calculate_sell_price_line_item_alter() trigger excessive queries » Move 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
FileSize
1631836-8.cache_commerce_cart_order_id.patch4.49 KB

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.

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Status: Needs review » Fixed

Alrighty, committed.

pounard’s picture

pounard
Primary language French
CreditAttribution: pounard commented

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.

rszrama’s picture

rszrama CreditAttribution: rszrama commented
Issue tags: +Performance

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

rszrama’s picture

rszrama CreditAttribution: rszrama commented

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.