From 918dc27345319fbabf25a43bd65b613878b3a66e Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 14:50:39 +0200 Subject: [PATCH 01/22] Compile ActionController::Base.config's methods to avoid method_missing overhead. --- actionpack/lib/action_controller/railtie.rb | 10 ++++++++ actionpack/lib/action_view/base.rb | 3 +-- .../lib/active_support/configurable.rb | 24 +++++++++++++++++-- .../lib/active_support/ordered_options.rb | 4 ++++ activesupport/test/configurable_test.rb | 18 ++++++++++++++ 5 files changed, 55 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_controller/railtie.rb b/actionpack/lib/action_controller/railtie.rb index aea28d9265..4c57d82f1c 100644 --- a/actionpack/lib/action_controller/railtie.rb +++ b/actionpack/lib/action_controller/railtie.rb @@ -26,6 +26,10 @@ module ActionController options.stylesheets_dir ||= paths.public.stylesheets.to_a.first options.page_cache_directory ||= paths.public.to_a.first + # make sure readers methods get compiled + options.asset_path ||= nil + options.asset_host ||= nil + ActiveSupport.on_load(:action_controller) do include app.routes.mounted_helpers extend ::AbstractController::Railties::RoutesHelpers.with(app.routes) @@ -33,5 +37,11 @@ module ActionController options.each { |k,v| send("#{k}=", v) } end end + + config.after_initialize do + ActiveSupport.on_load(:action_controller) do + config.crystalize! if config.respond_to?(:crystalize!) + end + end end end diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 3fa46d0f43..0bef3e3a08 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -209,8 +209,7 @@ module ActionView #:nodoc: @_request = controller.request if controller.respond_to?(:request) end - config = controller && controller.respond_to?(:config) ? controller.config : {} - @_config = ActiveSupport::InheritableOptions.new(config) + @_config = controller && controller.respond_to?(:config) ? controller.config.inheritable_copy : {} @_content_for = Hash.new { |h,k| h[k] = ActiveSupport::SafeBuffer.new } @_virtual_path = nil diff --git a/activesupport/lib/active_support/configurable.rb b/activesupport/lib/active_support/configurable.rb index 5b85f9394a..36634bd7f3 100644 --- a/activesupport/lib/active_support/configurable.rb +++ b/activesupport/lib/active_support/configurable.rb @@ -9,9 +9,29 @@ module ActiveSupport module Configurable extend ActiveSupport::Concern + class Configuration < ActiveSupport::InheritableOptions + def crystalize! + self.class.crystalize!(keys.reject {|key| respond_to?(key)}) + end + + # compiles reader methods so we don't have to go through method_missing + def self.crystalize!(keys) + keys.each do |key| + class_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{key}; self[#{key.inspect}]; end + RUBY + end + end + end + module ClassMethods def config - @_config ||= ActiveSupport::InheritableOptions.new(superclass.respond_to?(:config) ? superclass.config : {}) + @_config ||= if superclass.respond_to?(:config) + superclass.config.inheritable_copy + else + # create a new "anonymous" class that will host the compiled reader methods + Class.new(Configuration).new({}) + end end def configure @@ -48,7 +68,7 @@ module ActiveSupport # user.config.level # => 1 # def config - @_config ||= ActiveSupport::InheritableOptions.new(self.class.config) + @_config ||= self.class.config.inheritable_copy end end end diff --git a/activesupport/lib/active_support/ordered_options.rb b/activesupport/lib/active_support/ordered_options.rb index 37e357552c..2b67e72cff 100644 --- a/activesupport/lib/active_support/ordered_options.rb +++ b/activesupport/lib/active_support/ordered_options.rb @@ -39,5 +39,9 @@ module ActiveSupport #:nodoc: def initialize(parent) super() { |h,k| parent[k] } end + + def inheritable_copy + self.class.new(self) + end end end diff --git a/activesupport/test/configurable_test.rb b/activesupport/test/configurable_test.rb index cef67e3cf9..4f288eb4d5 100644 --- a/activesupport/test/configurable_test.rb +++ b/activesupport/test/configurable_test.rb @@ -39,4 +39,22 @@ class ConfigurableActiveSupport < ActiveSupport::TestCase assert_equal :baz, instance.config.foo assert_equal :bar, Parent.config.foo end + + test "configuration is crystalizeable" do + parent = Class.new { include ActiveSupport::Configurable } + child = Class.new(parent) + + parent.config.bar = :foo + assert !parent.config.respond_to?(:bar) + assert !child.config.respond_to?(:bar) + assert !child.new.config.respond_to?(:bar) + + parent.config.crystalize! + assert_equal :foo, parent.config.bar + assert_equal :foo, child.new.config.bar + + assert_respond_to parent.config, :bar + assert_respond_to child.config, :bar + assert_respond_to child.new.config, :bar + end end \ No newline at end of file From 8cda132136a766621b4c976cb1df7007d12ee6b5 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 14:51:31 +0200 Subject: [PATCH 02/22] Make InheritableOptions's constructor more flexible. --- activesupport/lib/active_support/configurable.rb | 2 +- activesupport/lib/active_support/ordered_options.rb | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/activesupport/lib/active_support/configurable.rb b/activesupport/lib/active_support/configurable.rb index 36634bd7f3..3d91560833 100644 --- a/activesupport/lib/active_support/configurable.rb +++ b/activesupport/lib/active_support/configurable.rb @@ -30,7 +30,7 @@ module ActiveSupport superclass.config.inheritable_copy else # create a new "anonymous" class that will host the compiled reader methods - Class.new(Configuration).new({}) + Class.new(Configuration).new end end diff --git a/activesupport/lib/active_support/ordered_options.rb b/activesupport/lib/active_support/ordered_options.rb index 2b67e72cff..99c6c5a0c0 100644 --- a/activesupport/lib/active_support/ordered_options.rb +++ b/activesupport/lib/active_support/ordered_options.rb @@ -36,8 +36,12 @@ module ActiveSupport #:nodoc: end class InheritableOptions < OrderedOptions - def initialize(parent) - super() { |h,k| parent[k] } + def initialize(parent = nil) + if parent + super() { |h,k| parent[k] } + else + super() + end end def inheritable_copy From 5a518487fe66b11b81e21dc6c31d036057a410ed Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 14:48:06 +0200 Subject: [PATCH 03/22] Try to use Hash's native #[] for speed. --- activesupport/lib/active_support/configurable.rb | 2 +- activesupport/lib/active_support/ordered_options.rb | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/activesupport/lib/active_support/configurable.rb b/activesupport/lib/active_support/configurable.rb index 3d91560833..1914f10669 100644 --- a/activesupport/lib/active_support/configurable.rb +++ b/activesupport/lib/active_support/configurable.rb @@ -18,7 +18,7 @@ module ActiveSupport def self.crystalize!(keys) keys.each do |key| class_eval <<-RUBY, __FILE__, __LINE__ + 1 - def #{key}; self[#{key.inspect}]; end + def #{key}; _get(#{key.inspect}); end RUBY end end diff --git a/activesupport/lib/active_support/ordered_options.rb b/activesupport/lib/active_support/ordered_options.rb index 99c6c5a0c0..124e1a74f8 100644 --- a/activesupport/lib/active_support/ordered_options.rb +++ b/activesupport/lib/active_support/ordered_options.rb @@ -18,6 +18,9 @@ require 'active_support/ordered_hash' # module ActiveSupport #:nodoc: class OrderedOptions < OrderedHash + alias_method :_get, :[] # preserve the original #[] method + protected :_get # make it protected + def []=(key, value) super(key.to_sym, value) end @@ -37,7 +40,10 @@ module ActiveSupport #:nodoc: class InheritableOptions < OrderedOptions def initialize(parent = nil) - if parent + if parent.kind_of?(OrderedOptions) + # use the faster _get when dealing with OrderedOptions + super() { |h,k| parent._get(k) } + elsif parent super() { |h,k| parent[k] } else super() From 320382ccd359397f2c83f8c0c622b296dcfb472b Mon Sep 17 00:00:00 2001 From: thedarkone Date: Fri, 24 Sep 2010 18:01:47 +0200 Subject: [PATCH 04/22] Creating singleton class for every object whenever the instance-level accessor is used quite is expensive. --- .../core_ext/class/attribute.rb | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/activesupport/lib/active_support/core_ext/class/attribute.rb b/activesupport/lib/active_support/core_ext/class/attribute.rb index 688cba03db..511f26963e 100644 --- a/activesupport/lib/active_support/core_ext/class/attribute.rb +++ b/activesupport/lib/active_support/core_ext/class/attribute.rb @@ -72,11 +72,20 @@ class Class remove_possible_method(:#{name}) define_method(:#{name}) { val } end + + if singleton_class? + class_eval do + remove_possible_method(:#{name}) + def #{name} + defined?(@#{name}) ? @#{name} : singleton_class.#{name} + end + end + end val end def #{name} - defined?(@#{name}) ? @#{name} : singleton_class.#{name} + defined?(@#{name}) ? @#{name} : self.class.#{name} end def #{name}? @@ -87,4 +96,15 @@ class Class attr_writer name if instance_writer end end + + private + def singleton_class? + # in case somebody is crazy enough to overwrite allocate + allocate = Class.instance_method(:allocate) + # object.class always points to a real (non-singleton) class + allocate.bind(self).call.class != self + rescue TypeError + # MRI/YARV/JRuby all disallow creating new instances of a singleton class + true + end end From f2e0b3575e8c84cd23b75fcbfee83e7c683781ef Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 26 Sep 2010 17:05:54 +0200 Subject: [PATCH 05/22] Use native attr_* macros for performance reasons. --- .../core_ext/module/attr_internal.rb | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/activesupport/lib/active_support/core_ext/module/attr_internal.rb b/activesupport/lib/active_support/core_ext/module/attr_internal.rb index 28bc30ae26..00db75bfec 100644 --- a/activesupport/lib/active_support/core_ext/module/attr_internal.rb +++ b/activesupport/lib/active_support/core_ext/module/attr_internal.rb @@ -1,16 +1,12 @@ class Module # Declares an attribute reader backed by an internally-named instance variable. def attr_internal_reader(*attrs) - attrs.each do |attr| - module_eval "def #{attr}() #{attr_internal_ivar_name(attr)} end", __FILE__, __LINE__ - end + attrs.each {|attr_name| attr_internal_define(attr_name, :reader)} end # Declares an attribute writer backed by an internally-named instance variable. def attr_internal_writer(*attrs) - attrs.each do |attr| - module_eval "def #{attr}=(v) #{attr_internal_ivar_name(attr)} = v end", __FILE__, __LINE__ - end + attrs.each {|attr_name| attr_internal_define(attr_name, :writer)} end # Declares an attribute reader and writer backed by an internally-named instance @@ -29,4 +25,15 @@ class Module def attr_internal_ivar_name(attr) Module.attr_internal_naming_format % attr end + + def attr_internal_define(attr_name, type) + internal_name = attr_internal_ivar_name(attr_name).sub(/\A@/, '') + class_eval do # class_eval is necessary on 1.9 or else the methods a made private + # use native attr_* methods as they are faster on some Ruby implementations + send("attr_#{type}", internal_name) + end + attr_name, internal_name = "#{attr_name}=", "#{internal_name}=" if type == :writer + alias_method attr_name, internal_name + remove_method internal_name + end end From 05e53b4c1a3422131f73598848b0332cdc793f69 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Wed, 22 Sep 2010 20:18:05 +0200 Subject: [PATCH 06/22] Optimize relative_url_root rewriting code. --- actionpack/lib/action_view/helpers/asset_tag_helper.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index 687cb83d75..bf57321e86 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -718,6 +718,10 @@ module ActionView "#{host}#{source}" end + def rewrite_relative_url_root(source, relative_url_root) + relative_url_root && !source.starts_with?("#{relative_url_root}/") ? "#{relative_url_root}#{source}" : source + end + # Add the the extension +ext+ if not present. Return full URLs otherwise untouched. # Prefix with /dir/ if lacking a leading +/+. Account for relative URL # roots. Rewrite the asset path for cache-busting asset ids. Include @@ -733,9 +737,7 @@ module ActionView source = rewrite_asset_path(source, config.asset_path) has_request = controller.respond_to?(:request) - if has_request && include_host && source !~ %r{^#{controller.config.relative_url_root}/} - source = "#{controller.config.relative_url_root}#{source}" - end + source = rewrite_relative_url_root(source, controller.config.relative_url_root) if has_request && include_host source = rewrite_host_and_protocol(source, has_request) if include_host source From 7b2d51817d58336324bce5de422ff82fcab8e18a Mon Sep 17 00:00:00 2001 From: thedarkone Date: Wed, 22 Sep 2010 22:38:13 +0200 Subject: [PATCH 07/22] Make asset extension rewriting faster. --- .../lib/action_view/helpers/asset_tag_helper.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index bf57321e86..7576177392 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -705,9 +705,15 @@ module ActionView private - def rewrite_extension?(source, dir, ext) - source_ext = File.extname(source)[1..-1] - ext && (source_ext.blank? || (ext != source_ext && File.exist?(File.join(config.assets_dir, dir, "#{source}.#{ext}")))) + def rewrite_extension(source, dir, ext) + source_ext = File.extname(source) + + if source_ext.empty? + "#{source}.#{ext}" + elsif ext != source_ext[1..-1] + with_ext = "#{source}.#{ext}" + with_ext if File.exist?(File.join(config.assets_dir, dir, with_ext)) + end || source end def rewrite_host_and_protocol(source, has_request) @@ -729,8 +735,8 @@ module ActionView def compute_public_path(source, dir, ext = nil, include_host = true) return source if is_uri?(source) - source += ".#{ext}" if rewrite_extension?(source, dir, ext) - source = "/#{dir}/#{source}" unless source[0] == ?/ + source = rewrite_extension(source, dir, ext) if ext + source = "/#{dir}/#{source}" unless source[0] == ?/ if controller.respond_to?(:env) && controller.env["action_dispatch.asset_path"] source = rewrite_asset_path(source, controller.env["action_dispatch.asset_path"]) end From 86bcccf8dfced5e9428407a71f02c1dcc186f2ba Mon Sep 17 00:00:00 2001 From: thedarkone Date: Thu, 23 Sep 2010 18:08:43 +0200 Subject: [PATCH 08/22] No need to create a separate lambda for each call. --- .../lib/action_dispatch/routing/route_set.rb | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 1a5f21bd09..60b1342d55 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -334,6 +334,19 @@ module ActionDispatch end class Generator #:nodoc: + PARAMETERIZE = { + :parameterize => lambda do |name, value| + if name == :controller + value + elsif value.is_a?(Array) + value.map { |v| Rack::Mount::Utils.escape_uri(v.to_param) }.join('/') + else + return nil unless param = value.to_param + param.split('/').map { |v| Rack::Mount::Utils.escape_uri(v) }.join("/") + end + end + } + attr_reader :options, :recall, :set, :named_route def initialize(options, recall, set, extras = false) @@ -422,7 +435,7 @@ module ActionDispatch end def generate - path, params = @set.set.generate(:path_info, named_route, options, recall, opts) + path, params = @set.set.generate(:path_info, named_route, options, recall, PARAMETERIZE) raise_routing_error unless path @@ -436,20 +449,6 @@ module ActionDispatch raise_routing_error end - def opts - parameterize = lambda do |name, value| - if name == :controller - value - elsif value.is_a?(Array) - value.map { |v| Rack::Mount::Utils.escape_uri(v.to_param) }.join('/') - else - return nil unless param = value.to_param - param.split('/').map { |v| Rack::Mount::Utils.escape_uri(v) }.join("/") - end - end - {:parameterize => parameterize} - end - def raise_routing_error raise ActionController::RoutingError.new("No route matches #{options.inspect}") end From 8fdb34b2373b13eee24a403a3af398cddb2a5ad0 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 15:11:19 +0200 Subject: [PATCH 09/22] Cache url_options on a per-request basis. --- .../lib/action_controller/metal/url_for.rb | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index 85c6b0a9b5..ef1071bb3d 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -5,16 +5,18 @@ module ActionController include AbstractController::UrlFor def url_options - options = {} - if _routes.equal?(env["action_dispatch.routes"]) - options[:script_name] = request.script_name.dup - end + @_url_options ||= begin + options = {} + if _routes.equal?(env["action_dispatch.routes"]) + options[:script_name] = request.script_name.dup + end - super.merge(options).reverse_merge( - :host => request.host_with_port, - :protocol => request.protocol, - :_path_segments => request.symbolized_path_parameters - ) + super.merge(options).reverse_merge( + :host => request.host_with_port, + :protocol => request.protocol, + :_path_segments => request.symbolized_path_parameters + ).freeze + end end end end From e12e2fb4f660da479110b35b375694bf267aedfb Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 17:37:40 +0200 Subject: [PATCH 10/22] Cache 2 of Request's commonly called methods. --- actionpack/lib/action_dispatch/http/url.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb index 2e39d0dbc2..3e5cd6a2f9 100644 --- a/actionpack/lib/action_dispatch/http/url.rb +++ b/actionpack/lib/action_dispatch/http/url.rb @@ -15,12 +15,12 @@ module ActionDispatch # Returns 'https://' if this is an SSL request and 'http://' otherwise. def protocol - ssl? ? 'https://' : 'http://' + @protocol ||= ssl? ? 'https://' : 'http://' end # Is this an SSL request? def ssl? - @env['HTTPS'] == 'on' || @env['HTTP_X_FORWARDED_PROTO'] == 'https' + @ssl ||= @env['HTTPS'] == 'on' || @env['HTTP_X_FORWARDED_PROTO'] == 'https' end # Returns the \host for this request, such as "example.com". From 4c360c15e53324a7e5940f943129a06da51ac7d3 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Fri, 24 Sep 2010 23:09:12 +0200 Subject: [PATCH 11/22] No need for an extra wrapper array. --- activesupport/lib/active_support/core_ext/hash/keys.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activesupport/lib/active_support/core_ext/hash/keys.rb b/activesupport/lib/active_support/core_ext/hash/keys.rb index 045a6944fa..2cb2c89d30 100644 --- a/activesupport/lib/active_support/core_ext/hash/keys.rb +++ b/activesupport/lib/active_support/core_ext/hash/keys.rb @@ -39,7 +39,7 @@ class Hash # { :name => "Rob", :age => "28" }.assert_valid_keys("name", "age") # => raises "ArgumentError: Unknown key(s): name, age" # { :name => "Rob", :age => "28" }.assert_valid_keys(:name, :age) # => passes, raises nothing def assert_valid_keys(*valid_keys) - unknown_keys = keys - [valid_keys].flatten + unknown_keys = keys - valid_keys.flatten raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}") unless unknown_keys.empty? end end From 77efc20a54708ba37ba679ffe90021bf8a8d3a8a Mon Sep 17 00:00:00 2001 From: thedarkone Date: Fri, 24 Sep 2010 23:21:59 +0200 Subject: [PATCH 12/22] Make assert_valid_keys slightly faster. --- activesupport/lib/active_support/core_ext/hash/keys.rb | 10 ++++++---- activesupport/test/core_ext/hash_ext_test.rb | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/activesupport/lib/active_support/core_ext/hash/keys.rb b/activesupport/lib/active_support/core_ext/hash/keys.rb index 2cb2c89d30..d8748b1138 100644 --- a/activesupport/lib/active_support/core_ext/hash/keys.rb +++ b/activesupport/lib/active_support/core_ext/hash/keys.rb @@ -35,11 +35,13 @@ class Hash # as keys, this will fail. # # ==== Examples - # { :name => "Rob", :years => "28" }.assert_valid_keys(:name, :age) # => raises "ArgumentError: Unknown key(s): years" - # { :name => "Rob", :age => "28" }.assert_valid_keys("name", "age") # => raises "ArgumentError: Unknown key(s): name, age" + # { :name => "Rob", :years => "28" }.assert_valid_keys(:name, :age) # => raises "ArgumentError: Unknown key: years" + # { :name => "Rob", :age => "28" }.assert_valid_keys("name", "age") # => raises "ArgumentError: Unknown key: name" # { :name => "Rob", :age => "28" }.assert_valid_keys(:name, :age) # => passes, raises nothing def assert_valid_keys(*valid_keys) - unknown_keys = keys - valid_keys.flatten - raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}") unless unknown_keys.empty? + valid_keys.flatten! + each_key do |k| + raise(ArgumentError, "Unknown key: #{k}") unless valid_keys.include?(k) + end end end diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb index fc8d8170a1..e5438745e0 100644 --- a/activesupport/test/core_ext/hash_ext_test.rb +++ b/activesupport/test/core_ext/hash_ext_test.rb @@ -282,7 +282,7 @@ class HashExtTest < Test::Unit::TestCase { :failure => "stuff", :funny => "business" }.assert_valid_keys(:failure, :funny) end - assert_raise(ArgumentError, "Unknown key(s): failore") do + assert_raise(ArgumentError, "Unknown key: failore") do { :failore => "stuff", :funny => "business" }.assert_valid_keys([ :failure, :funny ]) { :failore => "stuff", :funny => "business" }.assert_valid_keys(:failure, :funny) end From 7cee1587f14638e61a33d12114a0d5ae2620d62b Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sat, 25 Sep 2010 16:15:21 +0200 Subject: [PATCH 13/22] options[:action] is very likely to be nil. --- actionpack/lib/action_dispatch/routing/polymorphic_routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb index 02ba5236ee..eaef09d445 100644 --- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb +++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb @@ -105,7 +105,7 @@ module ActionDispatch else [ record_or_hash_or_array ] end - inflection = if options[:action].to_s == "new" + inflection = if options[:action] && options[:action].to_s == "new" args.pop :singular elsif (record.respond_to?(:persisted?) && !record.persisted?) From e1bccc5169ba18238aa0bde720384efc74c533b0 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sat, 25 Sep 2010 16:40:53 +0200 Subject: [PATCH 14/22] Simple .empty? test will do fine here (rails_asset_id returns nice strings). --- actionpack/lib/action_view/helpers/asset_tag_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index 7576177392..59d1c8b7eb 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -810,7 +810,7 @@ module ActionView end asset_id = rails_asset_id(source) - if asset_id.blank? + if asset_id.empty? source else source + "?#{asset_id}" From 70357666bc86629c8d10501209105b144855ddbc Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sat, 25 Sep 2010 16:42:35 +0200 Subject: [PATCH 15/22] Do a single string interpolation. --- actionpack/lib/action_view/helpers/asset_tag_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index 59d1c8b7eb..efd238c692 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -813,7 +813,7 @@ module ActionView if asset_id.empty? source else - source + "?#{asset_id}" + "#{source}?#{asset_id}" end end From 5b81d1f3ef1f1b06495eadcc1c2d110d76ac1de3 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 17:42:32 +0200 Subject: [PATCH 16/22] Hash#empty? is faster than Enumerable#any? when used on a Hash. --- actionpack/lib/action_dispatch/routing/route_set.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 60b1342d55..eeec10a403 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -443,7 +443,7 @@ module ActionDispatch return [path, params.keys] if @extras - path << "?#{params.to_query}" if params.any? + path << "?#{params.to_query}" unless params.empty? path rescue Rack::Mount::RoutingError raise_routing_error From 7d9f605f8072ef473e2c30800b91493b56af5b2b Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 26 Sep 2010 14:04:51 +0200 Subject: [PATCH 17/22] Clean up url_for. --- actionpack/lib/action_view/helpers/url_helper.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index f8147840ed..8d1def05de 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -95,7 +95,7 @@ module ActionView # # => javascript:history.back() def url_for(options = {}) options ||= {} - url = case options + case options when String options when Hash @@ -106,8 +106,6 @@ module ActionView else polymorphic_path(options) end - - url end # Creates a link tag of the given +name+ using a URL created by the set From dcabc6c04301a44d875447e4f88534ca04a538d6 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 26 Sep 2010 14:24:48 +0200 Subject: [PATCH 18/22] Remove dead code. --- actionpack/lib/action_view/helpers/url_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index 8d1def05de..98ea75502f 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -591,9 +591,9 @@ module ActionView html_options['data-remote'] = 'true' end - confirm = html_options.delete("confirm") - method, href = html_options.delete("method"), html_options['href'] + confirm = html_options.delete('confirm') + method = html_options.delete('method') add_confirm_to_attributes!(html_options, confirm) if confirm add_method_to_attributes!(html_options, method) if method From b127e0cac97886d9fcaaea7b91f88becd2fcff2c Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 17:43:27 +0200 Subject: [PATCH 19/22] Performance: refactor convert_options_to_data_attributes. --- .../lib/action_view/helpers/url_helper.rb | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb index 98ea75502f..1c3ca78d28 100644 --- a/actionpack/lib/action_view/helpers/url_helper.rb +++ b/actionpack/lib/action_view/helpers/url_helper.rb @@ -584,20 +584,24 @@ module ActionView private def convert_options_to_data_attributes(options, html_options) - html_options = {} if html_options.nil? - html_options = html_options.stringify_keys + if html_options.nil? + link_to_remote_options?(options) ? {'data-remote' => 'true'} : {} + else + html_options = html_options.stringify_keys + html_options['data-remote'] = 'true' if link_to_remote_options?(options) || link_to_remote_options?(html_options) - if (options.is_a?(Hash) && options.key?('remote') && options.delete('remote')) || (html_options.is_a?(Hash) && html_options.key?('remote') && html_options.delete('remote')) - html_options['data-remote'] = 'true' + confirm = html_options.delete('confirm') + method = html_options.delete('method') + + add_confirm_to_attributes!(html_options, confirm) if confirm + add_method_to_attributes!(html_options, method) if method + + html_options end + end - - confirm = html_options.delete('confirm') - method = html_options.delete('method') - add_confirm_to_attributes!(html_options, confirm) if confirm - add_method_to_attributes!(html_options, method) if method - - html_options + def link_to_remote_options?(options) + options.is_a?(Hash) && options.key?('remote') && options.delete('remote') end def add_confirm_to_attributes!(html_options, confirm) From bb47927d914480218b003d322c8e8ebcd388093e Mon Sep 17 00:00:00 2001 From: thedarkone Date: Mon, 27 Sep 2010 17:44:11 +0200 Subject: [PATCH 20/22] Convert unless/else into if/else. --- .../lib/action_dispatch/routing/polymorphic_routes.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb index eaef09d445..49e237f8db 100644 --- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb +++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb @@ -168,10 +168,7 @@ module ActionDispatch end def build_named_route_call(records, inflection, options = {}) - unless records.is_a?(Array) - record = extract_record(records) - route = [] - else + if records.is_a?(Array) record = records.pop route = records.map do |parent| if parent.is_a?(Symbol) || parent.is_a?(String) @@ -180,6 +177,9 @@ module ActionDispatch ActiveModel::Naming.route_key(parent).singularize end end + else + record = extract_record(records) + route = [] end if record.is_a?(Symbol) || record.is_a?(String) From dc09cc055ab79ec8181ec4089449c94759700e51 Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 26 Sep 2010 14:45:27 +0200 Subject: [PATCH 21/22] Assume compute_asset_host returns reasonable values. --- actionpack/lib/action_view/helpers/asset_tag_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb index efd238c692..c1dfbe5dc3 100644 --- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb @@ -718,7 +718,7 @@ module ActionView def rewrite_host_and_protocol(source, has_request) host = compute_asset_host(source) - if has_request && host.present? && !is_uri?(host) + if has_request && host && !is_uri?(host) host = "#{controller.request.protocol}#{host}" end "#{host}#{source}" From 9c57bd8578e5ed47f143a2755b03ffd2ef85c65d Mon Sep 17 00:00:00 2001 From: thedarkone Date: Sun, 26 Sep 2010 14:54:49 +0200 Subject: [PATCH 22/22] Fix Namind#model_name. --- activemodel/lib/active_model/naming.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/activemodel/lib/active_model/naming.rb b/activemodel/lib/active_model/naming.rb index fc2abacb6d..2d580fd325 100644 --- a/activemodel/lib/active_model/naming.rb +++ b/activemodel/lib/active_model/naming.rb @@ -73,8 +73,10 @@ module ActiveModel # Returns an ActiveModel::Name object for module. It can be # used to retrieve all kinds of naming-related information. def model_name - namespace = self.parents.detect { |n| n.respond_to?(:_railtie) } - @_model_name ||= ActiveModel::Name.new(self, namespace) + @_model_name ||= begin + namespace = self.parents.detect { |n| n.respond_to?(:_railtie) } + ActiveModel::Name.new(self, namespace) + end end # Returns the plural class name of a record or class. Examples: