Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving query performance for coupon code check #1110

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

lemundo-team
Copy link
Contributor

@lemundo-team lemundo-team commented Jul 13, 2020

Adding coupon code to where clause in order to improve query performance

Description (*)

This was one of the major bottle necks over the last few Black Fridays! Thousands of customers applying coupon codes in checkout and the checkout is causing causing more and more load.

It might be that this query is only leading to bad performance when using certain MySQL versions like #295 does...

Related Pull Requests

#303

Manual testing scenarios (*)

  1. Let 1000 customers apply coupon codes every minute
  2. Checkout the slow query log

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@tmotyl
Copy link
Contributor

tmotyl commented Jul 13, 2020

Hi
Can you give more context?
Eg how the original query looks like and why adding the where constraint on coupon speeds things up. Do you have any numbers before/after optimization?
What mysql version are you using?

@kiatng kiatng added Component: SalesRule Relates to Mage_SalesRule performance Performance related labels Jul 14, 2020
@fballiano
Copy link
Contributor

pinging @lemundo-team to see if we can get some info although this PR is quite old

fballiano
fballiano previously approved these changes Jun 14, 2022
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested and (although I don't have milions of records) the query goes from 26ms to 0.5ms so that's quite an important gain in the context of high traffic

with around 100k coupons the query goes from 150ms to 0.6ms, great result!!

@fballiano fballiano added review needed Problem should be verified and removed waiting for feedback labels Jun 14, 2022
@@ -91,7 +91,7 @@ public function setValidationFilter($websiteId, $customerGroupId, $couponCode =
$connection->quoteInto(
'main_table.rule_id = rule_coupons.rule_id AND main_table.coupon_type != ?',
Mage_SalesRule_Model_Rule::COUPON_TYPE_NO_COUPON
),
) . $connection->quoteInto(' AND rule_coupons.code = ?', $couponCode), // Fixing Performance Issue for Coupon Checkout!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen if $couponCode = ''? Has this been tested?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cart page works fine, before and after adding the coupon

Copy link
Collaborator

@kiatng kiatng Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the method has default $couponCode = '', possibly called somewhere else other than cart page. This needs to be tested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked, the method only reference once. So may be we can do a BC change

from

public function setValidationFilter($websiteId, $customerGroupId, $couponCode = '', $now = null)

to

public function setValidationFilter($websiteId, $customerGroupId, $couponCode, $now = null)

tobihille
tobihille previously approved these changes Aug 20, 2022
@luigifab
Copy link
Collaborator

luigifab commented Jan 17, 2023

I discover this PR today, I also found a dramatic performance loss with setValidationFilter, I try to fix the problem by rewriting left joins. I will also try the proposed change. An image from Blackfire (before any changes):

image

@fballiano fballiano dismissed stale reviews from tobihille and themself via 72a9c7c January 17, 2023 10:41
@fballiano
Copy link
Contributor

fixed conflict

@sreichel
Copy link
Contributor

@Flyingmana can we setup demo-instances and connect them to blackfire.io?

  • latest v19 release
  • latest v20 release
  • dev-main

@Flyingmana
Copy link
Member

@Flyingmana can we setup demo-instances and connect them to blackfire.io?

  • latest v19 release
  • latest v20 release
  • dev-main

we have our demo on nexess which is configured here: https://github.com/OpenMage/OpenMage-demo-deployment (and where we have also newRelic configured)

although this looks more like something which fits into the gitpod thing
Or you want a permanent one for all 3 branches?

@luigifab
Copy link
Collaborator

Before any changes from checkout/cart with a coupon applied:

image

With the current PR:

image

With another approach (© me, dev in progress):

image

With another approach and with the current PR (© me, dev in progress):

image

@fballiano
Copy link
Contributor

I think we should have both the fixes

@luigifab
Copy link
Collaborator

luigifab commented Jan 17, 2023

I haven't finished the crash test for my version.

@sreichel
Copy link
Contributor

Or you want a permanent one for all 3 branches?

this would be nice, but should be discussed somewhere else.

@fballiano fballiano mentioned this pull request Jan 18, 2023
4 tasks
@kiatng
Copy link
Collaborator

kiatng commented Jan 19, 2023

$result = Mage::getResourceModel('salesrule/rule_collection')
            ->setValidationFilter(1, 10, 'abc')
            ->getSelect()->__toString();

$result is

SELECT`main_table`.*, `rule_coupons`.`code` 
FROM `salesrule` AS `main_table` 
INNER JOIN `salesrule_customer_group` AS `customer_group_ids` 
  ON main_table.rule_id = customer_group_ids.rule_id AND customer_group_ids.customer_group_id = 10 
LEFT JOIN `salesrule_coupon` AS `rule_coupons` 
  ON main_table.rule_id = rule_coupons.rule_id 
    AND main_table.coupon_type != 1 #cond1
    AND rule_coupons.code = 'abc' #cond2 added by PR
WHERE 
  (
    EXISTS (
      SELECT 1 
      FROM `salesrule_website` AS `website` 
      WHERE 
        (website.website_id IN (1)) AND (main_table.rule_id = website.rule_id)
    )
  ) 
  AND (from_date is null or from_date <= '2023-01-19') 
  AND (to_date is null or to_date >= '2023-01-19') 
  AND (`is_active` = '1') 
  AND (
    main_table.coupon_type = 1 #see cond1, this is always false
    OR (
      (
        (
          main_table.coupon_type = 3  AND rule_coupons.type = 0
        ) 
        OR (
          main_table.coupon_type = 2  AND main_table.use_auto_generation = 1  AND rule_coupons.type = 1
        ) 
        OR (
          main_table.coupon_type = 2  AND main_table.use_auto_generation = 0  AND rule_coupons.type = 0
        )
      ) 
      AND rule_coupons.code = 'abc'
    )
  )

Can be simplified to

SELECT`main_table`.*, `rule_coupons`.`code` 
FROM `salesrule` AS `main_table` 
INNER JOIN `salesrule_customer_group` AS `customer_group_ids` 
  ON main_table.rule_id = customer_group_ids.rule_id AND customer_group_ids.customer_group_id = 10 
LEFT JOIN `salesrule_coupon` AS `rule_coupons` 
  ON main_table.rule_id = rule_coupons.rule_id 
    AND main_table.coupon_type != 1 
    AND rule_coupons.code = 'abc' #cond2 added by PR
WHERE 
  (
    EXISTS (
      SELECT 1 
      FROM `salesrule_website` AS `website` 
      WHERE 
        (website.website_id IN (1)) AND (main_table.rule_id = website.rule_id)
    )
  ) 
  AND (from_date is null or from_date <= '2023-01-19') 
  AND (to_date is null or to_date >= '2023-01-19') 
  AND (`is_active` = '1') 
  AND rule_coupons.code = 'abc' #cond3
  AND   
      (
        (
          main_table.coupon_type = 3 AND rule_coupons.type = 0
        ) 
        OR (
          main_table.coupon_type = 2 AND main_table.use_auto_generation <= 1 AND rule_coupons.type <= 1
        ) 
      ) 

I think either cond2 or cond3 can be removed, but not sure which one is faster.

Copy link
Collaborator

@luigifab luigifab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with a cart with one rule without code, and with another rule with a coupon code.
I think this is good.

@fballiano
Copy link
Contributor

@kiatng let's merge this in the meanwhile and see if we can improve it ;-)

@fballiano fballiano merged commit b787fc6 into OpenMage:1.9.4.x Jan 19, 2023
fballiano added a commit that referenced this pull request Jan 19, 2023
…#1110)

Co-authored-by: Hannes Drittler <h.drittler@lemundo.de>
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano
Copy link
Contributor

merged and cherrypicked to v20

@pquerner
Copy link
Contributor

Just FYI
We've backported this to our magento installation (we're slowly updating openmage to its newer versions, we don't want to break anything and carefully examine each release) and we got a HUUUGE performance increase. We are talking 10s of millions records here.
(Down from ~9s to ~1s)

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: SalesRule Relates to Mage_SalesRule performance Performance related review needed Problem should be verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants