diff options
| -rw-r--r-- | actionpack/CHANGELOG | 2 | ||||
| -rwxr-xr-x | actionpack/lib/action_controller/base.rb | 27 | ||||
| -rwxr-xr-x | actionpack/lib/action_controller/cgi_ext/cgi_methods.rb | 33 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/cgi_ext/cookie_performance_fix.rb | 60 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/cgi_ext/raw_post_data_fix.rb | 51 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/cgi_process.rb | 18 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/filters.rb | 16 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/layout.rb | 2 | ||||
| -rwxr-xr-x | actionpack/lib/action_controller/request.rb | 49 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/test_process.rb | 1 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/url_rewriter.rb | 13 | ||||
| -rw-r--r-- | actionpack/test/controller/new_render_test.rb | 9 | ||||
| -rwxr-xr-x | actionpack/test/controller/redirect_test.rb | 2 | ||||
| -rw-r--r-- | actionpack/test/controller/render_test.rb | 9 |
14 files changed, 175 insertions, 117 deletions
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index a9ff040070..d69b94fb22 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *SVN* +* Improved performance 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 * Added support for per-action session management #1763 diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index bee72d90f2..471eb34a17 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -215,6 +215,10 @@ module ActionController #:nodoc: @@view_controller_internals = true cattr_accessor :view_controller_internals + # Protected instance variable cache + @@protected_variables_cache = nil + cattr_accessor :protected_variables_cache + # Prepends all the URL-generating helpers from AssetHelper. This makes it possible to easily move javascripts, stylesheets, # and images to a dedicated asset server away from the main web server. Example: # ActionController::Base.asset_host = "http://assets.example.com" @@ -351,12 +355,13 @@ module ActionController #:nodoc: assign_shortcuts(request, response) initialize_current_url @action_name = params[:action] || 'index' + @variables_added = nil - log_processing unless logger.nil? + log_processing if logger send(method, *arguments) close_session - return @response + @response end # Returns a URL that has been rewritten according to the options hash and the defined Routes. @@ -765,6 +770,8 @@ module ActionController #:nodoc: @template = @response.template @assigns = @response.template.assigns @headers = @response.headers + + @variables_added = nil end def initialize_current_url @@ -777,7 +784,7 @@ module ActionController #:nodoc: end def perform_action - if action_methods.include?(action_name) || action_methods.include?('method_missing') + if self.class.action_methods.include?(action_name) || self.class.action_methods.include?('method_missing') send(action_name) render unless performed? elsif template_exists? && template_public? @@ -792,17 +799,23 @@ module ActionController #:nodoc: end def action_methods - @action_methods ||= (self.class.public_instance_methods - self.class.hidden_actions) + self.class.action_methods end + def self.action_methods + @action_methods ||= (public_instance_methods - hidden_actions).inject({}) { |h, k| h[k] = true; h } + end def add_variables_to_assigns - add_instance_variables_to_assigns - add_class_variables_to_assigns if view_controller_internals + unless @variables_added + add_instance_variables_to_assigns + add_class_variables_to_assigns if view_controller_internals + @variables_added = true + end end def add_instance_variables_to_assigns - @@protected_variables_cache = protected_instance_variables.inject({}) { |h, k| h[k] = true; h } + @@protected_variables_cache ||= protected_instance_variables.inject({}) { |h, k| h[k] = true; h } instance_variables.each do |var| next if @@protected_variables_cache.include?(var) @assigns[var[1..-1]] = instance_variable_get(var) diff --git a/actionpack/lib/action_controller/cgi_ext/cgi_methods.rb b/actionpack/lib/action_controller/cgi_ext/cgi_methods.rb index c8ed3fb385..d922e48b10 100755 --- a/actionpack/lib/action_controller/cgi_ext/cgi_methods.rb +++ b/actionpack/lib/action_controller/cgi_ext/cgi_methods.rb @@ -12,10 +12,10 @@ class CGIMethods #:nodoc: query_string.split(/[&;]/).each { |p| k, v = p.split('=',2) - v = nil if (!v.nil? && v.empty?) + v = nil if (v && v.empty?) - k = CGI.unescape(k) unless k.nil? - v = CGI.unescape(v) unless v.nil? + k = CGI.unescape(k) if k + v = CGI.unescape(v) if v keys = split_key(k) last_key = keys.pop @@ -27,7 +27,7 @@ class CGIMethods #:nodoc: end } - return parsed_params + parsed_params end # Returns the request (POST/GET) parameters in a parsed form where pairs such as "customer[address][street]" / @@ -38,14 +38,16 @@ class CGIMethods #:nodoc: for key, value in params value = [value] if key =~ /.*\[\]$/ - CGIMethods.build_deep_hash( - CGIMethods.get_typed_value(value[0]), - parsed_params, - CGIMethods.get_levels(key) - ) + unless key.include?('[') + # much faster to test for the most common case first (GET) + # and avoid the call to build_deep_hash + parsed_params[key] = get_typed_value(value[0]) + else + build_deep_hash(get_typed_value(value[0]), parsed_params, get_levels(key)) + end end - return parsed_params + parsed_params end def self.parse_formatted_request_parameters(format, raw_post_data) @@ -72,14 +74,17 @@ class CGIMethods #:nodoc: keys.concat($2[1..-2].split('][')) keys << '' if key[-2..-1] == '[]' # Have to add it since split will drop empty strings - return keys + keys else - return [key] + [key] end end def CGIMethods.get_typed_value(value) - if value.respond_to?(:content_type) && !value.content_type.empty? + # test most frequent case first + if value.is_a?(String) + value + elsif value.respond_to?(:content_type) && !value.content_type.empty? # Uploaded file value elsif value.respond_to?(:read) @@ -88,7 +93,7 @@ class CGIMethods #:nodoc: elsif value.class == Array value.collect { |v| CGIMethods.get_typed_value(v) } else - # Standard value (not a multipart request) + # other value (neither string nor a multipart request) value.to_s end end diff --git a/actionpack/lib/action_controller/cgi_ext/cookie_performance_fix.rb b/actionpack/lib/action_controller/cgi_ext/cookie_performance_fix.rb index 225cea1905..1c30f82b19 100644 --- a/actionpack/lib/action_controller/cgi_ext/cookie_performance_fix.rb +++ b/actionpack/lib/action_controller/cgi_ext/cookie_performance_fix.rb @@ -23,28 +23,32 @@ class CGI #:nodoc: # servers. # # These keywords correspond to attributes of the cookie object. - def initialize(name = "", *value) - options = if name.kind_of?(String) - { "name" => name, "value" => value } - else - name - end - unless options.has_key?("name") + def initialize(name = '', *value) + if name.kind_of?(String) + @name = name + @value = Array(value) + @domain = nil + @expires = nil + @secure = false + @path = nil + else + @name = name['name'] + @value = Array(name['value']) + @domain = name['domain'] + @expires = name['expires'] + @secure = name['secure'] || false + @path = name['path'] + end + + unless @name raise ArgumentError, "`name' required" end - @name = options["name"] - @value = Array(options["value"]) # simple support for IE - if options["path"] - @path = options["path"] - else - %r|^(.*/)|.match(ENV["SCRIPT_NAME"]) - @path = ($1 or "") + unless @path + %r|^(.*/)|.match(ENV['SCRIPT_NAME']) + @path = ($1 or '') end - @domain = options["domain"] - @expires = options["expires"] - @secure = options["secure"] == true ? true : false super(@value) end @@ -102,20 +106,20 @@ class CGI #:nodoc: # def self.parse(raw_cookie) cookies = Hash.new([]) - return cookies unless raw_cookie - - raw_cookie.split(/; /).each do |pairs| - name, values = pairs.split('=',2) - next unless name and values - name = CGI::unescape(name) - values ||= "" - values = values.split('&').collect{|v| CGI::unescape(v) } - unless cookies.has_key?(name) - cookies[name] = new({ "name" => name, "value" => values }) + + if raw_cookie + raw_cookie.split(/; /).each do |pairs| + name, values = pairs.split('=',2) + next unless name and values + name = CGI::unescape(name) + values = values.split('&').collect!{|v| CGI::unescape(v) } + unless cookies.has_key?(name) + cookies[name] = new(name, *values) + end end end cookies end end # class Cookie -end
\ No newline at end of file +end diff --git a/actionpack/lib/action_controller/cgi_ext/raw_post_data_fix.rb b/actionpack/lib/action_controller/cgi_ext/raw_post_data_fix.rb index b2194eb07b..20fc6ff47f 100644 --- a/actionpack/lib/action_controller/cgi_ext/raw_post_data_fix.rb +++ b/actionpack/lib/action_controller/cgi_ext/raw_post_data_fix.rb @@ -6,14 +6,21 @@ class CGI #:nodoc: # Handles multipart forms (in particular, forms that involve file uploads). # Reads query parameters in the @params field, and cookies into @cookies. def initialize_query() - @cookies = CGI::Cookie::parse((env_table['HTTP_COOKIE'] || env_table['COOKIE'])) + @cookies = CGI::Cookie::parse(env_table['HTTP_COOKIE'] || env_table['COOKIE']) - if boundary = multipart_form_boundary + #fix some strange request environments + if method = env_table['REQUEST_METHOD'] + method = method.to_s.downcase.intern + else + method = :get + end + + if method == :post && (boundary = multipart_form_boundary) @multipart = true @params = read_multipart(boundary, Integer(env_table['CONTENT_LENGTH'])) else @multipart = false - @params = CGI::parse(read_query_params || "") + @params = CGI::parse(read_query_params(method) || "") end end @@ -22,21 +29,23 @@ class CGI #:nodoc: MULTIPART_FORM_BOUNDARY_RE = %r|\Amultipart/form-data.*boundary=\"?([^\";,]+)\"?|n #" end - def multipart_form_boundary - if env_table['REQUEST_METHOD'] == 'POST' - MULTIPART_FORM_BOUNDARY_RE.match(env_table['CONTENT_TYPE']).to_a.pop - end + def multipart_form_boundary + MULTIPART_FORM_BOUNDARY_RE.match(env_table['CONTENT_TYPE']).to_a.pop end - def read_params_from_query - if defined? MOD_RUBY + if defined? MOD_RUBY + def read_params_from_query Apache::request.args || '' - else - # fixes CGI querystring parsing for POSTs - if env_table['QUERY_STRING'].blank? && !env_table['REQUEST_URI'].blank? - env_table['QUERY_STRING'] = env_table['REQUEST_URI'].split('?', 2)[1] || '' + end + else + def read_params_from_query + # fixes CGI querystring parsing for lighttpd + env_qs = env_table['QUERY_STRING'] + if env_qs.blank? && !(uri=env_table['REQUEST_URI']).blank? + env_qs.replace(uri.split('?', 2)[1] || '') + else + env_qs end - env_table['QUERY_STRING'] end end @@ -46,13 +55,15 @@ class CGI #:nodoc: env_table['RAW_POST_DATA'] = content.split("&_").first.to_s.freeze # &_ is a fix for Safari Ajax postings that always append \000 end - def read_query_params - case env_table['REQUEST_METHOD'].to_s.upcase - when 'CMD' - read_from_cmdline - when 'POST', 'PUT' + def read_query_params(method) + case method + when :get + read_params_from_query + when :post, :put read_params_from_post - else # when 'GET', 'HEAD', 'DELETE', 'OPTIONS' + when :cmd + read_from_cmdline + else # when :head, :delete, :options read_params_from_query end end diff --git a/actionpack/lib/action_controller/cgi_process.rb b/actionpack/lib/action_controller/cgi_process.rb index 1b36274dd9..d1d8a660c7 100644 --- a/actionpack/lib/action_controller/cgi_process.rb +++ b/actionpack/lib/action_controller/cgi_process.rb @@ -52,19 +52,19 @@ module ActionController #:nodoc: end def query_string - return @cgi.query_string unless @cgi.query_string.nil? || @cgi.query_string.empty? - unless env['REQUEST_URI'].nil? - parts = env['REQUEST_URI'].split('?') + if (qs = @cgi.query_string) && !qs.empty? + qs + elsif uri = env['REQUEST_URI'] + parts = uri.split('?') + parts.shift + parts.join('?') else - return env['QUERY_STRING'] || '' - end - parts.shift - return parts.join('?') + env['QUERY_STRING'] || '' + end end def query_parameters - qs = self.query_string - qs.empty? ? {} : CGIMethods.parse_query_parameters(query_string) + (qs = self.query_string).empty? ? {} : CGIMethods.parse_query_parameters(qs) end def request_parameters diff --git a/actionpack/lib/action_controller/filters.rb b/actionpack/lib/action_controller/filters.rb index f3999a853e..a36370e969 100644 --- a/actionpack/lib/action_controller/filters.rb +++ b/actionpack/lib/action_controller/filters.rb @@ -157,7 +157,7 @@ module ActionController #:nodoc: conditions = extract_conditions!(filters) filters << block if block_given? add_action_conditions(filters, conditions) - append_filter_to_chain("before", filters) + append_filter_to_chain('before', filters) end # The passed <tt>filters</tt> will be prepended to the array of filters that's run _before_ actions @@ -166,7 +166,7 @@ module ActionController #:nodoc: conditions = extract_conditions!(filters) filters << block if block_given? add_action_conditions(filters, conditions) - prepend_filter_to_chain("before", filters) + prepend_filter_to_chain('before', filters) end # Short-hand for append_before_filter since that's the most common of the two. @@ -178,7 +178,7 @@ module ActionController #:nodoc: conditions = extract_conditions!(filters) filters << block if block_given? add_action_conditions(filters, conditions) - append_filter_to_chain("after", filters) + append_filter_to_chain('after', filters) end # The passed <tt>filters</tt> will be prepended to the array of filters that's run _after_ actions @@ -272,8 +272,8 @@ module ActionController #:nodoc: def add_action_conditions(filters, conditions) return unless conditions included, excluded = conditions[:only], conditions[:except] - write_inheritable_hash("included_actions", condition_hash(filters, included)) && return if included - write_inheritable_hash("excluded_actions", condition_hash(filters, excluded)) if excluded + write_inheritable_hash('included_actions', condition_hash(filters, included)) && return if included + write_inheritable_hash('excluded_actions', condition_hash(filters, excluded)) if excluded end def condition_hash(filters, *actions) @@ -322,7 +322,7 @@ module ActionController #:nodoc: else raise( ActionControllerError, - "Filters need to be either a symbol, proc/method, or class implementing a static filter method" + 'Filters need to be either a symbol, proc/method, or class implementing a static filter method' ) end @@ -334,11 +334,11 @@ module ActionController #:nodoc: end def filter_block?(filter) - filter.respond_to?("call") && (filter.arity == 1 || filter.arity == -1) + filter.respond_to?('call') && (filter.arity == 1 || filter.arity == -1) end def filter_class?(filter) - filter.respond_to?("filter") + filter.respond_to?('filter') end def action_exempted?(filter) diff --git a/actionpack/lib/action_controller/layout.rb b/actionpack/lib/action_controller/layout.rb index d7e01564a0..a6a8d4096f 100644 --- a/actionpack/lib/action_controller/layout.rb +++ b/actionpack/lib/action_controller/layout.rb @@ -210,7 +210,7 @@ module ActionController #:nodoc: @content_for_layout = render_with_no_layout(options.merge(:layout => false)) erase_render_results - add_variables_to_assigns + @assigns["content_for_layout"] = @content_for_layout render_with_no_layout(options.merge({ :text => @template.render_file(layout, true), :status => options[:status] || deprecated_status })) else render_with_no_layout(options, deprecated_status) diff --git a/actionpack/lib/action_controller/request.rb b/actionpack/lib/action_controller/request.rb index c41146f56a..3a29834b42 100755 --- a/actionpack/lib/action_controller/request.rb +++ b/actionpack/lib/action_controller/request.rb @@ -51,30 +51,31 @@ module ActionController # For backward compatibility, the post format is extracted from the # X-Post-Data-Format HTTP header if present. def post_format - if env['HTTP_X_POST_DATA_FORMAT'] - env['HTTP_X_POST_DATA_FORMAT'].downcase.to_sym - else - case env['CONTENT_TYPE'].to_s.downcase - when 'application/xml', 'text/xml' then :xml - when 'application/x-yaml', 'text/x-yaml' then :yaml - else :url_encoded - end - end + @post_format ||= + if env['HTTP_X_POST_DATA_FORMAT'] + env['HTTP_X_POST_DATA_FORMAT'].downcase.to_sym + else + case env['CONTENT_TYPE'].to_s.downcase + when 'application/xml', 'text/xml' then :xml + when 'application/x-yaml', 'text/x-yaml' then :yaml + else :url_encoded + end + end end # Is this a POST request formatted as XML or YAML? def formatted_post? - [ :xml, :yaml ].include?(post_format) && post? + post? && (post_format == :xml || post_format == :yaml) end # Is this a POST request formatted as XML? def xml_post? - post_format == :xml && post? + post? && post_format == :xml end # Is this a POST request formatted as YAML? def yaml_post? - post_format == :yaml && post? + post? && post_format == :yaml end # Returns true if the request's "X-Requested-With" header contains @@ -102,7 +103,7 @@ module ActionController return remote_ips.first.strip unless remote_ips.empty? end - return env['REMOTE_ADDR'] + env['REMOTE_ADDR'] end # Returns the domain part of a host, such as rubyonrails.org in "www.rubyonrails.org". You can specify @@ -127,25 +128,27 @@ module ActionController end def request_uri - unless env['REQUEST_URI'].nil? - (%r{^\w+\://[^/]+(/.*|$)$} =~ env['REQUEST_URI']) ? $1 : env['REQUEST_URI'] # Remove domain, which webrick puts into the request_uri. + if uri = env['REQUEST_URI'] + (%r{^\w+\://[^/]+(/.*|$)$} =~ uri) ? $1 : uri # Remove domain, which webrick puts into the request_uri. else # REQUEST_URI is blank under IIS - get this from PATH_INFO and SCRIPT_NAME - script_filename = env["SCRIPT_NAME"].to_s.match(%r{[^/]+$}) - request_uri = env["PATH_INFO"] - request_uri.sub!(/#{script_filename}\//, '') unless script_filename.nil? - request_uri += '?' + env["QUERY_STRING"] unless env["QUERY_STRING"].nil? || env["QUERY_STRING"].empty? - return request_uri + script_filename = env['SCRIPT_NAME'].to_s.match(%r{[^/]+$}) + uri = env['PATH_INFO'] + uri = uri.sub(/#{script_filename}\//, '') unless script_filename.nil? + unless (env_qs = env['QUERY_STRING']).nil? || env_qs.empty? + uri << '?' << env_qs + end + uri end end # Return 'https://' if this is an SSL request and 'http://' otherwise. def protocol - env["HTTPS"] == "on" ? 'https://' : 'http://' + ssl? ? 'https://' : 'http://' end # Is this an SSL request? def ssl? - protocol == 'https://' + env['HTTPS'] == 'on' end # Returns the interpreted path to requested resource after all the installation directory of this application was taken into account @@ -168,7 +171,7 @@ module ActionController # Returns the port number of this request as an integer. def port - env['SERVER_PORT'].to_i + @port_as_int ||= env['SERVER_PORT'].to_i end # Returns a port suffix like ":8080" if the port number of this request diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb index 17953240c8..ebbb66b4ec 100644 --- a/actionpack/lib/action_controller/test_process.rb +++ b/actionpack/lib/action_controller/test_process.rb @@ -35,6 +35,7 @@ module ActionController #:nodoc: def port=(number) @env["SERVER_PORT"] = number.to_i + @port_as_int = nil end def action=(action_name) diff --git a/actionpack/lib/action_controller/url_rewriter.rb b/actionpack/lib/action_controller/url_rewriter.rb index ecbc6851f4..7d66cf1ced 100644 --- a/actionpack/lib/action_controller/url_rewriter.rb +++ b/actionpack/lib/action_controller/url_rewriter.rb @@ -20,8 +20,10 @@ module ActionController private def rewrite_url(path, options) rewritten_url = "" - rewritten_url << (options[:protocol] || @request.protocol) unless options[:only_path] - rewritten_url << (options[:host] || @request.host_with_port) unless options[:only_path] + unless options[:only_path] + rewritten_url << (options[:protocol] || @request.protocol) + rewritten_url << (options[:host] || @request.host_with_port) + end rewritten_url << @request.relative_url_root.to_s unless options[:skip_relative_url_root] rewritten_url << path @@ -53,8 +55,11 @@ module ActionController only_keys.each do |key| value = hash[key] key = CGI.escape key.to_s - key << '[]' if value.class == Array - value = [ value ] unless value.class == Array + if value.class == Array + key << '[]' + else + value = [ value ] + end value.each { |val| elements << "#{key}=#{Routing.extract_parameter_value(val)}" } end diff --git a/actionpack/test/controller/new_render_test.rb b/actionpack/test/controller/new_render_test.rb index 8af594e94a..cd7a813df2 100644 --- a/actionpack/test/controller/new_render_test.rb +++ b/actionpack/test/controller/new_render_test.rb @@ -92,7 +92,7 @@ class NewRenderTestController < ActionController::Base end def accessing_params_in_template_with_layout - render :inline => "Hello: <%= params[:name] %>", :layout => nil + render :layout => nil, :inline => "Hello: <%= params[:name] %>" end def render_with_explicit_template @@ -201,15 +201,22 @@ class NewRenderTest < Test::Unit::TestCase end def test_access_to_request_in_view + view_internals_old_value = ActionController::Base.view_controller_internals + ActionController::Base.view_controller_internals = false + ActionController::Base.protected_variables_cache = nil get :hello_world assert_nil(assigns["request"]) ActionController::Base.view_controller_internals = true + ActionController::Base.protected_variables_cache = nil get :hello_world assert_kind_of ActionController::AbstractRequest, assigns["request"] + + ActionController::Base.view_controller_internals = view_internals_old_value + ActionController::Base.protected_variables_cache = nil end def test_render_xml diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index 36e742453a..403bb8f9d5 100755 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -97,4 +97,4 @@ module ModuleTest assert_redirected_to :controller => 'redirect', :action => "hello_world" end end -end
\ No newline at end of file +end diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 82ed4d755f..0a86cc914e 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -140,17 +140,24 @@ class RenderTest < Test::Unit::TestCase end def test_access_to_request_in_view + view_internals_old_value = ActionController::Base.view_controller_internals + ActionController::Base.view_controller_internals = false + ActionController::Base.protected_variables_cache = nil @request.action = "hello_world" response = process_request assert_nil response.template.assigns["request"] ActionController::Base.view_controller_internals = true + ActionController::Base.protected_variables_cache = nil @request.action = "hello_world" response = process_request - assert_kind_of ActionController::AbstractRequest, response.template.assigns["request"] + assert_kind_of ActionController::AbstractRequest, response.template.assigns["request"] + + ActionController::Base.view_controller_internals = view_internals_old_value + ActionController::Base.protected_variables_cache = nil end def test_render_xml |