Problem/Motivation

We've got a smallish commerce site with something like 60 products. We are using Commerce coupon to allow people to get discounted products etc. etc.
We also have some discounts that apply say, just on Wednesdays, that don't need a coupon, they are done using Commerce Discount only.

We have around 200 discounts, the majority using coupons, however, most people browsing the site don't have coupons on their order.

We want to render prices on the site with discounts taken into account, but when we do this we see a massive performance hit.

This is coming from a couple of places mainly all within commerce_coupon_discount_coupon_codes_exist_on_order.

  1. There's a check to see if the order allows coupons, like this: commerce_coupon_order_allows_coupons($order_wrapper->value()).

    However, commerce_coupon_order_allows_coupons just wraps the passed in value to call __isset using the EntityMetadataWrapper.

    This unwrapping and re-wrapping was taking a significant amount of time, and is pointless, IMHO.

  2. The commerce_coupon_discount_coupon_codes_exist_on_order seems to pointlessly check to ensure that the discount still exists and that it uses coupons.

    This seems to be because it's possible that some custom code or config might call the function with invald data, as the Commerce Coupon module itself does these checks before it adds the condition to the Rule in commerce_coupon_commerce_discount_rule_build.

    In my testing with xhprof, this was causing a massive slowdown because in particular the $discount_wrapper->value() check was actually causing an entity_load.

  3. The results of the expensive computation are not cached, so it's easy to repeat the work in the same page request over and over when the inputs aren't actually changing.

Additionally, much time was spent evaluating other rules conditions for these coupon discounts, like to check dates or if the discount is eligible, however, 99.9% of the time people don't have a coupon, so the commerce_coupon_discount_coupon_codes_exist_on_order would return FALSE anyway, wasting the time spent elsewhere.

Proposed resolution

Solve the above issues with:

  1. Allow the commerce_coupon_order_allows_coupons function to receive a wrapper and use that directly. Change the calling code in commerce_coupon_discount_coupon_codes_exist_on_order to not unwrap the order.
  2. Add an additional parameter to the rules condition that allows optional skipping of these discount checks if you know what you're doing,
    we then assume that the Commerce Coupon module knows what it's doing and set this to skip. The default however for any other calling code will be to not skip and keep the existing behavior.
  3. Add static caching to the commerce_coupon_discount_coupon_codes_exist_on_order function.
  4. When adding the condition to the rule in commerce_coupon_commerce_discount_rule_build add it and sort the conditions so that it is executed first. I assume that having a coupon is quite a 'rare' event, and worth checking quickly to see if all other processing can be aborted.

I don't think any of the above really changes the logic of the module? The only issue I can forsee is that we might try and look for a coupon when a discount has been deleted or somesuch, but it's my understanding that the reaction rule would have been deleted too?

Remaining tasks

Review the changes and discuss how appropriate they are.

User interface changes

Additional parameter to the rules condition visible in the Rules UI.

API changes

Additional parameter to the rules condition, additional parameter to commerce_coupon_discount_coupon_codes_exist_on_order - both should default to the current behavior.
commerce_coupon_order_allows_coupons now allows EntityMetadataWrappers to be passed in, not just $order objects.

Data model changes

None

Comments

Steven Jones created an issue. See original summary.

steven jones’s picture

StatusFileSize
new4.1 KB

Re-uploading the patch for the test bot.

steven jones’s picture

Issue summary: View changes
StatusFileSize
new5.23 KB

The previous patch could have done with some static caching of the results, adding that in too.

mglaman’s picture

The previous patch could have done with some static caching of the results, adding that in too.

Haven't reviewed yet, but that was my main question, if static caching of discount IDs/coupon IDs would help out. I'll give this a look over.

torgospizza’s picture

We're in need of this too, and are taking a look. Luckily it seems like this really only becomes an issue with large orders, but that really just makes the problem easier to see. Will test and provide feedback.

electric doorknob’s picture

Following on torgosPizza's comment with xhprof results of our testing.

The screenshots are with and without the patch in #3 on a /checkout page with a problematic order containing 184 line items. This unusually large order is exposing some scalability issues in various D7 Commerce modules that we're trying to knock down. The xhprof results are focused on commerce_coupon_discount_coupon_codes_exist_on_order()

steven jones’s picture

Issue summary: View changes

Thanks for testing this patch out!

In summary your execution time of commerce_coupon_discount_coupon_codes_exist_on_order was:

  • Before applying patch: 1546501μs
  • After applying patch: 69744μs

So about 95% faster :) Also, the memory usage looks to be not hugely more, 128B before, and 1544B after.