Only preload misses on multifetch cache by lsylvester · Pull Request #31250 · rails/rails

lsylvester · 2017-11-28T02:42:32Z

Summary

When rendering a collection with cached: true for an ActiveRecord::Relation that has preload values, the preload would normally only be required for rendering the partial when it misses the cache and nor required for generating the cache key.

Currently, we have to chose between using preload, which loads unused ActiveRecord instances when the have a high cache hit rate, or not preloading the associations, which can result in N + 1 queries when there are a large number of cache misses.

This PR attempts to optimise this be only preloading the associations for the records that miss the cache.

An example application to demonstrate this behaviour is at https://github.com/lsylvester/multifetch-demo. An example from the log demonstrating this behaviour is:

  Rendering categories/index.html.erb within layouts/application
  Category Load (0.1ms)  SELECT "categories".* FROM "categories"
  Product Load (0.4ms)  SELECT "products".* FROM "products" WHERE "products"."category_id" IN (?, ?, ?, ?)  [["category_id", 4], ["category_id", 5], ["category_id", 8], ["category_id", 9]]
  Rendered collection of categories/_category.html.erb [18 / 22 cache hits] (57.9ms)

Without this change, we would either so a query for each of the categories that miss the cache, or have to load products for every category.

Concerns and Further Work

There may be instances where we want to preload the associations for generation of cache keys, or we may want to have all of the associations loaded because the collection is used in other contexts other than the current rendering.

Currently there is no API to support this, but it is possible to acheive the load behaviour by calling load on the association before it is passed to the render call.

I am looking for feedback on:

Is preloading associations only for models that miss the cache reasonable default behaviour? Or should this be explicitly opted into when calling render?
If it is default behaviour, is calling load on the relation before rendering a suffient API for opting out, or should an additional option to the render call be added.

If it is ok to be default behaviour then I assume that I would need to add some application config to control this behavour to prevent breaking existing application and to provide deprication warnings.

I am also not sure about the structure of this code is it is very tightly connected to both ActionView and ActiveRecord, so any feedback in that area is welcome.

rails-bot · 2017-11-28T02:42:34Z

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

matthewd · 2017-11-28T03:05:24Z

Wow, this is super neat!

The tight coupling does seem less than ideal, but the basic principle of highlighting the subset of records that are "active" and only applying the preload to those is great!

rafaelfranca

We need to fix a lot of rubocop violations

rafaelfranca · 2017-11-28T05:10:33Z

activerecord/lib/active_record/relation.rb

can we make the arguments always required?

rafaelfranca · 2017-11-28T05:11:30Z

activerecord/lib/active_record/railtie.rb

collection_cache_association_loading.

rafaelfranca · 2017-11-28T05:16:17Z

I think the coupling is fine at this level. Of course, it would be better if we can find a way to avoid it, but if not we are using the right mechanisms and it is similar to what we are doing with the controller runtime logs.

lsylvester · 2017-11-28T23:00:09Z

@rafaelfranca The rubocop violations have been fixed and other suggestions implemented.

kaspth · 2018-03-15T19:49:26Z

Thanks!

…ion-error-with-http-caching-and-collection-caching" This reverts commit 8b0cdbe. Reason: #31250 is not backported to 5-2-stable, and this change broke build. https://travis-ci.org/rails/rails/builds/365396137

…r-with-http-caching-and-collection-caching Fix ActiveRecord::ImmutableRelation is raised when collection caching and HTTP caching are used together

ghiculescu · 2022-03-04T17:31:31Z

This PR creates a potential problem if you are using a custom cache key for the cached: option. For example:

@view.render partial: "test/partial", collection: @post, cached: proc { |post| [post, post.author] }

In this case it might not sense to use Russian-doll caching, as in, you may not want to update every single post when an author is changed. Instead you can use post.author in your cache key to ensure that the view is updated correctly if the author changes.

But if you do this you get an n+1 on post.author if you do @posts = Post.preload(:author) in your controller. As a workaround you can do @posts = Post.preload(:author).to_a, but that's a bit surprising and it's easy to forget to do.

I think this PR is great if you do cached: true, but is problematic if you do cached: proc {}. @kaspth would you accept a PR that modifies it so this logic only applies when doing cached: true?

eileencodes · 2022-03-04T17:38:02Z

@ghiculescu can you open a new issue with a reproduction? Or a PR with a repro that fixes this? Otherwise the notification from this will get lost and forgotten.

ghiculescu · 2022-03-04T17:55:45Z

@eileencodes I haven't tested it thoroughly but this is what I had in mind: #44616

rails#31250 optimised rendering a collection with `cached: true`, but also with `cached: proc {}`. The problem is the proc may reference associations that should be preloaded to avoid n+1s in generating the cache key. For example: ```ruby @posts = Post.preload(:author) @view.render partial: "test/partial", collection: @post, cached: proc { |post| [post, post.author] } ``` This will n+1 on `post.author`. To fix this, this PR will always preload the collection if there's a proc given for `cached`. `cached: true` will still not preload unless it renders, as it did in rails#31250.

ghiculescu · 2022-05-07T12:20:21Z

@eileencodes I haven't tested it thoroughly but this is what I had in mind: #44616

Took a bit longer than I planned, but this is now a green build and should be a fix for the issue 👍

rails-bot assigned eileencodes Nov 28, 2017

rafaelfranca requested changes Nov 28, 2017

View reviewed changes

lsylvester force-pushed the only-preload-misses-on-multifetch-cache branch 3 times, most recently from 1309daa to 5f1e76f Compare November 28, 2017 05:44

Only preload misses on multifetch cache

d04a32f

lsylvester force-pushed the only-preload-misses-on-multifetch-cache branch from 5f1e76f to d04a32f Compare March 6, 2018 03:11

kaspth merged commit 0085380 into rails:master Mar 15, 2018

lsylvester mentioned this pull request Mar 19, 2018

Add delayed preloading for arrays of records #32136

Closed

2 tasks

georgeclaghorn mentioned this pull request Apr 11, 2018

ActiveRecord::ImmutableRelation is raised when collection caching and HTTP caching are used together #32534

Closed

y-yagi referenced this pull request Apr 12, 2018

Merge pull request #32536 from lsylvester/fix-immutable-relation-erro…

8b0cdbe

…r-with-http-caching-and-collection-caching Fix ActiveRecord::ImmutableRelation is raised when collection caching and HTTP caching are used together

ghiculescu mentioned this pull request Mar 4, 2022

Always preload if using proc with multifetch cache #44616

Merged

lsylvester mentioned this pull request Jul 27, 2022

Allows associations to be automatically preloadable. #45231

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only preload misses on multifetch cache#31250

Only preload misses on multifetch cache#31250
kaspth merged 1 commit intorails:masterfrom
lsylvester:only-preload-misses-on-multifetch-cache

lsylvester commented Nov 28, 2017

Uh oh!

rails-bot commented Nov 28, 2017

Uh oh!

matthewd commented Nov 28, 2017

Uh oh!

rafaelfranca left a comment

Uh oh!

rafaelfranca Nov 28, 2017

Uh oh!

rafaelfranca Nov 28, 2017

Uh oh!

rafaelfranca commented Nov 28, 2017

Uh oh!

lsylvester commented Nov 28, 2017

Uh oh!

kaspth commented Mar 15, 2018

Uh oh!

ghiculescu commented Mar 4, 2022 •

edited

Loading

Uh oh!

eileencodes commented Mar 4, 2022

Uh oh!

ghiculescu commented Mar 4, 2022

Uh oh!

ghiculescu commented May 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

lsylvester commented Nov 28, 2017

Summary

Concerns and Further Work

Uh oh!

rails-bot commented Nov 28, 2017

Uh oh!

matthewd commented Nov 28, 2017

Uh oh!

rafaelfranca left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelfranca Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelfranca Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

rafaelfranca commented Nov 28, 2017

Uh oh!

lsylvester commented Nov 28, 2017

Uh oh!

kaspth commented Mar 15, 2018

Uh oh!

ghiculescu commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eileencodes commented Mar 4, 2022

Uh oh!

ghiculescu commented Mar 4, 2022

Uh oh!

ghiculescu commented May 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ghiculescu commented Mar 4, 2022 •

edited

Loading