Reduce price calculation attempts on a single product page

The sell price depends on the context (current user, etc.) that is not explicitly passed to the function. Would there be another way to avoid the double processing?

"Calculate sell price of a product" is firing twice on checkout/1234 causing my discount rule to apply twice. I applied the patch in #1 but it hasn't solved the double-discount.

Edit: sorry, ignore that. I was caught out by the persistence of Commerce Discount. Even when disabled the Discount line item remained in an order.

Yeah, fwiw I filed an issue against Commerce Discount related to the shopping cart refresh w/ Lukas last week. Probably the same thing you're noticing.

I'm not sure if this is related, but it looks like the "Calculating the sell price of a product" Event is firing for many (if not most) of my administration pages.

I see "Rules Evaluation Log" messages with a "Calculating the sell price of a product" trace in my administration section when I view a menu, a view, a content type, and a number of other places... (none of which have anything to do with products)

I was experiencing a similar behavior for a Rule that checked for admin role in order to fire. Then I noticed after dpm'ing that the Rule was also accessing data from a previous revision of the Product (Commerce Products come with revisions enabled by default). After removing the admin role check, the Rule is firing now only once and using the current data correctly.

I apply'd the patch to my site. Calculation rules continue to work as expected. Rules just trigges one time.

Same patch as #1 just moved the condition and early return to the top instead of wrapping. Not too concerned which patch gets in just find the approach is easier to see the changes in the patch review and slightly easier to sort out problems when "caching goes wrong" and who can say "no" to less nesting;)

I'd RTBC this issue though:)

Patch in #9 works great for me. Price calculation is only run once per product. Thanks, @joelpittet!

Credit goes to boom boom @bojanz ;)

Before committing, I think we may need to revisit Damien's feedback re: the full context. The issue is these lines come after we've already exited the function early based solely on the product ID:

  // Allow modules to prepare this as necessary.
  drupal_alter('commerce_product_calculate_sell_price_line_item', $line_item);

So it's possible the same product could end up being calculated with two different line item contexts in the same page request. I'm not sure how likely it is, but we can try to imagine applicable scenarios. (If none come to mind, we can always go with this.)

Also, fortunately the shopping cart refresh frequency settings we added in in Commerce 1.9 mitigate some of the other pain points mentioned in the comments above. But it is worth noting that the recalculation due to the shopping cart refresh remains unaffected by this patch. This patch primarily reduces the multiple price calculations due to the same function being called for both price field rendering and Add to Cart button checking.

The cart refresh frequency sketches me out a bit, so I've got it set to 0 because while developing I wasn't sure if I was getting the correct results or not. Though I probably don't understand what it's doing to make that kind of snap judgement.

It's hard to picture a scenario where the $line_item on the same call would change itself in the alter then expect a second call to re-calculate on the same request to use that context in order to change the price (which they didn't change already on the first call). Seems a bit far fetched. So I'm on the "none come to mind" bench.

The only thing in Commerce core that implements that alter hook is the Cart module to set the current cart order ID on the $line_item. So if a site were managing multiple carts for a user and sticking the same product into different carts, with one passing a bulk discount threshold and another not, it would create an error. But that's such an edge case... and I'm not aware if any other piece of code ever implements this hook.

We could always try a hash of the $line_item as a key in addition to the $product_id, but then a difference in the data array that's inconsequential to product pricing rules could mitigate static caching. That might be a lesser of two evils, though, compared to returning an incorrect price.

Tagging for sprint so we can review.

I'm not sure how we could make a hash. We couldn't just do a simple md5($line_item.$product); because of timestamps. Even with order refresh, calculation gets called an insane amount of times it seems. Try keeping xdebug on and have a breakpoint within the function here.

I've tried to hunt down a module implementing hook_commerce_product_calculate_sell_price_line_item and came up short so far.

I like that this adds stability in knowing that we calculate the price once. It'll help with debugging as well since xdebug won't trigger three times for one product's calculation :)

Per #14. Here is a more robust cache ID based on product ID, line item hash, and precalc. Also moved calculation logic inside of the "not set" so there's one single return point for code smellability.

Whoops. Converting object to JSON because we don't need the performance hit / cruft in serialize.

Google source for performance niceties over JSON v. serialize: http://stackoverflow.com/a/804089/1949744

This patch works for me as expected!

Moving this backs to needs review. We need to see if the performance of running json_encode on the object is negating any performance gained by running calculations less. Also need to see how often the cache is even hit. This should be tested on a site using discounts and other pricing rules.

I just ran some tests on this patch using XHProf for profiling. I profiled product listing pages, product details pages, and product details pages with discounts applied. In all cases the run time for this function increased ~60% with the patch applied.

Incidentally, this function before the patch took 52 microseconds to run twice on a basic product page on the site I was testing it on, not exactly a massive performance hit as it stands now anyway.

Small gain, but might as well mark it as RTBC as it's good to go even though it's a minimal gain.

Wait, tho - the runtime for the function increased by 60%? As in, it took longer to run?