Merge multi_fetch_fragments. #18948

Merged
merged 3 commits into from Feb 25, 2015

Conversation

Projects
None yet
@kaspth
Member

kaspth commented Feb 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 15, 2015

Member

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

Member

dhh commented Feb 15, 2015

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

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 15, 2015

Member

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'] } %>
Member

kaspth commented Feb 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 15, 2015

Member

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).

Member

dhh commented Feb 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 15, 2015

Member

Got it 👍

Member

kaspth commented Feb 15, 2015

Got it 👍

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

This comment has been minimized.

@kaspth

kaspth Feb 15, 2015

Member

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.

@kaspth

kaspth Feb 15, 2015

Member

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.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 16, 2015

Member

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

@georgeclaghorn

georgeclaghorn Feb 16, 2015

Member

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

This comment has been minimized.

@kaspth

kaspth Feb 16, 2015

Member

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

kaspth Feb 16, 2015

Member

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.

This comment has been minimized.

@kaspth

kaspth Feb 16, 2015

Member

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.

@kaspth

kaspth Feb 16, 2015

Member

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.

+
+ def expanded_cache_key(key)
+ caching_controller.fragment_cache_key(@view.cache_fragment_name(key)).tap do |k|
+ return k.dup if k.frozen? # #read_multi & #write may require mutability, Dalli 2.6.0.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 16, 2015

Member

The tap seems unnecessary when we'll always return from inside it:

def expanded_cache_key(key)
  expanded_key = caching_controller.fragment_cache_key(@view.cache_fragment_name(key))
  expanded_key.dup if expanded_key.frozen?
end
@georgeclaghorn

georgeclaghorn Feb 16, 2015

Member

The tap seems unnecessary when we'll always return from inside it:

def expanded_cache_key(key)
  expanded_key = caching_controller.fragment_cache_key(@view.cache_fragment_name(key))
  expanded_key.dup if expanded_key.frozen?
end

This comment has been minimized.

@gsamokovarov

gsamokovarov Feb 16, 2015

Contributor

This will return nil if the expanded_key isn't frozen.

@gsamokovarov

gsamokovarov Feb 16, 2015

Contributor

This will return nil if the expanded_key isn't frozen.

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Feb 16, 2015

Member

Whoops, right.

@georgeclaghorn

georgeclaghorn Feb 16, 2015

Member

Whoops, right.

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Why does Dalli mutate the key?

@jeremy

jeremy Feb 17, 2015

Member

Why does Dalli mutate the key?

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Agree that doing an explicit return within a #tap is confusing, since tap is designed to do just the opposite: return the receiver. Clearer to take its return value and do an explicit conditional: v = …; v.frozen? ? v.dup : v

@jeremy

jeremy Feb 17, 2015

Member

Agree that doing an explicit return within a #tap is confusing, since tap is designed to do just the opposite: return the receiver. Clearer to take its return value and do an explicit conditional: v = …; v.frozen? ? v.dup : v

This comment has been minimized.

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

It's duping the key before forcing encoding, though

@jeremy

jeremy Feb 17, 2015

Member

It's duping the key before forcing encoding, though

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

Are you saying we should remove our dup here?

Kasper

Den 17/02/2015 kl. 21.15 skrev Jeremy Kemper notifications@github.com:

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

  •  end
    
  • end
  • def cache_collection?
  •  ActionController::Base.perform_caching && (@options[:cache].present? || @locals[:cache].present?)
    
  • end
  • def collection_by_cache_keys
  •  seed = @options[:cache].respond_to?(:call) ? @options[:cache] : ->(i) { i }
    
  •  Hash[ @collection.map { |item| [expanded_cache_key(seed.call(item)), item] } ]
    
  • end
  • def expanded_cache_key(key)
  •  caching_controller.fragment_cache_key(@view.cache_fragment_name(key)).tap do |k|
    
  •    return k.dup if k.frozen? # #read_multi & #write may require mutability, Dalli 2.6.0.
    
    It's duping the key before forcing encoding, though


Reply to this email directly or view it on GitHub.

@kaspth

kaspth Feb 17, 2015

Member

Are you saying we should remove our dup here?

Kasper

Den 17/02/2015 kl. 21.15 skrev Jeremy Kemper notifications@github.com:

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

  •  end
    
  • end
  • def cache_collection?
  •  ActionController::Base.perform_caching && (@options[:cache].present? || @locals[:cache].present?)
    
  • end
  • def collection_by_cache_keys
  •  seed = @options[:cache].respond_to?(:call) ? @options[:cache] : ->(i) { i }
    
  •  Hash[ @collection.map { |item| [expanded_cache_key(seed.call(item)), item] } ]
    
  • end
  • def expanded_cache_key(key)
  •  caching_controller.fragment_cache_key(@view.cache_fragment_name(key)).tap do |k|
    
  •    return k.dup if k.frozen? # #read_multi & #write may require mutability, Dalli 2.6.0.
    
    It's duping the key before forcing encoding, though


Reply to this email directly or view it on GitHub.

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
Member

georgeclaghorn commented Feb 16, 2015

Awesome, @kaspth!

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 16, 2015

Member

@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?

Member

kaspth commented Feb 16, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 16, 2015

Member

@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!

Member

dhh commented Feb 16, 2015

@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

This comment has been minimized.

Show comment
Hide comment
+ 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

This comment has been minimized.

@morgoth

morgoth Feb 16, 2015

Member

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

@morgoth

morgoth Feb 16, 2015

Member

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

This comment has been minimized.

@kaspth

kaspth Feb 16, 2015

Member

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
@kaspth

kaspth Feb 16, 2015

Member

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

This comment has been minimized.

@simi

simi Feb 16, 2015

Contributor

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?

@simi

simi Feb 16, 2015

Contributor

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?

This comment has been minimized.

@dhh

dhh Feb 16, 2015

Member

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.

@dhh

dhh Feb 16, 2015

Member

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 kaspth added the actionview label Feb 16, 2015

@kaspth kaspth added this to the 5.0.0 milestone Feb 16, 2015

@kaspth kaspth self-assigned this Feb 16, 2015

+ end
+
+ def inferred_cache_name
+ @options[:as] || @template.virtual_path.split('/').last.sub('_', '')

This comment has been minimized.

@kaspth

kaspth Feb 16, 2015

Member

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?

@kaspth

kaspth Feb 16, 2015

Member

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?

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

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

@jeremy

jeremy Feb 17, 2015

Member

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

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

Great 👍

@kaspth

kaspth Feb 17, 2015

Member

Great 👍

+ end
+
+ def collection_only_renders_one_template?
+ @template # Template is only set when a collection renders one template.

This comment has been minimized.

@kaspth

kaspth Feb 16, 2015

Member

@dhh When you render a collection, do you render have it render different templates? Say a todo list renders todo and amazing_todo?

If so, we can't use read_multi there.

Because @template is only set if there's a @path and @path is only set if the collection renders a single template.

Eventually collection_without_template is called and we don't have access to where it finds templates.

We could try and patch it in there, but then we'd fetch the templates twice. I'll admit I don't know how costly that is. But I'd rather not 😄

@kaspth

kaspth Feb 16, 2015

Member

@dhh When you render a collection, do you render have it render different templates? Say a todo list renders todo and amazing_todo?

If so, we can't use read_multi there.

Because @template is only set if there's a @path and @path is only set if the collection renders a single template.

Eventually collection_without_template is called and we don't have access to where it finds templates.

We could try and patch it in there, but then we'd fetch the templates twice. I'll admit I don't know how costly that is. But I'd rather not 😄

This comment has been minimized.

@dhh

dhh Feb 16, 2015

Member

It'd be awesome if it would use read_multi PER template type was being fetched, but that can be a second step. So say you have 5 amazing todos and 3 regular todos -- that'd be 2 read_multi's. One per type.

@dhh

dhh Feb 16, 2015

Member

It'd be awesome if it would use read_multi PER template type was being fetched, but that can be a second step. So say you have 5 amazing todos and 3 regular todos -- that'd be 2 read_multi's. One per type.

This comment has been minimized.

@dhh

dhh Feb 16, 2015

Member

Which actually leads me to a separate point. <%= render @collection_with_different_types %> is not something we currently have a way of doing by hand, if your partials don't map to the RecordIdentifier lookup. Need a way to do that.

@dhh

dhh Feb 16, 2015

Member

Which actually leads me to a separate point. <%= render @collection_with_different_types %> is not something we currently have a way of doing by hand, if your partials don't map to the RecordIdentifier lookup. Need a way to do that.

This comment has been minimized.

@kaspth

kaspth Feb 16, 2015

Member

Your last comment is a little too inside baseball for me 😄 I do recognize something about an identifier, but I've only looked at Action View for two days so it's pretty fuzzy.

@kaspth

kaspth Feb 16, 2015

Member

Your last comment is a little too inside baseball for me 😄 I do recognize something about an identifier, but I've only looked at Action View for two days so it's pretty fuzzy.

This comment has been minimized.

@dhh

dhh Feb 16, 2015

Member

Haha, I'll explain. The template system includes a digest of the template file in all <% cache %> calls. This makes it such that if you change the template file, it automatically expires all the caches based on that template digest. The digest system does dependency tracking as well. So for example, a call to <%= render @notifications %> inside an index.html.erb file make make it such that any cache calls in that file depend on the notifications/_notification digest as well. It's a full dependency tree.

@dhh

dhh Feb 16, 2015

Member

Haha, I'll explain. The template system includes a digest of the template file in all <% cache %> calls. This makes it such that if you change the template file, it automatically expires all the caches based on that template digest. The digest system does dependency tracking as well. So for example, a call to <%= render @notifications %> inside an index.html.erb file make make it such that any cache calls in that file depend on the notifications/_notification digest as well. It's a full dependency tree.

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

Thanks for the explanation 👍 I've looked in the cache helper and it's fairly readable.

@kaspth

kaspth Feb 17, 2015

Member

Thanks for the explanation 👍 I've looked in the cache helper and it's fairly readable.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 16, 2015

Member

@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 😅

Member

kaspth commented Feb 16, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 16, 2015

Member

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?

Member

dhh commented Feb 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 16, 2015

Member

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?

Member

kaspth commented Feb 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 16, 2015

Member

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.

Member

dhh commented Feb 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 16, 2015

Member

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

Member

kaspth commented Feb 16, 2015

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

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 16, 2015

Member

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.

Member

dhh commented Feb 16, 2015

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.

actionview/lib/action_view/railtie.rb
@@ -34,6 +34,8 @@ class Railtie < Rails::Railtie # :nodoc:
ActionView::Resolver.caching = app.config.cache_classes
end
end
+
+ PartialRenderer.collection_cache = Rails.cache

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

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.

@jeremy

jeremy Feb 17, 2015

Member

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.

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

👍

+ end
+ end
+
+ def automatic_cache_eligable?

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

s/eligable/eligible

@jeremy

jeremy Feb 17, 2015

Member

s/eligable/eligible

+ def cache_collection?
+ if ActionController::Base.perform_caching
+ @options[:cache].present? || automatic_cache_eligable?
+ end

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

If we aren't checking for cache: '' or cache: [] values, we can rely on truthiness alone:

def cache_collection?
  ActionController::Base.perform_caching && @options.fetch(:cache, automatic_cache_eligible?)
end
@jeremy

jeremy Feb 17, 2015

Member

If we aren't checking for cache: '' or cache: [] values, we can rely on truthiness alone:

def cache_collection?
  ActionController::Base.perform_caching && @options.fetch(:cache, automatic_cache_eligible?)
end

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

The original implementation would turn caching off with cache: false. Is there a better way to express that?

@kaspth

kaspth Feb 17, 2015

Member

The original implementation would turn caching off with cache: false. Is there a better way to express that?

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

cache: false will result in @options.fetch(:cache, default_value) returning false, so cache_collection? will return false.

@jeremy

jeremy Feb 17, 2015

Member

cache: false will result in @options.fetch(:cache, default_value) returning false, so cache_collection? will return false.

+
+ def automatic_cache_eligable?
+ if collection_only_renders_one_template? && !custom_collection_cache_key?
+ @template.source =~ /\A<% cache\(?\s*(\w+)/ && $1 == inferred_cache_name.to_s

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Can we push this check down into templates and memoize it to avoid repeated checks? Then template handlers can provide their own (possibly non-ERb) decision-making about hoisting partial caches up to the collection.

@jeremy

jeremy Feb 17, 2015

Member

Can we push this check down into templates and memoize it to avoid repeated checks? Then template handlers can provide their own (possibly non-ERb) decision-making about hoisting partial caches up to the collection.

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

Good call 👍

I just found it can't be in here anyway. When a template is rendered once, it's source will be set to nil, so our check will fail.

Adding eligible_for_automatic_collection_caching? to templates shortly.

@kaspth

kaspth Feb 17, 2015

Member

Good call 👍

I just found it can't be in here anyway. When a template is rendered once, it's source will be set to nil, so our check will fail.

Adding eligible_for_automatic_collection_caching? to templates shortly.

+ rendered_partials = @collection.any? ? yield : []
+
+ fetch_or_cache_partial(partial_cache, order_by: keyed_collection.each_key) do
+ rendered_partials.shift

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

This mutates the return value of PartialRenderer#collection_with/without_template, exposing us to regression if those methods are modified (for example, memoizing their result) in the future.

@jeremy

jeremy Feb 17, 2015

Member

This mutates the return value of PartialRenderer#collection_with/without_template, exposing us to regression if those methods are modified (for example, memoizing their result) in the future.

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

I was thinking about that too.

The only idea I could come up with is to the render the extra partials and do a second multi_read and returning the then cached partials.

But then we'd be doing two multi reads whenever there's new templates.

@kaspth

kaspth Feb 17, 2015

Member

I was thinking about that too.

The only idea I could come up with is to the render the extra partials and do a second multi_read and returning the then cached partials.

But then we'd be doing two multi reads whenever there's new templates.

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

We could rendered_partials = @collection.any? ? yield.dup : []

@jeremy

jeremy Feb 17, 2015

Member

We could rendered_partials = @collection.any? ? yield.dup : []

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

I was thinking about this some more. It's a shame Enumerator raises a StopIteration exception, otherwise this would have worked (and we wouldn't have to dup the whole return value):

rendered_partials = @collection.any? ? yield.each : []

fetch_or_cache_partial(partial_cache, order_by: keyed_collection.each_key) do
  rendered_partials.next
end

The weird thing was adding this didn't make any tests blow up, which kind of worries me.

@kaspth

kaspth Feb 18, 2015

Member

I was thinking about this some more. It's a shame Enumerator raises a StopIteration exception, otherwise this would have worked (and we wouldn't have to dup the whole return value):

rendered_partials = @collection.any? ? yield.each : []

fetch_or_cache_partial(partial_cache, order_by: keyed_collection.each_key) do
  rendered_partials.next
end

The weird thing was adding this didn't make any tests blow up, which kind of worries me.

+ def collection_by_cache_keys
+ seed = callable_cache_key? ? @options[:cache] : ->(i) { i }
+
+ Hash[ @collection.map { |item| [expanded_cache_key(seed.call(item)), item] } ]

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

You can use #to_h instead of Hash[ … ] now.

@jeremy

jeremy Feb 17, 2015

Member

You can use #to_h instead of Hash[ … ] now.

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Quicker to build the hash as we go, too:

@collection.each_with_object({}) { |item, hash| hash[expanded_cache_key(seed.call(item))] = item }
@jeremy

jeremy Feb 17, 2015

Member

Quicker to build the hash as we go, too:

@collection.each_with_object({}) { |item, hash| hash[expanded_cache_key(seed.call(item))] = item }
+ end
+
+ def caching_controller
+ defined?(@_controller) ? @_controller : (@_caching_controller ||= ActionController::Base.new)

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Should we memoize the entire result? Will @_controller ever flip between defined/undefined?

@jeremy

jeremy Feb 17, 2015

Member

Should we memoize the entire result? Will @_controller ever flip between defined/undefined?

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

It won't. You're right.

@kaspth

kaspth Feb 17, 2015

Member

It won't. You're right.

+ defined?(@_controller) ? @_controller : (@_caching_controller ||= ActionController::Base.new)
+ end
+
+ def fetch_or_cache_partial(cache, order_by:)

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Would rename the cache argument to reflect that it's cache.read_multi results, not the cache itself.

@jeremy

jeremy Feb 17, 2015

Member

Would rename the cache argument to reflect that it's cache.read_multi results, not the cache itself.

+ cache_options = @options[:cache_options] || @locals[:cache_options] || {}
+
+ order_by.map do |key|
+ cache[key] || yield.tap { |item| collection_cache.write(key, item, cache_options) }

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Can use Hash#fetch to good effect here:

cached_partials.fetch key do
  yield.tap do |rendered_partial|
    collection_cache.write key, rendered_partial, cache_options
  end
end
@jeremy

jeremy Feb 17, 2015

Member

Can use Hash#fetch to good effect here:

cached_partials.fetch key do
  yield.tap do |rendered_partial|
    collection_cache.write key, rendered_partial, cache_options
  end
end

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

Right on. Does it make sense to check for cache_options in both @options or @locals? It was in the original multi_fetch_fragments implementation and I don't have enough experience with this to judge it with.

@kaspth

kaspth Feb 17, 2015

Member

Right on. Does it make sense to check for cache_options in both @options or @locals? It was in the original multi_fetch_fragments implementation and I don't have enough experience with this to judge it with.

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

I'm not sure where those options are coming from - depends on the caller behavior; hard to trace back.

@jeremy

jeremy Feb 17, 2015

Member

I'm not sure where those options are coming from - depends on the caller behavior; hard to trace back.

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

I copied them over from the original implementation and there was no tests for it. I'm guessing it was useful to Nate and his team, but I'd just remove it.

Kasper

Den 17/02/2015 kl. 21.16 skrev Jeremy Kemper notifications@github.com:

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

  • def expanded_cache_key(key)
  •  caching_controller.fragment_cache_key(@view.cache_fragment_name(key)).tap do |k|
    
  •    return k.dup if k.frozen? # #read_multi & #write may require mutability, Dalli 2.6.0.
    
  •  end
    
  • end
  • def caching_controller
  •  defined?(@_controller) ? @_controller : (@_caching_controller ||= ActionController::Base.new)
    
  • end
  • def fetch_or_cache_partial(cache, order_by:)
  •  cache_options = @options[:cache_options] || @locals[:cache_options] || {}
    
  •  order_by.map do |key|
    
  •    cache[key] || yield.tap { |item| collection_cache.write(key, item, cache_options) }
    
    I'm not sure where those options are coming from - depends on the caller behavior; hard to trace back.


Reply to this email directly or view it on GitHub.

@kaspth

kaspth Feb 17, 2015

Member

I copied them over from the original implementation and there was no tests for it. I'm guessing it was useful to Nate and his team, but I'd just remove it.

Kasper

Den 17/02/2015 kl. 21.16 skrev Jeremy Kemper notifications@github.com:

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

  • def expanded_cache_key(key)
  •  caching_controller.fragment_cache_key(@view.cache_fragment_name(key)).tap do |k|
    
  •    return k.dup if k.frozen? # #read_multi & #write may require mutability, Dalli 2.6.0.
    
  •  end
    
  • end
  • def caching_controller
  •  defined?(@_controller) ? @_controller : (@_caching_controller ||= ActionController::Base.new)
    
  • end
  • def fetch_or_cache_partial(cache, order_by:)
  •  cache_options = @options[:cache_options] || @locals[:cache_options] || {}
    
  •  order_by.map do |key|
    
  •    cache[key] || yield.tap { |item| collection_cache.write(key, item, cache_options) }
    
    I'm not sure where those options are coming from - depends on the caller behavior; hard to trace back.


Reply to this email directly or view it on GitHub.

@@ -31,6 +31,10 @@ def errors
def persisted?
id.present?
end
+
+ def cache_key
+ "#{name}"

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

name.to_s?

@jeremy

jeremy Feb 17, 2015

Member

name.to_s?

+
+ def automatic_cache_eligible?
+ if single_template_render? && !callable_cache_key?
+ @template.source =~ /\A<% cache\(?\s*(\w+\.?)/ && $1 == inferred_cache_name.to_s

This comment has been minimized.

@jeremy

jeremy Feb 17, 2015

Member

Partials often begin with <# Comment about partial %> - any way to detect that? It'll be tough to diagnose when/why autocaching works.

@jeremy

jeremy Feb 17, 2015

Member

Partials often begin with <# Comment about partial %> - any way to detect that? It'll be tough to diagnose when/why autocaching works.

This comment has been minimized.

@dhh

dhh Feb 17, 2015

Member

We should log a fair chat about all this. Whether automatic caching is kicking in, etc.

@dhh

dhh Feb 17, 2015

Member

We should log a fair chat about all this. Whether automatic caching is kicking in, etc.

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

I'm looking in to extending the regex to beware of those comments. My current attempt is trying to have an optional capture group something like this /\A(:?<%#.*%>)?<% cache\(?\s*(\w+\.?)/, but it doesn't work yet.

@dhh other places in both template and partial_renderer use instrumentation, should we go with as well or a plain old Rails.logger debug call?

@kaspth

kaspth Feb 17, 2015

Member

I'm looking in to extending the regex to beware of those comments. My current attempt is trying to have an optional capture group something like this /\A(:?<%#.*%>)?<% cache\(?\s*(\w+\.?)/, but it doesn't work yet.

@dhh other places in both template and partial_renderer use instrumentation, should we go with as well or a plain old Rails.logger debug call?

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

Got it! Misremembered turning off capturing for a group as :? instead of ?:. I also added a \n?.

Here are my test cases: https://gist.github.com/kaspth/6ba8ac0356549e219b57. Let me know if you find scenarios I've missed. 😄

@kaspth

kaspth Feb 17, 2015

Member

Got it! Misremembered turning off capturing for a group as :? instead of ?:. I also added a \n?.

Here are my test cases: https://gist.github.com/kaspth/6ba8ac0356549e219b57. Let me know if you find scenarios I've missed. 😄

This comment has been minimized.

@kaspth

kaspth Feb 17, 2015

Member

I've updated the gist trying to annotate the regex, but Ruby says "unmatched close parenthesis". I can't figure out how to get it working in. I'll upload the latest code without annotation now.

@kaspth

kaspth Feb 17, 2015

Member

I've updated the gist trying to annotate the regex, but Ruby says "unmatched close parenthesis". I can't figure out how to get it working in. I'll upload the latest code without annotation now.

+ end
+
+ def cache_collection?
+ ActionController::Base.perform_caching && @options.fetch(:cache, automatic_cache_eligible?)

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2015

Member

I don't think we should be accessing Action Controller here.

@rafaelfranca

rafaelfranca Feb 18, 2015

Member

I don't think we should be accessing Action Controller here.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

Thanks, the cache helper in any template will check this anyway so you're right ❤️

@kaspth

kaspth Feb 18, 2015

Member

Thanks, the cache helper in any template will check this anyway so you're right ❤️

+ end
+
+ def caching_controller
+ @_caching_controller ||= defined?(@_controller) ? @_controller : ActionController::Base.new

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2015

Member

Same here. Not sure if we changed our minds but we wanted to decouple Action View from Action Pack and this is coupling it more.

@rafaelfranca

rafaelfranca Feb 18, 2015

Member

Same here. Not sure if we changed our minds but we wanted to decouple Action View from Action Pack and this is coupling it more.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

👍, we're going to have to extract fragment_cache_key into a separate module included in controllers and views.

Then this would just be @view.fragment_cache_key

@kaspth

kaspth Feb 18, 2015

Member

👍, we're going to have to extract fragment_cache_key into a separate module included in controllers and views.

Then this would just be @view.fragment_cache_key

@@ -0,0 +1,9 @@
+module FragmentCacheKey
+ # Given a key (as described in ActionController::Caching::Fragments.expire_fragment),

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

@rafaelfranca am I referencing this constant right? There's some documentation below that does it like this.

@kaspth

kaspth Feb 18, 2015

Member

@rafaelfranca am I referencing this constant right? There's some documentation below that does it like this.

+ # cached fragment. All keys are prefixed with <tt>views/</tt> and uses
+ # ActiveSupport::Cache.expand_cache_key for the expansion.
+ def fragment_cache_key(key)
+ ActiveSupport::Cache.expand_cache_key(key.is_a?(Hash) ? url_for(key).split("://").last : key, :views)

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2015

Member

Hmm, it doesn't sounds right to have this in Active Support. Can't we duplicate the implementation? Also why it is required in both frameworks?

@rafaelfranca

rafaelfranca Feb 18, 2015

Member

Hmm, it doesn't sounds right to have this in Active Support. Can't we duplicate the implementation? Also why it is required in both frameworks?

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

Sure, I'll dupe it in CollectionCaching instead.

Honestly, I don't know. This should go into Action View where the other caching helpers are.

@kaspth

kaspth Feb 18, 2015

Member

Sure, I'll dupe it in CollectionCaching instead.

Honestly, I don't know. This should go into Action View where the other caching helpers are.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

Whoops, I keep forgetting CollectionCaching is mixed into PartialRenderer and not ActionView::Base. I'll dupe it in ActionView::Base instead.

@kaspth

kaspth Feb 18, 2015

Member

Whoops, I keep forgetting CollectionCaching is mixed into PartialRenderer and not ActionView::Base. I'll dupe it in ActionView::Base instead.

+ def template_eligible?
+ @template.eligible_for_collection_caching?(as: @options[:as]).tap do
+ ActionView::Base.logger.try :debug,
+ "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

@dhh @jeremy is this an appropriate amount of logging to you?

We could potentially add conditional logging to explain why automatic caching didn't kick in.
Did the collection render several templates? Did you pass a callable cache key? And was or wasn't the template eligible?

We'd probably need to soup the logging code up more though. I'm not exactly happy with soldering it into each of the methods, but it works.

@kaspth

kaspth Feb 18, 2015

Member

@dhh @jeremy is this an appropriate amount of logging to you?

We could potentially add conditional logging to explain why automatic caching didn't kick in.
Did the collection render several templates? Did you pass a callable cache key? And was or wasn't the template eligible?

We'd probably need to soup the logging code up more though. I'm not exactly happy with soldering it into each of the methods, but it works.

This comment has been minimized.

@dhh

dhh Feb 18, 2015

Member

Hmm, I wonder if it won't actually be enough to rely on the multi_get render call since we'll see lines like "Read fragment views/recordings/431-6/dc02965b56829c216cdad6b48ecfaabc". Not sure what the multi_get lines logs.

@dhh

dhh Feb 18, 2015

Member

Hmm, I wonder if it won't actually be enough to rely on the multi_get render call since we'll see lines like "Read fragment views/recordings/431-6/dc02965b56829c216cdad6b48ecfaabc". Not sure what the multi_get lines logs.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

I did remember seeing lines containing the fragment, but I don't think they were from read_multi. I'll recheck tomorrow.

I'm also logging the count of partials read from and written to cache. But I don't know how useful that is.

Sorry about this taking so long :)

Kasper

Den 18/02/2015 kl. 22.06 skrev David Heinemeier Hansson notifications@github.com:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    
    Hmm, I wonder if it won't actually be enough to rely on the multi_get render call since we'll see lines like "Read fragment views/recordings/431-6/dc02965b56829c216cdad6b48ecfaabc". Not sure what the multi_get lines logs.


Reply to this email directly or view it on GitHub.

@kaspth

kaspth Feb 18, 2015

Member

I did remember seeing lines containing the fragment, but I don't think they were from read_multi. I'll recheck tomorrow.

I'm also logging the count of partials read from and written to cache. But I don't know how useful that is.

Sorry about this taking so long :)

Kasper

Den 18/02/2015 kl. 22.06 skrev David Heinemeier Hansson notifications@github.com:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    
    Hmm, I wonder if it won't actually be enough to rely on the multi_get render call since we'll see lines like "Read fragment views/recordings/431-6/dc02965b56829c216cdad6b48ecfaabc". Not sure what the multi_get lines logs.


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@dhh

dhh Feb 18, 2015

Member

No worries at all. Let’s get it right. Happy to also just get the main logic going, and then working on what the optimal logging mix is second.

On Feb 18, 2015, at 13:20, Kasper Timm Hansen notifications@github.com wrote:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    

I did remember seeing lines containing the fragment, but I don't think they were from read_multi. I'll recheck tomorrow. I'm also logging the count of partials read from and written to cache. But I don't know how useful that is. Sorry about this taking so long :)


Reply to this email directly or view it on GitHub.

@dhh

dhh Feb 18, 2015

Member

No worries at all. Let’s get it right. Happy to also just get the main logic going, and then working on what the optimal logging mix is second.

On Feb 18, 2015, at 13:20, Kasper Timm Hansen notifications@github.com wrote:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    

I did remember seeing lines containing the fragment, but I don't think they were from read_multi. I'll recheck tomorrow. I'm also logging the count of partials read from and written to cache. But I don't know how useful that is. Sorry about this taking so long :)


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2015

Member

👍 on splitting it out. I can scoop out the logging and you can get it in your app. Let's extract follow up issues from what you are missing. Feel free to assign me to those.

Kasper

Den 18/02/2015 kl. 22.35 skrev David Heinemeier Hansson notifications@github.com:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    
    No worries at all. Let’s get it right. Happy to also just get the main logic going, and then working on what the optimal logging mix is second.


    Reply to this email directly or view it on GitHub.
@kaspth

kaspth Feb 18, 2015

Member

👍 on splitting it out. I can scoop out the logging and you can get it in your app. Let's extract follow up issues from what you are missing. Feel free to assign me to those.

Kasper

Den 18/02/2015 kl. 22.35 skrev David Heinemeier Hansson notifications@github.com:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    
    No worries at all. Let’s get it right. Happy to also just get the main logic going, and then working on what the optimal logging mix is second.


    Reply to this email directly or view it on GitHub.

This comment has been minimized.

@kaspth

kaspth Feb 19, 2015

Member

Okay, I've been looking more into logging and captured this from the logging test:

Collection automatically caches customers/_customer, so next render uses a multi read for any templates rendered now.
Cache digest for /Users/kasperhansen/Documents/code/rails/actionpack/test/fixtures/customers/_customer.html.erb: 4184ab71db6849621a4d8820fcd2c0ad
Cache digest for /Users/kasperhansen/Documents/code/rails/actionpack/test/fixtures/collection_cache/index.html.erb: 8f52a1ba24922b018905574fbb082059
Cache digest for /Users/kasperhansen/Documents/code/rails/actionpack/test/fixtures/customers/_customer.html.erb: 4184ab71db6849621a4d8820fcd2c0ad
Cache read: views/david/1/4184ab71db6849621a4d8820fcd2c0ad
Cache write: views/david/1/4184ab71db6849621a4d8820fcd2c0ad
1 partials will be written to cache.
Cache write: views/david/1/8f52a1ba24922b018905574fbb082059

Collection automatically caches customers/_customer, so next render uses a multi read for any templates rendered now.
1 partials multi read from cache.
Cache read: views/david/2/4184ab71db6849621a4d8820fcd2c0ad
Cache write: views/david/2/4184ab71db6849621a4d8820fcd2c0ad
Cache read: views/david/3/4184ab71db6849621a4d8820fcd2c0ad
Cache write: views/david/3/4184ab71db6849621a4d8820fcd2c0ad
2 partials will be written to cache.
Cache write: views/david/2/8f52a1ba24922b018905574fbb082059
Cache write: views/david/3/8f52a1ba24922b018905574fbb082059

Collection automatically caches customers/_customer, so next render uses a multi read for any templates rendered now.
3 partials multi read from cache.

It seems the cache store doesn't log when using multi read. But the "3 partials multi read from cache." lacks information to be really useful, I think.

@kaspth

kaspth Feb 19, 2015

Member

Okay, I've been looking more into logging and captured this from the logging test:

Collection automatically caches customers/_customer, so next render uses a multi read for any templates rendered now.
Cache digest for /Users/kasperhansen/Documents/code/rails/actionpack/test/fixtures/customers/_customer.html.erb: 4184ab71db6849621a4d8820fcd2c0ad
Cache digest for /Users/kasperhansen/Documents/code/rails/actionpack/test/fixtures/collection_cache/index.html.erb: 8f52a1ba24922b018905574fbb082059
Cache digest for /Users/kasperhansen/Documents/code/rails/actionpack/test/fixtures/customers/_customer.html.erb: 4184ab71db6849621a4d8820fcd2c0ad
Cache read: views/david/1/4184ab71db6849621a4d8820fcd2c0ad
Cache write: views/david/1/4184ab71db6849621a4d8820fcd2c0ad
1 partials will be written to cache.
Cache write: views/david/1/8f52a1ba24922b018905574fbb082059

Collection automatically caches customers/_customer, so next render uses a multi read for any templates rendered now.
1 partials multi read from cache.
Cache read: views/david/2/4184ab71db6849621a4d8820fcd2c0ad
Cache write: views/david/2/4184ab71db6849621a4d8820fcd2c0ad
Cache read: views/david/3/4184ab71db6849621a4d8820fcd2c0ad
Cache write: views/david/3/4184ab71db6849621a4d8820fcd2c0ad
2 partials will be written to cache.
Cache write: views/david/2/8f52a1ba24922b018905574fbb082059
Cache write: views/david/3/8f52a1ba24922b018905574fbb082059

Collection automatically caches customers/_customer, so next render uses a multi read for any templates rendered now.
3 partials multi read from cache.

It seems the cache store doesn't log when using multi read. But the "3 partials multi read from cache." lacks information to be really useful, I think.

This comment has been minimized.

@dhh

dhh Feb 19, 2015

Member

I think we should just have the multi_get log:

Caches multi read: 
- views/david/2/4184ab71db6849621a4d8820fcd2c0ad
- views/david/2/4184ab71db6849621a4d8820fcd2c0ad
- views/david/3/4184ab71db6849621a4d8820fcd2c0ad
- views/david/3/4184ab71db6849621a4d8820fcd2c0ad

And that's actually all we need. We don't need this feature to log anything in particular. It's clear that it's working when multi reads are being logged. Then everything else can just stay the same.

@dhh

dhh Feb 19, 2015

Member

I think we should just have the multi_get log:

Caches multi read: 
- views/david/2/4184ab71db6849621a4d8820fcd2c0ad
- views/david/2/4184ab71db6849621a4d8820fcd2c0ad
- views/david/3/4184ab71db6849621a4d8820fcd2c0ad
- views/david/3/4184ab71db6849621a4d8820fcd2c0ad

And that's actually all we need. We don't need this feature to log anything in particular. It's clear that it's working when multi reads are being logged. Then everything else can just stay the same.

This comment has been minimized.

@kaspth

kaspth Feb 19, 2015

Member

Awesome 👍, I'll swing it tomorrow.

Kasper

Den 19/02/2015 kl. 22.45 skrev David Heinemeier Hansson notifications@github.com:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    
    I think we should just have the multi_get log:

Caches multi read:

  • views/david/2/4184ab71db6849621a4d8820fcd2c0ad
  • views/david/2/4184ab71db6849621a4d8820fcd2c0ad
  • views/david/3/4184ab71db6849621a4d8820fcd2c0ad
  • views/david/3/4184ab71db6849621a4d8820fcd2c0ad
    And that's actually all we need. We don't need this feature to log anything in particular. It's clear that it's working when multi reads are being logged. Then everything else can just stay the same.


Reply to this email directly or view it on GitHub.

@kaspth

kaspth Feb 19, 2015

Member

Awesome 👍, I'll swing it tomorrow.

Kasper

Den 19/02/2015 kl. 22.45 skrev David Heinemeier Hansson notifications@github.com:

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

  • def automatic_cache_eligible?
  •  single_template_render? && !callable_cache_key? && template_eligible?
    
  • end
  • def single_template_render?
  •  @template # Template is only set when a collection renders one template.
    
  • end
  • def callable_cache_key?
  •  @options[:cache].respond_to?(:call)
    
  • end
  • def template_eligible?
  •  @template.eligible_for_collection_caching?(as: @options[:as]).tap do
    
  •    ActionView::Base.logger.try :debug,
    
  •      "Collection automatically caches #{@template.virtual_path}, so next render uses a multi read for any templates rendered now."
    
    I think we should just have the multi_get log:

Caches multi read:

  • views/david/2/4184ab71db6849621a4d8820fcd2c0ad
  • views/david/2/4184ab71db6849621a4d8820fcd2c0ad
  • views/david/3/4184ab71db6849621a4d8820fcd2c0ad
  • views/david/3/4184ab71db6849621a4d8820fcd2c0ad
    And that's actually all we need. We don't need this feature to log anything in particular. It's clear that it's working when multi reads are being logged. Then everything else can just stay the same.


Reply to this email directly or view it on GitHub.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 20, 2015

Member

Alright, @dhh. I've added instrumentation in Cache and MemCacheStore formatted as you proposed. I've tweaked the internal log method and added instrument_multi to do this.

Member

kaspth commented Feb 20, 2015

Alright, @dhh. I've added instrumentation in Cache and MemCacheStore formatted as you proposed. I've tweaked the internal log method and added instrument_multi to do this.

+ end
+
+ private
+

This comment has been minimized.

@dhh

dhh Feb 20, 2015

Member

✂️ newline and indent everything private.

@dhh

dhh Feb 20, 2015

Member

✂️ newline and indent everything private.

rafaelfranca added a commit that referenced this pull request Feb 25, 2015

@rafaelfranca rafaelfranca merged commit 68a2a67 into rails:master Feb 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 25, 2015

Member

🎉

On Feb 25, 2015, at 06:55, Rafael Mendonça França notifications@github.com wrote:

Merged #18948.


Reply to this email directly or view it on GitHub.

Member

dhh commented Feb 25, 2015

🎉

On Feb 25, 2015, at 06:55, Rafael Mendonça França notifications@github.com wrote:

Merged #18948.


Reply to this email directly or view it on GitHub.

@kaspth kaspth deleted the kaspth:automatic-collection-caching branch Feb 26, 2015

@cristianbica

This comment has been minimized.

Show comment
Hide comment
@cristianbica

cristianbica Feb 26, 2015

Member

niiiice. @kaspth you should add a changelog entry

Member

cristianbica commented Feb 26, 2015

niiiice. @kaspth you should add a changelog entry

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 26, 2015

Member

@cristianbica right, I'll add one.

Member

kaspth commented Feb 26, 2015

@cristianbica right, I'll add one.

@dubek

This comment has been minimized.

Show comment
Hide comment
@dubek

dubek May 23, 2015

I think I'm missing something: I tried to remove the <% cache customer do %> line and the corresponding <% end %> line; the tests (test/controller/caching_test.rb) still pass. I think the following line doesn't actually check whether the cache was used:

ActionView::PartialRenderer.expects(:collection_with_template).never

(used twice in the test)

BTW - my goal was to try to beat the regexp of resource_cache_call_pattern, for example: add extra spaces like <% cache or spaces around the <% cache ... do %> line (I think that erubis trims spaces if the only thing on the line is a <% ... %> directive).

I think I'm missing something: I tried to remove the <% cache customer do %> line and the corresponding <% end %> line; the tests (test/controller/caching_test.rb) still pass. I think the following line doesn't actually check whether the cache was used:

ActionView::PartialRenderer.expects(:collection_with_template).never

(used twice in the test)

BTW - my goal was to try to beat the regexp of resource_cache_call_pattern, for example: add extra spaces like <% cache or spaces around the <% cache ... do %> line (I think that erubis trims spaces if the only thing on the line is a <% ... %> directive).

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth May 24, 2015

Owner

Thanks @dubek, you're right. I've submitted a pull request here: rails#20284.

Let us know if you find a way to beat the resource_cache_call_pattern regex 😄

Owner

kaspth replied May 24, 2015

Thanks @dubek, you're right. I've submitted a pull request here: rails#20284.

Let us know if you find a way to beat the resource_cache_call_pattern regex 😄

huacnlee added a commit to ruby-china/homeland that referenced this pull request May 23, 2015

@dubek

This comment has been minimized.

Show comment
Hide comment
@dubek

dubek May 25, 2015

Contributor

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).

Contributor

dubek commented May 25, 2015

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).

chloerei added a commit to ruby-china/homeland that referenced this pull request May 26, 2015

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth May 26, 2015

Member

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?

Member

kaspth commented May 26, 2015

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?

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

prathamesh-sonpatki added a commit to codetriage/codetriage that referenced this pull request Mar 9, 2016

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Mar 9, 2016

Member

Is this behavior mentioned in the guides?

Member

schneems commented Mar 9, 2016

Is this behavior mentioned in the guides?

@ghiculescu

This comment has been minimized.

Show comment
Hide comment
@ghiculescu

ghiculescu Jun 20, 2016

Contributor

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.

Contributor

ghiculescu commented Jun 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 5, 2016

Member

@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.

Member

kaspth commented Jul 5, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@ghiculescu

ghiculescu Jul 5, 2016

Contributor

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

Contributor

ghiculescu commented Jul 5, 2016

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

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 6, 2016

Member

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

Member

kaspth commented Jul 6, 2016

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

@mickey

This comment has been minimized.

Show comment
Hide comment
@mickey

mickey Jul 30, 2016

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

mickey commented Jul 30, 2016

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

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Aug 1, 2016

Member

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.

Member

schneems commented Aug 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mickey

mickey Aug 2, 2016

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

mickey commented Aug 2, 2016

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

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 2, 2016

Member

@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!

Member

kaspth commented Aug 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@mickey

mickey Aug 2, 2016

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

mickey commented Aug 2, 2016

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

@mahemoff mahemoff referenced this pull request in n8/multi_fetch_fragments Nov 3, 2016

Open

"alias_method_chain" Rails 5 deprecation warning #34

@aried3r

This comment has been minimized.

Show comment
Hide comment
@aried3r

aried3r Apr 7, 2017

Contributor

@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?

partials are just right

Contributor

aried3r commented Apr 7, 2017

@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?

partials are just right

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Apr 7, 2017

Member

Nope, check master that has the latest implementation.

Member

kaspth commented Apr 7, 2017

Nope, check master that has the latest implementation.

@aried3r

This comment has been minimized.

Show comment
Hide comment
@aried3r

aried3r Apr 7, 2017

Contributor

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

Contributor

aried3r commented Apr 7, 2017

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

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Apr 9, 2017

Member

@aried3r that's the one, yeah. 👍

Member

kaspth commented Apr 9, 2017

@aried3r that's the one, yeah. 👍

hlcfan added a commit to hlcfan/pokr that referenced this pull request May 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment