diff options
| -rw-r--r-- | actionpack/CHANGELOG | 4 | ||||
| -rwxr-xr-x | actionpack/lib/action_controller.rb | 2 | ||||
| -rwxr-xr-x | actionpack/lib/action_controller/base.rb | 148 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/benchmarking.rb | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/deprecated_redirects.rb | 17 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/deprecated_renders_and_redirects.rb | 76 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/layout.rb | 70 | ||||
| -rw-r--r-- | actionpack/lib/action_view/base.rb | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_view/partials.rb | 12 | ||||
| -rw-r--r-- | actionpack/test/abstract_unit.rb | 1 | ||||
| -rw-r--r-- | actionpack/test/controller/action_pack_assertions_test.rb | 2 | ||||
| -rw-r--r-- | actionpack/test/controller/active_record_assertions_test.rb | 8 | ||||
| -rw-r--r-- | actionpack/test/controller/filters_test.rb | 12 | ||||
| -rw-r--r-- | actionpack/test/controller/flash_test.rb | 12 | ||||
| -rw-r--r-- | actionpack/test/controller/new_render_test.rb | 8 |
15 files changed, 186 insertions, 198 deletions
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index d69b94fb22..c3c54727ed 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,6 +1,8 @@ *SVN* -* Improved performance with 5-30% through a series of Action Pack optimizations #1811 [Stefan Kaes] +* Improved performance of test app req/sec with ~10% refactoring the render method #1823 [Stefan Kaes] + +* Improved performance of test app req/sec with 5-30% through a series of Action Pack optimizations #1811 [Stefan Kaes] * Changed caching/expiration/hit to report using the DEBUG log level and errors to use the ERROR log level instead of both using INFO diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb index 253ff6a467..cbcb35b686 100755 --- a/actionpack/lib/action_controller.rb +++ b/actionpack/lib/action_controller.rb @@ -32,7 +32,7 @@ rescue LoadError end require 'action_controller/base' -require 'action_controller/deprecated_renders_and_redirects' +require 'action_controller/deprecated_redirects' require 'action_controller/rescue' require 'action_controller/benchmarking' require 'action_controller/filters' diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index 471eb34a17..995edfd9e6 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -571,78 +571,106 @@ module ActionController #:nodoc: # # # Renders an empty response with status code 401 (access denied) # render :nothing => true, :status => 401 - def render(options = {}, deprecated_status = nil) #:doc: - # puts "Rendering: #{options.inspect}" - raise DoubleRenderError if performed? + def render(options = nil, deprecated_status = nil) #:doc: + raise DoubleRenderError, "Can only render or redirect once per action" if performed? # Backwards compatibility - return render({ :template => options || default_template_name, :status => deprecated_status }) if !options.is_a?(Hash) + unless options.is_a?(Hash) + return render_file(options || default_template_name, deprecated_status, true) + end - add_variables_to_assigns - options[:status] = (options[:status] || DEFAULT_RENDER_STATUS_CODE).to_s - - if options[:text] - @response.headers["Status"] = options[:status] - @response.body = options[:text] - @performed_render = true - return options[:text] - - elsif options[:file] - assert_existance_of_template_file(options[:file]) if options[:use_full_path] - logger.info("Rendering #{options[:file]} (#{options[:status]})") unless logger.nil? - render(options.merge({ :text => @template.render_file(options[:file], options[:use_full_path])})) - - elsif options[:template] - render(options.merge({ :file => options[:template], :use_full_path => true })) - - elsif options[:inline] - render(options.merge({ - :text => - @template.render_template( - options[:type] || :rhtml, - options[:inline], - nil, - options[:locals] || {} - ) - })) - - elsif options[:action] - render(options.merge({ :template => default_template_name(options[:action]) })) - - elsif options[:partial] && options[:collection] - render(options.merge({ - :text => ( - @template.render_partial_collection( - options[:partial] == true ? default_template_name : options[:partial], - options[:collection], options[:spacer_template], - options[:locals] || {} - ) || '' - ) - })) - - elsif options[:partial] - render(options.merge({ :text => @template.render_partial( - options[:partial] == true ? default_template_name : options[:partial], - options[:object], options[:locals] || {} - ) })) - - elsif options[:nothing] - # Safari doesn't pass the headers of the return if the response is zero length - render(options.merge({ :text => " " })) + if text = options[:text] + render_text(text, options[:status]) else - render(options.merge({ :action => action_name })) + if file = options[:file] + render_file(file, options[:status], options[:use_full_path]) + + elsif template = options[:template] + render_file(template, options[:status], true) + + elsif inline = options[:inline] + render_template(inline, options[:status], options[:type]) + + elsif action_name = options[:action] + render_action(action_name, options[:status], options[:layout]) + + elsif partial = options[:partial] + partial = default_template_name if partial == true + if collection = options[:collection] + render_partial_collection(partial, collection, options[:spacer_template], options[:locals], options[:status]) + else + render_partial(partial, options[:object], options[:locals], options[:status]) + end + + elsif options[:nothing] + # Safari doesn't pass the headers of the return if the response is zero length + render_text(" ", options[:status]) + + else + render_file(default_template_name, options[:status], true) + + end end end # Renders according to the same rules as <tt>render</tt>, but returns the result in a string instead # of sending it as the response body to the browser. - def render_to_string(options = {}) #:doc: + def render_to_string(options = nil) #:doc: result = render(options) erase_render_results - return result + result + end + + def render_action(action_name, status = nil, with_layout = true) + # logger.info("Rendering action #{action_name} (#{status},#{with_layout})") if logger + if with_layout + render_with_layout(default_template_name(action_name), status) + else + render_with_no_layout(default_template_name(action_name), status) + end + end + + def render_file(template_path, status = nil, use_full_path = false) + add_variables_to_assigns + assert_existance_of_template_file(template_path) if use_full_path + logger.info("Rendering #{template_path} (#{status})") if logger + render_text(@template.render_file(template_path, use_full_path), status) end - + + def render_template(template, status = nil, type = :rhtml) + add_variables_to_assigns + render_text(@template.render_template(type, template), status) + end + + def render_text(text = nil, status = nil) + @performed_render = true + @response.headers['Status'] = (status || DEFAULT_RENDER_STATUS_CODE).to_s + @response.body = text + end + + def render_nothing(status = nil) + render_text(' ', status) + end + + def render_partial(partial_path = default_template_name, object = nil, local_assigns = nil, status = nil) + add_variables_to_assigns + render_text(@template.render_partial(partial_path, object, local_assigns), status) + end + + def render_partial_collection(partial_name, collection, partial_spacer_template = nil, local_assigns = nil, status = nil) + add_variables_to_assigns + render_text(@template.render_partial_collection(partial_name, collection, partial_spacer_template, local_assigns), status) + end + + def render_with_layout(template_name = default_template_name, status = nil, layout = nil) + render_with_a_layout(template_name, status, layout) + end + + def render_without_layout(template_name = default_template_name, status = nil) + render_with_no_layout(template_name, status) + end + # Clears the rendered results, allowing for another render to be performed. def erase_render_results #:nodoc: @@ -770,8 +798,6 @@ module ActionController #:nodoc: @template = @response.template @assigns = @response.template.assigns @headers = @response.headers - - @variables_added = nil end def initialize_current_url diff --git a/actionpack/lib/action_controller/benchmarking.rb b/actionpack/lib/action_controller/benchmarking.rb index db129f1839..386a291196 100644 --- a/actionpack/lib/action_controller/benchmarking.rb +++ b/actionpack/lib/action_controller/benchmarking.rb @@ -15,8 +15,8 @@ module ActionController #:nodoc: } end - def render_with_benchmark(options = {}, deprecated_status = nil) - if logger.nil? + def render_with_benchmark(options = nil, deprecated_status = nil) + unless logger render_without_benchmark(options, deprecated_status) else db_runtime = ActiveRecord::Base.connection.reset_runtime if Object.const_defined?("ActiveRecord") && ActiveRecord::Base.connected? @@ -35,7 +35,7 @@ module ActionController #:nodoc: end def perform_action_with_benchmark - if logger.nil? + unless logger perform_action_without_benchmark else runtime = [Benchmark::measure{ perform_action_without_benchmark }.real, 0.0001].max diff --git a/actionpack/lib/action_controller/deprecated_redirects.rb b/actionpack/lib/action_controller/deprecated_redirects.rb new file mode 100644 index 0000000000..264ac8de82 --- /dev/null +++ b/actionpack/lib/action_controller/deprecated_redirects.rb @@ -0,0 +1,17 @@ +module ActionController + class Base + protected + # Deprecated in favor of calling redirect_to directly with the path. + def redirect_to_path(path) + redirect_to(path) + end + + # Deprecated in favor of calling redirect_to directly with the url. If the resource has moved permanently, it's possible to pass + # true as the second parameter and the browser will get "301 Moved Permanently" instead of "302 Found". This can also be done through + # just setting the headers["Status"] to "301 Moved Permanently" before using the redirect_to. + def redirect_to_url(url, permanently = false) + headers["Status"] = "301 Moved Permanently" if permanently + redirect_to(url) + end + end +end diff --git a/actionpack/lib/action_controller/deprecated_renders_and_redirects.rb b/actionpack/lib/action_controller/deprecated_renders_and_redirects.rb deleted file mode 100644 index 44715d9cae..0000000000 --- a/actionpack/lib/action_controller/deprecated_renders_and_redirects.rb +++ /dev/null @@ -1,76 +0,0 @@ -module ActionController - class Base - protected - # Works like render, but instead of requiring a full template name, you can get by with specifying the action name. So calling - # <tt>render_action "show_many"</tt> in WeblogController#display will render "#{template_root}/weblog/show_many.rhtml" or - # "#{template_root}/weblog/show_many.rxml". - def render_action(action_name, status = nil) - render :action => action_name, :status => status - end - - # Works like render, but disregards the template_root and requires a full path to the template that needs to be rendered. Can be - # used like <tt>render_file "/Users/david/Code/Ruby/template"</tt> to render "/Users/david/Code/Ruby/template.rhtml" or - # "/Users/david/Code/Ruby/template.rxml". - def render_file(template_path, status = nil, use_full_path = false) - render :file => template_path, :status => status, :use_full_path => use_full_path - end - - # Renders the +template+ string, which is useful for rendering short templates you don't want to bother having a file for. So - # you'd call <tt>render_template "Hello, <%= @user.name %>"</tt> to greet the current user. Or if you want to render as Builder - # template, you could do <tt>render_template "xml.h1 @user.name", nil, "rxml"</tt>. - def render_template(template, status = nil, type = "rhtml") - render :inline => template, :status => status, :type => type - end - - # Renders the +text+ string without parsing it through any template engine. Useful for rendering static information as it's - # considerably faster than rendering through the template engine. - # Use block for response body if provided (useful for deferred rendering or streaming output). - def render_text(text = nil, status = nil) - render :text => text, :status => status - end - - # Renders an empty response that can be used when the request is only interested in triggering an effect. Do note that good - # HTTP manners mandate that you don't use GET requests to trigger data changes. - def render_nothing(status = nil) - render :nothing => true, :status => status - end - - # Renders the partial specified by <tt>partial_path</tt>, which by default is the name of the action itself. Example: - # - # class WeblogController < ActionController::Base - # def show - # render_partial # renders "weblog/_show.r(xml|html)" - # end - # end - def render_partial(partial_path = default_template_name, object = nil, local_assigns = {}) - render :partial => partial_path, :object => object, :locals => local_assigns - end - - # Renders a collection of partials using <tt>partial_name</tt> to iterate over the +collection+. - def render_partial_collection(partial_name, collection, partial_spacer_template = nil, local_assigns = {}) - render :partial => partial_name, :collection => collection, :spacer_template => partial_spacer_template, :locals => local_assigns - end - - def render_with_layout(template_name = default_template_name, status = nil, layout = nil) - render :template => template_name, :status => status, :layout => layout - end - - def render_without_layout(template_name = default_template_name, status = nil) - render :template => template_name, :status => status, :layout => false - end - - - # Deprecated in favor of calling redirect_to directly with the path. - def redirect_to_path(path) - redirect_to(path) - end - - # Deprecated in favor of calling redirect_to directly with the url. If the resource has moved permanently, it's possible to pass - # true as the second parameter and the browser will get "301 Moved Permanently" instead of "302 Found". This can also be done through - # just setting the headers["Status"] to "301 Moved Permanently" before using the redirect_to. - def redirect_to_url(url, permanently = false) - headers["Status"] = "301 Moved Permanently" if permanently - redirect_to(url) - end - end -end
\ No newline at end of file diff --git a/actionpack/lib/action_controller/layout.rb b/actionpack/lib/action_controller/layout.rb index a6a8d4096f..4247e8a1cf 100644 --- a/actionpack/lib/action_controller/layout.rb +++ b/actionpack/lib/action_controller/layout.rb @@ -194,66 +194,76 @@ module ActionController #:nodoc: # weblog/standard, but <tt>layout "standard"</tt> will return layouts/standard. def active_layout(passed_layout = nil) layout = passed_layout || self.class.read_inheritable_attribute("layout") + active_layout = case layout when Symbol then send(layout) when Proc then layout.call(self) when String then layout end + active_layout.include?("/") ? active_layout : "layouts/#{active_layout}" if active_layout end - def render_with_a_layout(options = {}, deprecated_status = nil, deprecated_layout = nil) #:nodoc: - options = render_with_a_layout_options(options) - if (layout = pick_layout(options, deprecated_layout)) - logger.info("Rendering #{options[:template]} within #{layout}") unless logger.nil? + def render_with_a_layout(options = nil, deprecated_status = nil, deprecated_layout = nil) #:nodoc: + template_with_options = options.is_a?(Hash) - @content_for_layout = render_with_no_layout(options.merge(:layout => false)) - erase_render_results + if apply_layout?(template_with_options, options) && (layout = pick_layout(template_with_options, options, deprecated_layout)) + logger.info("Rendering #{options} within #{layout}") if logger + + if template_with_options + content_for_layout = render_with_no_layout(options) + deprecated_status = options[:status] || deprecated_status + else + content_for_layout = render_with_no_layout(options, deprecated_status) + end - @assigns["content_for_layout"] = @content_for_layout - render_with_no_layout(options.merge({ :text => @template.render_file(layout, true), :status => options[:status] || deprecated_status })) + erase_render_results + @template.instance_variable_set("@content_for_layout", content_for_layout) + render_text(@template.render_file(layout, true), deprecated_status) else render_with_no_layout(options, deprecated_status) end end private - def render_with_a_layout_options(options) - return { :template => options } unless options.is_a?(Hash) - if options.values_at(:text, :file, :inline, :partial, :nothing).compact.empty? - options + def apply_layout?(template_with_options, options) + if template_with_options + (options.has_key?(:layout) && options[:layout]!=false) || options.values_at(:text, :file, :inline, :partial, :nothing).compact.empty? else - { :layout => false }.merge(options) + true end end - def pick_layout(options = {}, deprecated_layout = nil) - return deprecated_layout if !deprecated_layout.nil? - - if options.is_a?(Hash) - case options[:layout] + def pick_layout(template_with_options, options, deprecated_layout) + if deprecated_layout + deprecated_layout + elsif template_with_options + case layout = options[:layout] when FalseClass nil when NilClass, TrueClass active_layout if action_has_layout? else - active_layout(options[:layout]) + active_layout(layout) end else - (deprecated_layout || active_layout) if action_has_layout? + active_layout if action_has_layout? end end - + def action_has_layout? - conditions = self.class.layout_conditions || {} - case - when conditions[:only] - conditions[:only].include?(action_name) - when conditions[:except] - !conditions[:except].include?(action_name) - else - true + if conditions = self.class.layout_conditions + case + when only = conditions[:only] + only.include?(action_name) + when except = conditions[:except] + !except.include?(action_name) + else + true + end + else + true end end end -end +end
\ No newline at end of file diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 304d6bf1ed..7fc0c3b020 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -158,6 +158,7 @@ module ActionView #:nodoc: def initialize(base_path = nil, assigns_for_first_render = {}, controller = nil)#:nodoc: @base_path, @assigns = base_path, assigns_for_first_render + @assigns_added = nil @controller = controller @logger = controller && controller.logger end @@ -280,7 +281,10 @@ module ActionView #:nodoc: end def evaluate_assigns(local_assigns = {}) - @assigns.each { |key, value| instance_variable_set("@#{key}", value) } + unless @assigns_added + @assigns.each { |key, value| instance_variable_set("@#{key}", value) } + @assigns_added = true + end saved_locals = {} local_assigns.each do |key, value| diff --git a/actionpack/lib/action_view/partials.rb b/actionpack/lib/action_view/partials.rb index e078c52262..be9d40ca53 100644 --- a/actionpack/lib/action_view/partials.rb +++ b/actionpack/lib/action_view/partials.rb @@ -46,21 +46,25 @@ module ActionView # This will render the partial "advertisement/_ad.rhtml" regardless of which controller this is being called from. module Partials # Deprecated, use render :partial - def render_partial(partial_path, local_assigns = {}, deprecated_local_assigns = {}) #:nodoc: + def render_partial(partial_path, local_assigns = nil, deprecated_local_assigns = nil) #:nodoc: path, partial_name = partial_pieces(partial_path) object = extracting_object(partial_name, local_assigns, deprecated_local_assigns) local_assigns = extract_local_assigns(local_assigns, deprecated_local_assigns) + local_assigns = local_assigns ? local_assigns.clone : {} add_counter_to_local_assigns!(partial_name, local_assigns) + local_assigns[partial_name] = object - render("#{path}/_#{partial_name}", { partial_name => object }.merge(local_assigns)) + render("#{path}/_#{partial_name}", local_assigns) end # Deprecated, use render :partial, :collection - def render_partial_collection(partial_name, collection, partial_spacer_template = nil, local_assigns = {}) #:nodoc: + def render_partial_collection(partial_name, collection, partial_spacer_template = nil, local_assigns = nil) #:nodoc: collection_of_partials = Array.new counter_name = partial_counter_name(partial_name) + local_assigns = local_assigns ? local_assigns.clone : {} collection.each_with_index do |element, counter| - collection_of_partials.push(render_partial(partial_name, element, { counter_name => counter }.merge(local_assigns))) + local_assigns[counter_name] = counter + collection_of_partials.push(render_partial(partial_name, element, local_assigns)) end return nil if collection_of_partials.empty? diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 5c797a4434..0f8c309e06 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -10,3 +10,4 @@ require 'action_controller/test_process' ActionController::Base.logger = nil ActionController::Base.ignore_missing_templates = false ActionController::Routing::Routes.reload rescue nil + diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb index bccff5ab98..e7faeb25cd 100644 --- a/actionpack/test/controller/action_pack_assertions_test.rb +++ b/actionpack/test/controller/action_pack_assertions_test.rb @@ -48,7 +48,7 @@ class ActionPackAssertionsController < ActionController::Base # assign some template instance variables def assign_this @howdy = "ho" - render_text "Mr. Henke" + render :inline => "Mr. Henke" end def render_based_on_parameters diff --git a/actionpack/test/controller/active_record_assertions_test.rb b/actionpack/test/controller/active_record_assertions_test.rb index 7b3478e2a8..d4cbda80e9 100644 --- a/actionpack/test/controller/active_record_assertions_test.rb +++ b/actionpack/test/controller/active_record_assertions_test.rb @@ -47,7 +47,7 @@ class ActiveRecordAssertionsController < ActionController::Base @company = Company.new @company.name = "B" @company.rating = 2 - render_text "snicker...." + render :inline => "snicker...." end # fail with 2 bad columns @@ -55,7 +55,7 @@ class ActiveRecordAssertionsController < ActionController::Base @company = Company.new @company.name = "" @company.rating = 2 - render_text "double snicker...." + render :inline => "double snicker...." end # this will pass validation @@ -63,13 +63,13 @@ class ActiveRecordAssertionsController < ActionController::Base @company = Company.new @company.name = "A" @company.rating = 69 - render_text "Goodness Gracious!" + render :inline => "Goodness Gracious!" end # this will fail validation def bad_company @company = Company.new - render_text "Who's Bad?" + render :inline => "Who's Bad?" end # the safety dance...... diff --git a/actionpack/test/controller/filters_test.rb b/actionpack/test/controller/filters_test.rb index 5480843e22..1bd625f084 100644 --- a/actionpack/test/controller/filters_test.rb +++ b/actionpack/test/controller/filters_test.rb @@ -5,7 +5,7 @@ class FilterTest < Test::Unit::TestCase before_filter :ensure_login def show - render_text "ran action" + render :inline => "ran action" end private @@ -20,26 +20,26 @@ class FilterTest < Test::Unit::TestCase def show @ran_action = true - render_text "ran action" + render :inline => "ran action" end private def render_something_else - render_text "something else" + render :inline => "something else" end end class ConditionalFilterController < ActionController::Base def show - render_text "ran action" + render :inline => "ran action" end def another_action - render_text "ran action" + render :inline => "ran action" end def show_without_filter - render_text "ran action without filter" + render :inline => "ran action without filter" end private diff --git a/actionpack/test/controller/flash_test.rb b/actionpack/test/controller/flash_test.rb index 3a6e828bc8..54a1444f97 100644 --- a/actionpack/test/controller/flash_test.rb +++ b/actionpack/test/controller/flash_test.rb @@ -4,32 +4,32 @@ class FlashTest < Test::Unit::TestCase class TestController < ActionController::Base def set_flash flash["that"] = "hello" - render_text "hello" + render :inline => "hello" end def set_flash_now flash.now["that"] = "hello" @flash_copy = {}.update flash - render_text "hello" + render :inline => "hello" end def attempt_to_use_flash_now @flash_copy = {}.update flash @flashy = flash["that"] - render_text "hello" + render :inline => "hello" end def use_flash @flash_copy = {}.update flash @flashy = flash["that"] - render_text "hello" + render :inline => "hello" end def use_flash_and_keep_it @flash_copy = {}.update flash @flashy = flash["that"] keep_flash - render_text "hello" + render :inline => "hello" end def rescue_action(e) @@ -91,4 +91,4 @@ class FlashTest < Test::Unit::TestCase def process_request TestController.process(@request, @response) end -end
\ No newline at end of file +end diff --git a/actionpack/test/controller/new_render_test.rb b/actionpack/test/controller/new_render_test.rb index cd7a813df2..b2e3b6b399 100644 --- a/actionpack/test/controller/new_render_test.rb +++ b/actionpack/test/controller/new_render_test.rb @@ -311,8 +311,8 @@ class NewRenderTest < Test::Unit::TestCase assert_equal "<title>Talking to the layout</title>\nAction was here!", @response.body end - # def test_partials_list - # get :partials_list - # assert_equal "goodbyeHello: davidHello: marygoodbye\n", @response.body - # end + def test_partials_list + get :partials_list + assert_equal "goodbyeHello: davidHello: marygoodbye\n", @response.body + end end
\ No newline at end of file |