Contrib.social

Improve performance for batches involving promotions/coupons

Open on Drupal.org →
Created on 17 January 2024, over 1 year ago

Any kind of batch process involving promotions/coupons (most notably coupon generation) will run into exponential performance issues as the number of the promotion's coupons will grow.

This is because both entities enfore a reference to eachother in their respective postSave().

In the coupon's postSave(), after a redundant check if the promotion has the coupon (because addCoupon() already checks this), addCoupon() will trigger a promotion save:

// Ensure there's a back-reference on each coupon.
foreach ($this->getCoupons() as $coupon) {
  if (!$coupon->getPromotionId()) {
    $coupon->promotion_id = $this->id();
    $coupon->save();
  }
}

Then, in the promotion's postSave(), we loop all of its coupons and enforce the reference:

// Ensure there's a back-reference on each coupon.
foreach ($this->getCoupons() as $coupon) {
  if (!$coupon->getPromotionId()) {
    $coupon->promotion_id = $this->id();
    $coupon->save();
  }
}

When generating (or running a custom import of) coupons, this last piece of code is entirely redundant and will exponentially affect performance (try generating 10k coupons).

Ideally, the promotion entity would greatly benefit from a refresh service similar to that of the order entity, which would execute certain operations only when prompted.

✨ Feature request
Status

Active

Version

2.0

Component

Promotions

Created by

ts.ag

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @ts.ag
  • Comment over 1 year ago →
    ts.ag

    Attaching a simplistic patch which enables skipping the problematic operations inside both postSave()s if a batch is running and corresponding flags are set in its definition. Usage:

    $batch_definition = [
  'commerce_promotion__skip_ops' => TRUE,
  'commerce_promotion_coupon__skip_ops' => TRUE,
  ...
];
batch_set($batch_definition);
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    790 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update over 1 year ago
    790 pass
  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    ts.ag
  • Comment over 1 year ago →
    🇮🇱Israel jsacksick

    I experienced issues with this code too... In one of the projects I'm running, I completely stopped referencing coupons from the promotion.
    Whenever the promotion references too many coupons, crashes are experienced on save.
    Perhaps we can use direct SQL queries instead of fully loading entities etc... I don't think your fix is the right one.

  • Comment about 1 year ago →
    ts.ag

    Expanded previous patch to use the skip feature in the batch from CouponGenerateForm.

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update about 1 year ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year ago
    Patch Failed to Apply
  • Comment about 1 year ago →
    ts.ag

    Fixed patch from #5 due to of incorrect calls to $this.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year ago
    Patch Failed to Apply
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update about 1 year ago
    Patch Failed to Apply
  • Comment 5 days ago →
    🇺🇸United States loze Los Angeles

    I've run into this too.

    @jsacksick if you can can provide some guidance as to how you envision it working, Im happy to work on a MR.

  • Comment 5 days ago →
    🇺🇸United States loze Los Angeles

    Does the promotion need to reference the promotions? Couldn't the coupons just reference the promotions it applies to.
    then getCoupons() could just query for coupons referencing the promotion?

  • Comment 4 days ago →
    🇮🇱Israel jsacksick

    Does the promotion need to reference the promotions?

    You mean

    Does the promotion need to reference the promotions?

    .

    As mentioned in #4 in one of the projects I'm taking care of, promotions aren't referencing coupons as this wasn't scaling. However doing this in core would be a breaking change, so not 100% sure how to go about this because of that.

  • Comment 4 days ago →
    🇺🇸United States loze Los Angeles

    Yes, that's what I meant.

    I suppose what im getting at is, since you've gone down this road, if I do the same and patch commerce to remove the saving of the references on the promotion, out what other parts can I expect to break?

    I have a site with a lot of coupons, and am running into the same scaling issues.

  • Comment 4 days ago →
    🇮🇱Israel jsacksick

    @loze: I think I have some view overrides (but it's been a while so I'd have to double check), but basically it means you cannot rely on the relationship in views, but you can have a contextual filter based on the promotion (if that isn't already the case).

    I also have a patch to fix the UI indication showing the number of coupons referenced.

  • Comment 4 days ago →
    🇺🇸United States loze Los Angeles

    Thanks @jsacksick I will check this out.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024