Add StockItem#fill_status to enable perf improvements

[jordan-brough]

While reviewing #565 and checking performance I noticed that the Packer was really slow. If people think this is worth doing then I'll add specs for the new method.

This adds a fill_status method to StockItem and updates
StockLocation#fill_status to use it.

This allows us to avoid N+1 queries by loading multiple stock items at
once and querying them directly.

While using this as a perf test:
https://gist.github.com/jordan-brough/d614babd1202a890b7da
I saw this performance difference:

Old code: 4270ms
This PR: 772ms

[cbrunsdon]

I'm a big fan of the spec changes, and the optimization on the packer looks reasonable enough to me.

I would personally have probably done the following rather than the each_with_object:

def stock_item_lookup
  @stock_item_lookup ||= begin
    Hash[ Spree::StockItem.
      where(variant_id: inventory_units.map(&:variant_id).uniq).
      where(stock_location_id: stock_location.id).
      order(:id).map { |stock_item| [stock_item.variant_id, stock_item] } ]
  end
end

but don't care that much to ask someone to change it.

I might also have put an intermediary private stock_item_for(variant) rather than exposing the state of that hash to the method, but again not super strong.

[jordan-brough]

I might also have put an intermediary private stock_item_for(variant) rather than exposing the state of that hash to the method

Good point, I like that. I'll update if others are on board with the change in general.

I would personally have probably done the following rather than the each_with_object

No strong preference either way (though I think I'd say Array#to_h instead of Hash.[]?). But I do like how the current each_with_object explicitly chooses the first found stock item for a given variant_id to put into the hash. We'd have to do order(id: :desc) to accomplish that with to_h. There shouldn't be any collisions, but we don't have db-level constraints around that right now so it feels good to be safe and use the first one like StockLocation.stock_item does.

[cbrunsdon]

Oh snap, good point, I hadn't realized I clobber that functionality when I swapped it. I'm cool with yours.

[mtomov]

Thank you very much for pursuing those performance improvements. They are indeed much needed!

[jhawthorn]

StockItem has

validates_uniqueness_of :variant_id, scope: [:stock_location_id, :deleted_at]

So we should only ever have active stock item per-stock-location-variant. I'd prefer we use Array#to_h and also lose the order(:id).

Otherwise this is great 👍

Side note, I love the performance test gist. Is there anywhere in the project it would make sense to include these sort of benchmarks?

[mtomov]

I also love the performance test. The most difficult part of those is to start, but if there's a place to copy paste it from, that would indeed be great.

Maybe in a new folder under specs, and add an Rspec tag of performance, and don't run it by default ? Or maybe they were called "stress / smoke", or something of the kind.. Have to lookup how other libraries are doing it. I think Rails must have those.

[jordan-brough]

@jhawthorn:

So we should only ever have active stock item per-stock-location-variant

I didn't want to trust the application-level uniqueness constraint since it's vulnerable to race conditions. I've now added a commit that adds a db-level uniqueness constraint for DBs that support them in this scenario (Postgres and SQLite) and switched to to_h.

Btw, I was also trying to avoid the temporary arrays generated by the to_h approach but the perf test didn't show any difference when switching to to_h so that seems fine.

@cbrunsdon:

I've now added a stock_item_for method.

[jordan-brough]

Also FYI I've added the sprockets-related commit here just so that the specs will pass until #605 (or something equivalent) gets merged into master.

[cbrunsdon]

Balling, 👍 from me, but we'll have to fix the history for the sprockets of course.

[jordan-brough]

The sprockets thing got merged so I've updated to remove that commit.