Performance improvements on package generation

[dfmedeiros]

Right now, when Solidus tries to create the packages on Coordinator, it iterates over each one of the active stock locations and skip those without the requested variants in stock.

Let's suppose we have 100 different stock locations (which is my case) and the current user has 5 items on his cart. Once it tries to create the packages it triggers 500 different queries to the database.

Here I try to address this issue, iterating over just the stock locations with the requested variants in stock.

[cbrunsdon]

@dfmedeiros this looks fantastic, really appreciated.

I'm slammed by BlackFriday/Monday stuff but will pull it down and give it a run early next week. On first glance everything looks great though.

[jhawthorn]

Looks good to me. 👍

I bet we could go further here and select the stock items grouped by stock_location, and skip the N+1 on line 59, but this is an excellent step towards that.

[dfmedeiros]

@cbrunsdon - Thanks for the feedback!

@jhawthorn - Yep, good catch! But I didn't get it correctly, what do you mean by select the stock items grouped by stock_location?

My plan is to select the correct stock items by variant_id including the stock_location and the variant itself, so we could check inside the loop. I think this would prevent the N + 1 as well. Something like:

def requested_stock_items
  Spree::StockItem.where(variant_id: unnalocated_variant_ids).includes(:variant, :stock_location)
end

def build_packages(packages = Array.new)
  requested_stock_items.each do |stock_item|
    units_for_location = unallocated_inventory_units.select { |unit| unit.variant.eql?(stock_item.variant) }
    packer = build_packer(stock_item.stock_location, units_for_location)
    packages += packer.packages
  end
  packages
end

What do you think about this approach? Do you recommend doing something else?
Thanks again.

[athal7]

👍 on this and on @jhawthorn's suggestion

[dfmedeiros]

@jhawthorn - Forget about my last comment, it doesn't work as I expected, but, I didn't get your last comment to select stock items grouped by stock location. I don't see how this would do any benefit for the queries on line 59.

[philbirt]

unallocated*

[philbirt]

Solid 👍

[jhawthorn]

@dfmedeiros no worries. We can worry about future improvements after addressing this. I think this is good to merge pending the spelling fix.

[dfmedeiros]

@jhawthorn - Great, just fixed the typo. Thanks.

[jhawthorn]

Thank you @dfmedeiros