From 80ff0b9f1c07eae7668680fd12335ffa218e53ac Mon Sep 17 00:00:00 2001
From: Michael Koziarski <michael@koziarski.com>
Date: Sun, 9 Sep 2007 00:18:55 +0000
Subject: Optimise named route generation when using positional arguments.
Closes #9450 [Koz]
This change delivers significant performance benefits for the most
common usage scenarios for modern rails applications by avoiding the
costly trip through url_for. Initial benchmarks indicate this is
between 6 and 20 times as fast.
git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7421 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
---
actionpack/CHANGELOG | 7 +
actionpack/lib/action_controller/integration.rb | 4 +
actionpack/lib/action_controller/routing.rb | 40 ++++--
.../lib/action_controller/routing_optimisation.rb | 99 +++++++++++++++
actionpack/test/controller/resources_test.rb | 12 ++
actionpack/test/controller/routing_test.rb | 141 +++++++++++++--------
6 files changed, 244 insertions(+), 59 deletions(-)
create mode 100644 actionpack/lib/action_controller/routing_optimisation.rb
diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index fdf6fd7e8c..33cf662375 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,12 @@
*SVN*
+* Optimise named route generation when using positional arguments. [Koz]
+
+ This change delivers significant performance benefits for the most
+ common usage scenarios for modern rails applications by avoiding the
+ costly trip through url_for. Initial benchmarks indicate this is
+ between 6 and 20 times as fast.
+
* Explicitly require active_record/query_cache before using it. [Jeremy Kemper]
* Fix layout overriding response status. #9476 [lotswholetime]
diff --git a/actionpack/lib/action_controller/integration.rb b/actionpack/lib/action_controller/integration.rb
index 2bff3a7ae0..80602489b9 100644
--- a/actionpack/lib/action_controller/integration.rb
+++ b/actionpack/lib/action_controller/integration.rb
@@ -74,6 +74,10 @@ module ActionController
unless defined? @named_routes_configured
# install the named routes in this session instance.
+ # But we have to disable the optimisation code so that we can
+ # generate routes without @request being initialized
+ Routing.optimise_named_routes=false
+ Routing::Routes.reload!
klass = class<<self; self; end
Routing::Routes.install_helpers(klass)
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 4cf9b010ba..d20c17c30e 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -1,6 +1,7 @@
require 'cgi'
require 'uri'
require 'action_controller/polymorphic_routes'
+require 'action_controller/routing_optimisation'
class Object
def to_param
@@ -255,6 +256,11 @@ module ActionController
mattr_accessor :controller_paths
self.controller_paths = []
+ # Indicates whether or not optimise the generated named
+ # route helper methods
+ mattr_accessor :optimise_named_routes
+ self.optimise_named_routes = true
+
# A helper module to hold URL related helpers.
module Helpers
include PolymorphicRoutes
@@ -325,13 +331,25 @@ module ActionController
end
class Route #:nodoc:
- attr_accessor :segments, :requirements, :conditions
+ attr_accessor :segments, :requirements, :conditions, :optimise
def initialize
@segments = []
@requirements = {}
@conditions = {}
end
+
+ # Indicates whether the routes should be optimised with the string interpolation
+ # version of the named routes methods.
+ def optimise?
+ @optimise
+ end
+
+ def segment_keys
+ segments.collect do |segment|
+ segment.key if segment.respond_to? :key
+ end.compact
+ end
# Write and compile a +generate+ method for this Route.
def write_generation
@@ -381,6 +399,7 @@ module ActionController
end
requirement_conditions * ' && ' unless requirement_conditions.empty?
end
+
def generation_structure
segments.last.string_structure segments[0..-2]
end
@@ -977,9 +996,15 @@ module ActionController
requirements = assign_route_options(segments, defaults, requirements)
route = Route.new
+
route.segments = segments
route.requirements = requirements
route.conditions = conditions
+
+ # Routes cannot use the current string interpolation method
+ # if there are user-supplied :requirements as the interpolation
+ # code won't raise RoutingErrors when generating
+ route.optimise = !options.key?(:requirements) && ActionController::Routing.optimise_named_routes
if !route.significant_keys.include?(:action) && !route.requirements[:action]
route.requirements[:action] = "index"
@@ -1051,7 +1076,7 @@ module ActionController
# named routes.
class NamedRouteCollection #:nodoc:
include Enumerable
-
+ include ActionController::Routing::Optimisation
attr_reader :routes, :helpers
def initialize
@@ -1128,15 +1153,15 @@ module ActionController
def define_url_helper(route, name, kind, options)
selector = url_helper_name(name, kind)
-
# The segment keys used for positional paramters
- segment_keys = route.segments.collect do |segment|
- segment.key if segment.respond_to? :key
- end.compact
+
hash_access_method = hash_access_name(name, kind)
@module.send :module_eval, <<-end_eval # We use module_eval to avoid leaks
def #{selector}(*args)
+
+ #{generate_optimisation_block(route, kind)}
+
opts = if args.empty? || Hash === args.first
args.first || {}
else
@@ -1154,7 +1179,7 @@ module ActionController
# foo_url(bar, baz, bang, :sort_by => 'baz')
#
options = args.last.is_a?(Hash) ? args.pop : {}
- args = args.zip(#{segment_keys.inspect}).inject({}) do |h, (v, k)|
+ args = args.zip(#{route.segment_keys.inspect}).inject({}) do |h, (v, k)|
h[k] = v
h
end
@@ -1167,7 +1192,6 @@ module ActionController
@module.send(:protected, selector)
helpers << selector
end
-
end
attr_accessor :routes, :named_routes
diff --git a/actionpack/lib/action_controller/routing_optimisation.rb b/actionpack/lib/action_controller/routing_optimisation.rb
new file mode 100644
index 0000000000..535f1bed63
--- /dev/null
+++ b/actionpack/lib/action_controller/routing_optimisation.rb
@@ -0,0 +1,99 @@
+module ActionController
+ module Routing
+ # Much of the slow performance from routes comes from the
+ # complexity of expiry, :requirements matching, defaults providing
+ # and figuring out which url pattern to use. With named routes
+ # we can avoid the expense of finding the right route. So if
+ # they've provided the right number of arguments, and have no
+ # :requirements, we can just build up a string and return it.
+ #
+ # To support building optimisations for other common cases, the
+ # generation code is seperated into several classes
+ module Optimisation
+ def generate_optimisation_block(route, kind)
+ return "" unless route.optimise?
+ OPTIMISERS.inject("") do |memo, klazz|
+ optimiser = klazz.new(route, kind)
+ memo << "return #{optimiser.generation_code} if #{optimiser.guard_condition}\n"
+ end
+ end
+
+ class Optimiser
+ attr_reader :route, :kind
+ def initialize(route, kind)
+ @route = route
+ @kind = kind
+ end
+
+ def guard_condition
+ 'false'
+ end
+
+ def generation_code
+ 'nil'
+ end
+ end
+
+ # Given a route:
+ # map.person '/people/:id'
+ #
+ # If the user calls person_url(@person), we can simply
+ # return a string like "/people/#{@person.to_param}"
+ # rather than triggering the expensive logic in url_for
+ class PositionalArguments < Optimiser
+ def guard_condition
+ number_of_arguments = route.segment_keys.size
+ # if they're using foo_url(:id=>2) it's one
+ # argument, but we don't want to generate /foos/id2
+ if number_of_arguments == 1
+ "args.size == 1 && !args.first.is_a?(Hash)"
+ else
+ "args.size == #{number_of_arguments}"
+ end
+ end
+
+ def generation_code
+ elements = []
+ idx = 0
+
+
+ if kind == :url
+ elements << '#{request.protocol}'
+ elements << '#{request.host_with_port}'
+ end
+
+ # The last entry in route.segments appears to
+ # *always* be a 'divider segment' for '/'
+ # but we have assertions to ensure that we don't
+ # include the trailing slashes, so skip them
+ route.segments[0..-2].each do |segment|
+ if segment.is_a?(DynamicSegment)
+ elements << "\#{URI.escape(args[#{idx}].to_param, ActionController::Routing::Segment::UNSAFE_PCHAR)}"
+ idx += 1
+ else
+ elements << segment.to_s
+ end
+ end
+ %("#{elements * ''}")
+ end
+ end
+
+ # This case is mostly the same as the positional arguments case
+ # above, but it supports additional query parameters as the last
+ # argument
+ class PositionalArgumentsWithAdditionalParams < PositionalArguments
+ def guard_condition
+ "args.size == #{route.segment_keys.size + 1}"
+ end
+
+ # This case uses almost the Use the same code as positional arguments,
+ # but add an args.last.to_query on the end
+ def generation_code
+ super.insert(-2, '?#{args.last.to_query}')
+ end
+ end
+
+ OPTIMISERS = [PositionalArguments, PositionalArgumentsWithAdditionalParams]
+ end
+ end
+end
\ No newline at end of file
diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb
index 57b956e777..29c25e6cfc 100644
--- a/actionpack/test/controller/resources_test.rb
+++ b/actionpack/test/controller/resources_test.rb
@@ -26,6 +26,18 @@ module Backoffice
end
class ResourcesTest < Test::Unit::TestCase
+
+
+ # The assertions in these tests are incompatible with the hash method
+ # optimisation. This could indicate user level problems
+ def setup
+ ActionController::Routing.optimise_named_routes = false
+ end
+
+ def tear_down
+ ActionController::Routing.optimise_named_routes = true
+ end
+
def test_should_arrange_actions
resource = ActionController::Resources::Resource.new(:messages,
:collection => { :rss => :get, :reorder => :post, :csv => :post },
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 5ca63396b4..4901c93a60 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -47,6 +47,8 @@ end
class LegacyRouteSetTests < Test::Unit::TestCase
attr_reader :rs
def setup
+ # These tests assume optimisation is on, so re-enable it.
+ ActionController::Routing.optimise_named_routes = true
@rs = ::ActionController::Routing::RouteSet.new
@rs.draw {|m| m.connect ':controller/:action/:id' }
ActionController::Routing.use_controllers! %w(content admin/user admin/news_feed)
@@ -166,40 +168,49 @@ class LegacyRouteSetTests < Test::Unit::TestCase
def test_basic_named_route
rs.add_named_route :home, '', :controller => 'content', :action => 'list'
- x = setup_for_named_route.new
- assert_equal({:controller => 'content', :action => 'list', :use_route => :home, :only_path => false},
+ x = setup_for_named_route
+ assert_equal("http://named.route.test",
x.send(:home_url))
end
def test_named_route_with_option
rs.add_named_route :page, 'page/:title', :controller => 'content', :action => 'show_page'
- x = setup_for_named_route.new
- assert_equal({:controller => 'content', :action => 'show_page', :title => 'new stuff', :use_route => :page, :only_path => false},
+ x = setup_for_named_route
+ assert_equal("http://named.route.test/page/new%20stuff",
x.send(:page_url, :title => 'new stuff'))
end
def test_named_route_with_default
rs.add_named_route :page, 'page/:title', :controller => 'content', :action => 'show_page', :title => 'AboutPage'
- x = setup_for_named_route.new
- assert_equal({:controller => 'content', :action => 'show_page', :title => 'AboutPage', :use_route => :page, :only_path => false},
- x.send(:page_url))
- assert_equal({:controller => 'content', :action => 'show_page', :title => 'AboutRails', :use_route => :page, :only_path => false},
+ x = setup_for_named_route
+ assert_equal("http://named.route.test/page/AboutRails",
x.send(:page_url, :title => "AboutRails"))
end
def test_named_route_with_nested_controller
rs.add_named_route :users, 'admin/user', :controller => '/admin/user', :action => 'index'
- x = setup_for_named_route.new
- assert_equal({:controller => '/admin/user', :action => 'index', :use_route => :users, :only_path => false},
+ x = setup_for_named_route
+ assert_equal("http://named.route.test/admin/user",
x.send(:users_url))
end
+
+ uses_mocha "named route optimisation" do
+ def test_optimised_named_route_call_never_uses_url_for
+ rs.add_named_route :users, 'admin/user', :controller => '/admin/user', :action => 'index'
+ x = setup_for_named_route
+ x.expects(:url_for).never
+ x.send(:users_url)
+ x.send(:users_path)
+ x.send(:users_url, :some_arg=>"some_value")
+ x.send(:users_path, :some_arg=>"some_value")
+ end
+ end
def setup_for_named_route
- x = Class.new
- x.send(:define_method, :url_for) {|x| x}
- rs.install_helpers(x)
- x
+ klass = Class.new(MockController)
+ rs.install_helpers(klass)
+ klass.new(rs)
end
def test_named_route_without_hash
@@ -214,13 +225,13 @@ class LegacyRouteSetTests < Test::Unit::TestCase
:year => /\d+/, :month => /\d+/, :day => /\d+/
map.connect ':controller/:action/:id'
end
- x = setup_for_named_route.new
+ x = setup_for_named_route
+ # assert_equal(
+ # {:controller => 'page', :action => 'show', :title => 'hi', :use_route => :article, :only_path => false},
+ # x.send(:article_url, :title => 'hi')
+ # )
assert_equal(
- {:controller => 'page', :action => 'show', :title => 'hi', :use_route => :article, :only_path => false},
- x.send(:article_url, :title => 'hi')
- )
- assert_equal(
- {:controller => 'page', :action => 'show', :title => 'hi', :day => 10, :year => 2005, :month => 6, :use_route => :article, :only_path => false},
+ "http://named.route.test/page/2005/6/10/hi",
x.send(:article_url, :title => 'hi', :day => 10, :year => 2005, :month => 6)
)
end
@@ -408,8 +419,8 @@ class LegacyRouteSetTests < Test::Unit::TestCase
assert_equal '/test', rs.generate(:controller => 'post', :action => 'show')
assert_equal '/test', rs.generate(:controller => 'post', :action => 'show', :year => nil)
- x = setup_for_named_route.new
- assert_equal({:controller => 'post', :action => 'show', :use_route => :blog, :only_path => false},
+ x = setup_for_named_route
+ assert_equal("http://named.route.test/test",
x.send(:blog_url))
end
@@ -455,8 +466,8 @@ class LegacyRouteSetTests < Test::Unit::TestCase
assert_equal '/', rs.generate(:controller => 'content', :action => 'index')
assert_equal '/', rs.generate(:controller => 'content')
- x = setup_for_named_route.new
- assert_equal({:controller => 'content', :action => 'index', :use_route => :home, :only_path => false},
+ x = setup_for_named_route
+ assert_equal("http://named.route.test",
x.send(:home_url))
end
@@ -571,6 +582,17 @@ class LegacyRouteSetTests < Test::Unit::TestCase
ensure
Object.send(:remove_const, :SubpathBooksController) rescue nil
end
+
+ def test_failed_requirements_raises_exception_with_violated_requirements
+ rs.draw do |r|
+ r.foo_with_requirement 'foos/:id', :controller=>'foos', :requirements=>{:id=>/\d+/}
+ end
+
+ x = setup_for_named_route
+ assert_raises(ActionController::RoutingError) do
+ x.send(:foo_with_requirement_url, "I am Against the requirements")
+ end
+ end
end
class SegmentTest < Test::Unit::TestCase
@@ -840,7 +862,46 @@ class ControllerSegmentTest < Test::Unit::TestCase
end
uses_mocha 'RouteTest' do
-
+
+ class MockController
+ attr_accessor :routes
+
+ def initialize(routes)
+ self.routes = routes
+ end
+
+ def url_for(options)
+ only_path = options.delete(:only_path)
+ path = routes.generate(options)
+ only_path ? path : "http://named.route.test#{path}"
+ end
+
+ def request
+ @request ||= MockRequest.new(:host => "named.route.test", :method => :get)
+ end
+ end
+
+ class MockRequest
+ attr_accessor :path, :path_parameters, :host, :subdomains, :domain, :method
+
+ def initialize(values={})
+ values.each { |key, value| send("#{key}=", value) }
+ if values[:host]
+ subdomain, self.domain = values[:host].split(/\./, 2)
+ self.subdomains = [subdomain]
+ end
+ end
+
+ def protocol
+ "http://"
+ end
+
+ def host_with_port
+ (subdomains * '.') + '.' + domain
+ end
+
+ end
+
class RouteTest < Test::Unit::TestCase
def setup
@@ -1302,32 +1363,10 @@ class RouteBuilderTest < Test::Unit::TestCase
end
-class RouteSetTest < Test::Unit::TestCase
- class MockController
- attr_accessor :routes
-
- def initialize(routes)
- self.routes = routes
- end
- def url_for(options)
- only_path = options.delete(:only_path)
- path = routes.generate(options)
- only_path ? path : "http://named.route.test#{path}"
- end
- end
- class MockRequest
- attr_accessor :path, :path_parameters, :host, :subdomains, :domain, :method
- def initialize(values={})
- values.each { |key, value| send("#{key}=", value) }
- if values[:host]
- subdomain, self.domain = values[:host].split(/\./, 2)
- self.subdomains = [subdomain]
- end
- end
- end
+class RouteSetTest < Test::Unit::TestCase
def set
@set ||= ROUTING::RouteSet.new
@@ -1458,10 +1497,10 @@ class RouteSetTest < Test::Unit::TestCase
controller.send(:multi_url, 7, "hello", 5, :baz => "bar")
end
- def test_named_route_url_method_with_ordered_parameters_and_hash_ordered_parameters_override_hash
+ def test_named_route_url_method_with_no_positional_arguments
controller = setup_named_route_test
- assert_equal "http://named.route.test/people/go/7/hello/joe/5?baz=bar",
- controller.send(:multi_url, 7, "hello", 5, :foo => 666, :baz => "bar")
+ assert_equal "http://named.route.test/people?baz=bar",
+ controller.send(:index_url, :baz => "bar")
end
def test_draw_default_route
--
cgit v1.2.3