Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

ActionView::PartialRenderer cleanup since, we removed caching #8274

Closed
wants to merge 1 commit into from

3 participants

@senny
Owner

With #8212 we removed the caching of ActionView::PartialRenderer. This PR is to cleanup the PartialRenderer from code added specifically because of the caching.

I performed:

  • removed assignments to of ivars to locals
@senny
Owner

@carlosantoniodasilva @josevalim you mentioned fragments, that were added specifically for the caching. Please let me know if there is more to update.

@josevalim
Owner

I don't think we should merge this. Indeed, we removed the memoization, but accessing an instance variable in a loop is still more expensive. And reducing the number of instance variable access is the main reason for the current code.

@carlosantoniodasilva

I was told once by @josevalim that these were local vars because it's faster this way than accessing ivars - since rendering partials is a hot spot, so I'm unsure about merging it.

@carlosantoniodasilva

Ha! there we go :smile:

@senny
Owner

ah I see. Are there other fragments that were linked to the caching? Otherwise simply close the PR.

@josevalim josevalim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 19, 2012
  1. @senny
This page is out of date. Refresh to see the latest.
Showing with 14 additions and 21 deletions.
  1. +14 −21 actionpack/lib/action_view/renderer/partial_renderer.rb
View
35 actionpack/lib/action_view/renderer/partial_renderer.rb
@@ -290,21 +290,18 @@ def render_collection
end
def render_partial
- view, locals, block = @view, @locals, @block
- object, as = @object, @variable
-
- if !block && (layout = @options[:layout])
+ if !@block && (layout = @options[:layout])
layout = find_template(layout, @template_keys)
end
- object ||= locals[as]
- locals[as] = object
+ @object ||= @locals[@variable]
+ @locals[@variable] = @object
- content = @template.render(view, locals) do |*name|
- view._layout_for(*name, &block)
+ content = @template.render(@view, @locals) do |*name|
+ @view._layout_for(*name, &@block)
end
- content = layout.render(view, locals){ content } if layout
+ content = layout.render(@view, @locals){ content } if layout
content
end
@@ -374,39 +371,35 @@ def find_template(path, locals)
end
def collection_with_template
- view, locals, template = @view, @locals, @template
- as, counter = @variable, @variable_counter
-
if layout = @options[:layout]
layout = find_template(layout, @template_keys)
end
index = -1
@collection.map do |object|
- locals[as] = object
- locals[counter] = (index += 1)
+ @locals[@variable] = object
+ @locals[@variable_counter] = (index += 1)
- content = template.render(view, locals)
- content = layout.render(view, locals) { content } if layout
+ content = @template.render(@view, @locals)
+ content = layout.render(@view, @locals) { content } if layout
content
end
end
def collection_without_template
- view, locals, collection_data = @view, @locals, @collection_data
cache = {}
keys = @locals.keys
index = -1
@collection.map do |object|
index += 1
- path, as, counter = collection_data[index]
+ path, as, counter = @collection_data[index]
- locals[as] = object
- locals[counter] = index
+ @locals[as] = object
+ @locals[counter] = index
template = (cache[path] ||= find_template(path, keys + [as, counter]))
- template.render(view, locals)
+ template.render(@view, @locals)
end
end
Something went wrong with that request. Please try again.