Merge multi_fetch_fragments.

[kaspth]

Fixes #18900.

Makes caching a collection of template partials faster using read_multi
on the Rails cache store.

Some caching implementations have optimized read_multi so we don't have
to check in the cache store for every template.

@dhh, I will be tackling automatic caching next. This is just so my approach and choices, so far, can be reviewed.

[dhh]

Seems like a legit approach to me 👍. Much smoother implementation than the original as well.

[kaspth]

Glad to hear it 👍

Given your thoughts on having the caching be automatic, do you want to keep the support for a custom cache key?

<%= render partial: 'item', collection: @items, cache: ->(item) { [item, 'show'] } %>

[dhh]

Yeah, I think that case still makes sense. But when you’re using the custom cache key like this, it should ignore the specific <% cache %> calls within the partials.

I think using a custom cache key is going to be the minority case, so focusing on the default case first makes sense.

On Feb 15, 2015, at 1:07 PM, Kasper Timm Hansen notifications@github.com wrote:

Glad to hear it

Given your thoughts on having the caching be automatic, do you want to keep the support for a custom cache key?

<%= render partial: 'item', collection: @Items, cache: ->(item) { [item, 'show'] } %>
—
Reply to this email directly or view it on GitHub #18948 (comment).

[kaspth]

Got it 👍

[kaspth]

The Railties tests fail because PartialRenderer doesn't respond to collection_cache= here. But I don't understand why it doesn't.

PartialRenderer is set to be autoloaded, so this reference should require CollectionCaching and include it into the renderer. I'm all ears if anybody has any ideas.

[georgeclaghorn]

You've declared collection_cache to be a mattr_reader. There doesn't seem to be a corresponding writer.

[kaspth]

Do'h! Thanks 😄

Kasper

Den 16/02/2015 kl. 07.04 skrev George Claghorn notifications@github.com:

In actionview/lib/action_view/railtie.rb:

@@ -33,6 +33,8 @@ class Railtie < Rails::Railtie # :nodoc:
if app.config.action_view.cache_template_loading.nil?
ActionView::Resolver.caching = app.config.cache_classes
end
+

•    PartialRenderer.collection_cache = Rails.cache
  

  You've declared collection_cache to be a mattr_reader. There doesn't seem to be a corresponding writer.

—
Reply to this email directly or view it on GitHub.

[kaspth]

I ditched the initializer and used Rails.try(:cache) in the collection_cache definition. I still changed it to a mattr_accessor, so Action View users outside of Rails can switch cache store if the in memory one isn't a fit.

[georgeclaghorn]

Awesome, @kaspth!

[kaspth]

@georgeclaghorn Thanks for the review 😄

@dhh I've pushed a commit that does a multi_read even if cache: true isn't passed to the collection and the template starts with a cache call with a name that matches the as option or what we infer it to be.

How close is this to what you want?

[dhh]

@kaspth So when we hit a render collection call, you look in the partial to see if there's a <% cache call, right? Because we don't want all render collection to obviously cache unless the partials have that call. If that's the way it is, then that sounds like the way it should be!

And thanks for getting this working so quickly. Very excited to try this in my latest app!

[kaspth]

Yes, it will only look in the cache if there's a cache call. Check these lines: https://github.com/kaspth/rails/blob/automatic-collection-caching/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb#L30-L40

[morgoth]

Maybe this could be set in railtie? Not sure if there is a Rails defined if actionview is used standalone.

[kaspth]

You're right. I had it as a railtie first, but then I switched to this. I should have just checked the console:

kaspth:action view kasperhansen$ irb -raction_view
irb(main):001:0> Rails
NameError: uninitialized constant Rails

[simi]

What about to move Rails.cache to ActiveSupport.cache (and all configs and stuff) and just delegate it from Rails.cache there? So this line will change to ActiveSupport.cache and default storage will be chosen there?

Does it make sense? Would be this PR accepted?

[dhh]

I don’t think we should be hanging off globals in ActiveSupport. Rails is the global context for this stuff.

On Feb 16, 2015, at 11:22, Josef Šimánek notifications@github.com wrote:

In actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb:

@@ -0,0 +1,74 @@
+require 'active_support/core_ext/object/try'
+
+module ActionView

• module CollectionCaching
• extend ActiveSupport::Concern

• included do

•  # Use Rails.cache or the fallback cache store if Action View is used without Rails.
  

•  mattr_accessor(:collection_cache) do
  

•    Rails.try(:cache) || ActiveSupport::Cache::MemoryStore.new
  

What about to move Rails.cache to ActiveSupport.cache (and all configs and stuff) and just delegate it from Rails.cache there? So this line will change to ActiveSupport.cache and default storage will be chosen there?

Does it make sense? Would be this PR accepted?

—
Reply to this email directly or view it on GitHub.

[kaspth]

The documentation says virtual_path isn't set for inline templates: https://github.com/rails/rails/blob/master/actionview/lib/action_view/template.rb#L162.

Can someone smarter than me say if an inline template would get this far in the eligibility logic?

[jeremy]

Collection rendering is for partials only, so a render inline: … won't ever be here.

[kaspth]

Great 👍

[kaspth]

@georgeclaghorn I guess I was illegible 😎

@dhh you had another concern on the original issue:

The one improvement I'd like to see is that the partial being rendered should still be able to have it's own caching call that's used when the partial is referred to on it's own, but this call obviously needs to be disregarded when rendered as part of a multi_get.

What do you mean by disregarded?
Should we try and extract the inner template content from the cache call?
Or that the collection renders the template as is - so it's cached independently - but stores it under a key the collection itself generates?

If it's the last scenario, then we're doing it. As far as I can understand my own code 😅

[dhh]

It should do the former. This setup should work with only 1 cache:

# index.html.erb
<%= render @notifications %>

# In the following case, the render collection call detects a caching match, 
# and will only cache things ONCE. 
# notifications/_notification.html.erb
<% cache notification do %>
 Something something
<% end %>

# In the following case, the render collection call detects a caching MISMATCH, 
# and will not attempt to do any caching whatsoever.
# notifications/_notification.html.erb
<% cache notification.event do %>
 Something something
<% end %>

The partial with the cache call has to match what we expect from the render collection cache. If it does not, no caching. Because otherwise the render collection cache would make a bunch of caches that don't expire on the right things. In the example above, notification.event might change sooner than notification, but if render collection just caches on notification, it won't invalidate.

Makes sense?

[kaspth]

Right. I think it actually does work like that.

We do check if the cache call matches the name (i.e. notification and not notification.event). So as long as we just generate the same key as cache notification then we can read from the cache.

Is that what you meant by Just Work in the original issue?

[dhh]

Yes, cools. Then we’re on the same page 👍

On Feb 16, 2015, at 14:01, Kasper Timm Hansen notifications@github.com wrote:

Right. I think it actually does work like that.

We do check if the cache call matches the name (i.e. notification and not notification.event). So as long as we just generate the same key as cache notification then we can read from the cache.

Is that what you meant by Just Work in the original issue?

—
Reply to this email directly or view it on GitHub.

[kaspth]

@dhh phew! This cache stuff screws with your head. I'll add a test to verify this 👍

[dhh]

Haha, yeah, it’s a lot of balls to keep in the air at the same time! We’re deducing a lot of stuff here. Both the caching fit and the caching key that mixes in the partial digest.

On Feb 16, 2015, at 14:06, Kasper Timm Hansen notifications@github.com wrote:

@dhh phew! This cache stuff screws with your head. I'll add a test to verify this

—
Reply to this email directly or view it on GitHub.

[jeremy]

I'd expect this to use config.action_controller.cache_store (ActionController::Base.cache_store) since that's already used for fragment caching, and falls back to Rails.cache if unconfigured.

[kaspth]

👍

[dubek]

So I used the new tests from #20284 and then tried to tackle the resource_cache_call_pattern regex. Generally trying to parsing Ruby code with regex is probably not the correct thing to do, but not sure there is other easy approach for this task here.

The following contents for _commented_customer.html.erb break the tests (which means - they are not caught by the regex):

1. Two comments at top:

   <%# I'm a comment %>
   <%# I'm a comment too %>
   <% cache customer do %>
     <%= customer.name %>, <%= customer.id %>
   <% end %>
   

2. Multi-line comment at top:

   <%# I'm a multi-line
       comment %>
   <% cache customer do %>
     <%= customer.name %>, <%= customer.id %>
   <% end %>
   

3. Spaces in front of <% cache customer do %> (they are not emitted to the output generated by Erubis):

   <%# I'm a comment %>
     <% cache customer do %>
     <%= customer.name %>, <%= customer.id %>
     <% end %>
   

4. Extra spaces between <% and cache:

   <%# I'm a comment %>
   <%      cache customer do %>
     <%= customer.name %>, <%= customer.id %>
   <% end %>
   

5. This is probably accidentally caught by the regex, even though it shouldn't ($1 will be _is_full):

   <% cache_is_full = true %>
   

6. The example in the comment says that <% cache notification.event do %> should fail to a extract, but it actually matches and sets $1 to notification..

Many of these are not idiomatic Ruby style; but still it's very strange if caching works OK when you have one space in <% cache ... but stops working (without any notice) when you have two spaces there. You wouldn't expect any performance changes in Ruby programs when you change whitespace in the program.

For 1-4 above you can run erubis -x test/fixtures/customers/_commented_customer.html.erb and see that the output is an identical Ruby program (say if you parse it with Ripper or something like that).

[kaspth]

Sweet, you've found some bugs! Rather than burden this already closed pull request, can you open a new issue or if you've beat the regex (or want to do it) can you open a pull request? 😄

We should add more tests for Action View Template's eligible_for_collection_caching?

[schneems]

Is this behavior mentioned in the guides?

[prathamesh-sonpatki]

@schneems Yes. It's here http://edgeguides.rubyonrails.org/caching_with_rails.html#collection-caching

[ghiculescu]

Not sure if this should be in a new issue or if a comment here is fine. I recently came across an issue using https://github.com/n8/multi_fetch_fragments on a long list of complicated partials. It turns out the computation of digests for the partial was being repeated n times and thus was taking longer than the time saved by this technique. Specifically, calculating the digest took 13ms, and was being repeated for 300 partials, which was resulting in 4 seconds of overhead before even reading from memcached.

ghiculescu/multi_fetch_fragments@59612d5 is my fix for it. It's a pretty simplistic fix - just disable Rails' digesting if we are providing our own cache keys - but it serves my needs.

I think it would be good to add a similar sort of option to pass through to https://github.com/rails/rails/pull/18948/files#diff-738cc3c10058587e78559e48066ad0bfR54 since otherwise similar issues will come up for other people.

@kaspth does this make sense? Happy to log this somewhere else if you prefer.

edit: just came across n8/multi_fetch_fragments#31 so I'm not the only one having this issue.

[kaspth]

@ghiculescu hey 👋

Rails 5 has got you covered. It keeps a template digest cached throughout a request in development (they are expired when the request is finished, so reloading is still possible). See #20361 and my attached pull request for more.

[ghiculescu]

It's an issue in production too @kaspth - though apparently fixed in rails 5 as well #14645 (comment)

[kaspth]

@ghiculescu prepared SQL statements are an entirely different feature altogether from template digests. Is there something I'm not getting here?

[mickey]

Any idea why write_multi is not used when writing the non cached renders?

[schneems]

Any idea why write_multi is not used when writing the non cached renders?

I don't know. I haven't looked into it. However I could see a problem if you were creating many many cache items, say hundreds...to keep them all in memory before trying to write them all at the same time. So maybe it would be better to have that be configurable instead of a default.

Either way if you want to add it feel free to submit a PR and we can talk more there.

[mickey]

@schneems Makes total sense thanks for the explanation. I'll take a look.

[kaspth]

@mickey you mean other than there being no write_multi for ActiveSupport::Cache as far as my google skills can find? 😁

But yeah, do try to implement write_multi and make it work here!

[mickey]

@kaspth Yes sorry about that. I discovered a little earlier that write_multi is a specificity of a redis backend we use.

[aried3r]

@schneems Yes. It's here http://edgeguides.rubyonrails.org/caching_with_rails.html#collection-caching

Isn't this incomplete since 11644fd which made it automatic if the partials are set up just right?

[kaspth]

Nope, check master that has the latest implementation.

[aried3r]

Got it, found b4558c1. For some reason I was browsing your fork instead of rails/rails. ¯\_(ツ)_/¯

[kaspth]

@aried3r that's the one, yeah. 👍