Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index eager load performance #2211

Merged
merged 5 commits into from
Jun 28, 2022

Conversation

jamie
Copy link
Contributor

@jamie jamie commented Jun 14, 2022

Attempted fix for Can't avoid under/overfetching

Hey @pablobm - I took your suggestion from point 1 here and ran it a little further to get the tests passing.

  • Update attribute_includes to restrict eager loading to BelongsTo and HasOne association fields
  • Also created item_associations and attribute_associated methods with the old behaviour so that sanitized_order_params in ApplicationHelper still works correctly - it was needed to fix a few specs I think related to column sorting.
  • No new specs, as there should be no behavioural change to users.

Old request logs from the test app (excluding renders):

Started GET "/admin/customers?search=jesus" for 127.0.0.1 at 2022-06-13 20:38:22 -0700
   (1.2ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
Processing by Admin::CustomersController#index as HTML
  Parameters: {"search"=>"jesus"}
  Customer Load (2.4ms)  SELECT "customers".* FROM "customers" LEFT OUTER JOIN "countries" ON "countries"."code" = "customers"."country_code" WHERE "customers"."hidden" = $1 AND (LOWER(CAST("customers"."email" AS CHAR(256))) LIKE '%jesus%' OR LOWER(CAST("customers"."name" AS CHAR(256))) LIKE '%jesus%' OR LOWER(CAST("customers"."kind" AS CHAR(256))) LIKE '%jesus%' OR LOWER(CAST("countries"."name" AS CHAR(256))) LIKE '%jesus%') LIMIT $2 OFFSET $3  [["hidden", false], ["LIMIT", 20], ["OFFSET", 0]]
  Order Load (0.4ms)  SELECT "orders".* FROM "orders" WHERE "orders"."customer_id" = $1  [["customer_id", 1]]
  LogEntry Load (0.4ms)  SELECT "log_entries".* FROM "log_entries" WHERE "log_entries"."logeable_type" = $1 AND "log_entries"."logeable_id" = $2  [["logeable_type", "Customer"], ["logeable_id", 1]]
  Country Load (0.6ms)  SELECT "countries".* FROM "countries" WHERE "countries"."code" = $1  [["code", "CN"]]
  LineItem Load (0.4ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 1]]
  ↳ app/models/order.rb:16:in `map'
  LineItem Load (0.2ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 2]]
  ↳ app/models/order.rb:16:in `map'
  LineItem Load (0.2ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 3]]
  ↳ app/models/order.rb:16:in `map'
  LineItem Load (0.3ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 4]]
  ↳ app/models/order.rb:16:in `map'
Completed 200 OK in 1502ms (Views: 1442.8ms | ActiveRecord: 22.1ms | Allocations: 803206)

Same request, with these changes:

Started GET "/admin/customers?search=jesus" for 127.0.0.1 at 2022-06-13 20:39:38 -0700
   (0.4ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
Processing by Admin::CustomersController#index as HTML
  Parameters: {"search"=>"jesus"}
  Customer Load (2.4ms)  SELECT "customers".* FROM "customers" LEFT OUTER JOIN "countries" ON "countries"."code" = "customers"."country_code" WHERE "customers"."hidden" = $1 AND (LOWER(CAST("customers"."email" AS CHAR(256))) LIKE '%jesus%' OR LOWER(CAST("customers"."name" AS CHAR(256))) LIKE '%jesus%' OR LOWER(CAST("customers"."kind" AS CHAR(256))) LIKE '%jesus%' OR LOWER(CAST("countries"."name" AS CHAR(256))) LIKE '%jesus%') LIMIT $2 OFFSET $3  [["hidden", false], ["LIMIT", 20], ["OFFSET", 0]]
  Country Load (0.2ms)  SELECT "countries".* FROM "countries" WHERE "countries"."code" = $1  [["code", "CN"]]
  Order Load (0.6ms)  SELECT "orders".* FROM "orders" WHERE "orders"."customer_id" = $1  [["customer_id", 1]]
  ↳ app/models/customer.rb:24:in `map'
  LineItem Load (1.0ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 1]]
  ↳ app/models/order.rb:16:in `map'
  LineItem Load (0.3ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 2]]
  ↳ app/models/order.rb:16:in `map'
  LineItem Load (0.1ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 3]]
  ↳ app/models/order.rb:16:in `map'
  LineItem Load (0.1ms)  SELECT "line_items".* FROM "line_items" WHERE "line_items"."order_id" = $1  [["order_id", 4]]
  ↳ app/models/order.rb:16:in `map'
   (0.6ms)  SELECT COUNT(*) FROM "log_entries" WHERE "log_entries"."logeable_id" = $1 AND "log_entries"."logeable_type" = $2  [["logeable_id", 1], ["logeable_type", "Customer"]]
  ↳ app/views/fields/has_many_variant/_index.html.erb:6
Completed 200 OK in 1394ms (Views: 1348.5ms | ActiveRecord: 18.2ms | Allocations: 799880)

This change converted the log_entries from an eager load to just a count. The orders association would also have converted, except that there's that silly lifetime_value calculation that does some manual math on the orders, so it gets loaded in for that anyway (and is its own N+1 problem, but that's not Administrate's fault).

jamie added 2 commits June 13, 2022 19:35
Modifies BaseDashboard#attribute_includes to exclude HasMany fields
from being eager loaded, as they would only be used to provide a
count on index/show pages by default.

Also sets up a sibling #attribute_associated with the old behaviour
for ApplicationHelper#sanitized_order_params so that they will still
be whitelisted as column sorting options.
@pablobm
Copy link
Collaborator

pablobm commented Jun 28, 2022

Thank you!

@pablobm pablobm merged commit 4de073d into thoughtbot:main Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants