More eager loading in admin and api #3398

softr8 · 2019-10-24T00:57:11Z

Description
With recent changes, some previous eager loading attempts are no longer applicable, I'm trying to update as much as possible.

I've added the option to specify includes in the resource controller when the resource depends of another one and is specified via belongs_to. We can now use it everywhere

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)

backend/app/controllers/spree/admin/product_properties_controller.rb

tvdeyen

Awesome, thanks.

spaghetticode

@softr8 thank you for this PR!

I have some minor concerns about some changes, please let me know what you think... thanks 🙏

spaghetticode · 2019-10-25T08:58:58Z

api/app/views/spree/api/orders/_order.json.jbuilder

@@ -3,7 +3,7 @@
 json.cache! [I18n.locale, order] do
  json.(order, *order_attributes)
  json.display_item_total(order.display_item_total.to_s)
-  json.total_quantity(order.line_items.sum(:quantity))
+  json.total_quantity(order.line_items.to_a.sum(&:quantity))


Not sure about this change... I think the previous code was simpler and more performant, as the sum was done directly in the DB, without needing for ruby to instantiate all the line items, convert them to_a and sum their quantity fields... or is the change for another reason?

The order should already have preloaded the line items. If we do a sum in the database than we always load it from the database. And even if the line items have not been preloaded yet, they would be loaded by this call. It is always preferable to use array methods instead of where statements as the latter will always - no matter what - load from the database, while the former would only load if necessary.

Great write up https://www.speedshop.co/2019/01/10/three-activerecord-mistakes.html

Oh, I see:

order = create :order_with_line_items, line_items_count: 10 Benchmark.bm do |bm| bm.report { order.line_items.sum :quantity } bm.report { order.line_items.to_a.sum &:quantity } bm.report { order.line_items.reload.to_a.sum &:quantity } end user system total real 0.000908 0.000217 0.001125 ( 0.001124) 0.000036 0.000006 0.000042 ( 0.000041) 0.000956 0.000162 0.001118 ( 0.001122)

so, as long as the association has already been loaded from the DB, using ruby is so much faster... and when not, the numbers seem to be in the same ballpark. Then this makes definitely sense 👍

spaghetticode · 2019-10-25T09:26:33Z

core/app/models/concerns/spree/user_methods.rb

+    def available_store_credit_total(currency:, force_reload: true)
+      # This reload makes eager loading not to work when hitting read only endpoints
+      store_credits.reload if force_reload
+      store_credits.to_a.


while we are at it, I think it may be worth, in a dedicated commit, to slightly optimize this method... I don't see any reason to not use ActiveRecord for selecting the store credits with the right currency:

store_credits.where(currency: currency).sum(&:amount_remaining)

what do you think?

Not sure about it, the thing is that we're trying to re-use as much as possible all data pre-loaded with eager loading, and if we do it, it'd execute extra queries to the db.

spaghetticode · 2019-10-25T09:33:39Z

core/app/models/spree/order.rb

@@ -671,13 +672,13 @@ def add_store_credit_payments

    def covered_by_store_credit?
      return false unless user
-      user.available_store_credit_total(currency: currency) >= total
+      user.available_store_credit_total(currency: currency, force_reload: false) >= total


I think we're changing the behavior here, as it was reloading previously... can you explain the reason for this?

spaghetticode · 2019-10-25T09:34:03Z

core/app/models/spree/order.rb

    end
    alias_method :covered_by_store_credit, :covered_by_store_credit?

    def total_available_store_credit
      return 0.0 unless user
-      user.available_store_credit_total(currency: currency)
+      user.available_store_credit_total(currency: currency, force_reload: false)


same as above, I think the behavior is changed now

spaghetticode · 2019-10-25T09:35:21Z

core/app/models/spree/order.rb

@@ -686,9 +687,9 @@ def order_total_after_store_credit

    def total_applicable_store_credit
      if can_complete? || complete?
-        payments.store_credits.valid.sum(:amount)
+        valid_store_credit_payments.to_a.sum(&:amount)


Like above, I think the previous version was more performant, as it did all calculations in the DB without instantiating and cycling ruby objects

It is not more performant this way. Quite contrary this only does the math in the database if the payments have not been loaded yet. If they already were loaded the sum in database approach is actually slower, as it unnecessarily loads the payments again.

spaghetticode · 2019-10-25T09:38:35Z

core/app/models/spree/order.rb

      else
-        [total, (user.try(:available_store_credit_total, currency: currency) || 0.0)].min
+        [total, (user.try(:available_store_credit_total, currency: currency, force_reload: false) || 0.0)].min


I see another change of behavior... by the way, this may be totally legit, I did not investigate, can you share your motivation for this change? 🙏

With the concept of galleries, the relationship changed and it now uses variant images

This changes allows to specify eager loading in the belongs_to definition at the conrtoller level class Controller < Spree::Admin::ResourceController belongs_to :product, find_by: :slug, includes: {variant: :prices} end

Calling product property name was causing query n+1, eager loading properties solved the problem

We can eager load only the first level

It requires a lot of information to be rendered, this change tries to eager load as much data as possible, perfomance gains are notable in orders with multiple line items

spaghetticode · 2019-11-13T16:26:26Z

@softr8 I reviewed the changes, now I understand using to_a should be beneficial for performances 👍

I see specs are red, can you look if the issue is related to these changes?

Updated eager loading relationships, it no renders big partial so no need to eager load extra information, created new relationship to fetch valid store credits payments so rails can load them automatically and also used previous relationship with .to_a instead of active record executing extra queries

spaghetticode

looks good to me, thank you @softr8 👍

houndci-bot reviewed Oct 24, 2019

View reviewed changes

backend/app/controllers/spree/admin/product_properties_controller.rb Outdated Show resolved Hide resolved

softr8 force-pushed the more-eager-loading-in-ddmin branch from cb7fd19 to 99dbbfc Compare October 24, 2019 17:38

softr8 marked this pull request as ready for review October 24, 2019 19:43

tvdeyen approved these changes Oct 25, 2019

View reviewed changes

spaghetticode reviewed Oct 25, 2019

View reviewed changes

kennyadsl added the Needs Work label Nov 6, 2019

softr8 added 5 commits November 8, 2019 16:19

Added eager loading to product variant images

90a1046

With the concept of galleries, the relationship changed and it now uses variant images

Allowing resources controller to eager load objects

1e84994

This changes allows to specify eager loading in the belongs_to definition at the conrtoller level class Controller < Spree::Admin::ResourceController belongs_to :product, find_by: :slug, includes: {variant: :prices} end

Eager loading product properties

22c25cb

Calling product property name was causing query n+1, eager loading properties solved the problem

Eager loading taxons information in taxonomy endpoint

4dd5e68

We can eager load only the first level

Eager loading order data in orders api endpoint

10c4709

It requires a lot of information to be rendered, this change tries to eager load as much data as possible, perfomance gains are notable in orders with multiple line items

softr8 force-pushed the more-eager-loading-in-ddmin branch from 99dbbfc to c1f89ef Compare November 8, 2019 22:54

softr8 force-pushed the more-eager-loading-in-ddmin branch from c1f89ef to 234a36d Compare November 13, 2019 23:08

spaghetticode approved these changes Nov 14, 2019

View reviewed changes

spaghetticode removed the Needs Work label Nov 15, 2019

spaghetticode merged commit a8bef20 into solidusio:master Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More eager loading in admin and api #3398

More eager loading in admin and api #3398

softr8 commented Oct 24, 2019

tvdeyen left a comment

spaghetticode left a comment

spaghetticode Oct 25, 2019

tvdeyen Nov 6, 2019

tvdeyen Nov 6, 2019

spaghetticode Nov 13, 2019

spaghetticode Oct 25, 2019

softr8 Nov 8, 2019

spaghetticode Oct 25, 2019

spaghetticode Oct 25, 2019

spaghetticode Oct 25, 2019

tvdeyen Nov 6, 2019

spaghetticode Oct 25, 2019

spaghetticode commented Nov 13, 2019

spaghetticode left a comment

More eager loading in admin and api #3398

More eager loading in admin and api #3398

Conversation

softr8 commented Oct 24, 2019

tvdeyen left a comment

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spaghetticode commented Nov 13, 2019

spaghetticode left a comment

Choose a reason for hiding this comment