Perf improvements for Stock::Coordinator#build_packages #574
Conversation
|
This is definitely getting more complicated to read than I'm happy with. I tried and couldn't make any changes to make it noticeably cleaner though. Once I got my head wrapped around the whole thing I definitely am okay with the change in logic, seems to be doing the same thing (which is good!). I'm going to say I'm pretty ambivalent on the change too, I'm sure when Hawth's back next week he'll have an opinion though. |
| location_variant_ids.each_with_object({}) do |(location_id, variant_id), hash| | ||
| hash[location_lookup[location_id]] ||= Set.new | ||
| hash[location_lookup[location_id]] << variant_id | ||
| end |
There was a problem hiding this comment.
<nitpick> I don't like ending a method with a block like that; I don't find it particularly easy to figure out that's what the method returns. If I were writing this, I'd assign the result of the block to a variable and then explicitly return that variable on the last line.</nitpick>
There was a problem hiding this comment.
That sounds great to me. Thanks. I'll update if we decide we like this.
There was a problem hiding this comment.
@magnusvk I think Enumerable#each_with_object will return the hash. I believe the nature of it was so that you didn't have to store it in a variable and then return it :p
There was a problem hiding this comment.
Oh, I know it will return the hash. I just don't find that to be very readable code.
|
Harder-to-read code is often faster than "prettier," more naive code. But performance matters, particularly in a framework like Solidus. If we have solid tests around these methods I don't see a problem with adding this complexity to make them faster. I'm just curious if this is a premature optimization -- is this only an issue in contrived examples or does this actually happen in real stores? If the latter, I'm 👍. |
It very well could be. @dfmedeiros you mentioned in #550 that you had 100 stock locations. For a given item, how many of those locations would you expect to have the item in stock? My testing used 100 stock locations with 30 items in an order and all of the items in stock in every stock location. What would be the maximum reasonable scenario in your store, for example? |
Ditto on both fronts. I'm not super gung-ho on this (I just tried it out while I was looking at the area) so if no one jumps in saying they think these changes are awesome and will be great for them then I'll be happy to just close the PR. |
| def unallocated_inventory_units | ||
| inventory_units - @preallocated_inventory_units | ||
| def stock_location_variant_ids | ||
| location_variant_ids = StockItem.where(variant_id: unallocated_variant_ids).joins(:stock_location).merge(StockLocation.active).pluck(:stock_location_id, :variant_id) |
There was a problem hiding this comment.
I feel like some of this should be moved off to a scope and then use ruby's multiple assignment abilities to make things more readable:
stock_location_ids, variant_ids = StockItem.stock_location_and_variant_ids
location_lookup = StockLocation.where(id: stock_location_ids.uniq).map { |l| [l.id, l] }.to_hThis way you can write stock_location_ids.uniq instead of location_variant_ids.map(&:first).uniq.
There was a problem hiding this comment.
@BenMorganIO I don't need stock_location_ids and variant_ids separately, I need them associated with each other. And I need them in a specific enough way (for performance) that it doesn't seem good to move this into the model. Everything here is already using a scope or else is the way it is for perf reasons.
|
👍 from me. I think this is a good performance win. I don't think the variant being in stock in all locations will be a common case. However I think the performance test gist does a good job of demonstrating the worst case: 3000 stock items across 100 stock locations. The previous code loaded 3000 |
|
@jhawthorn I updated with @magnusvk's suggestion and added a bunch of comments. The comments are lengthy but the logic is a bit hard to follow so it seemed worth it. |
jordan-brough
force-pushed
the
improve-coordinator-perf
branch
2 times, most recently
from
301679b to
d7639e9
Compare
|
FYI: I added the sprockets commit here just to get the specs passing. See #605. |
jordan-brough
force-pushed
the
improve-coordinator-perf
branch
from
1dd3ab5 to
27a9037
Compare
|
The sprockets thing got merged so I've updated to remove that commit. |
|
👍, thanks Jordan. will be interesting to see if 6 months from now anyone found the comments helpful. |
Perf improvements for Stock::Coordinator#build_packages
I noticed this while looking at PR 565. Just for kicks I took a stab at improving the stock item querying and eliminating the querying and instantiation of a stock location for every stock item and here's what I saw for an extreme test case (30 variants and 100 stock locations):
This does use 2 queries instead of 1, but each query is simpler and should be faster.
Overall I'm not sure this one is worth the extra complexity but what do others think?