Changeset 7421

Timestamp:

09/09/07 00:18:55 (1 year ago)

Author:

nzkoz

Message:

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.

Files:

• trunk/actionpack/CHANGELOG (modified) (1 diff)
• trunk/actionpack/lib/action_controller/integration.rb (modified) (1 diff)
• trunk/actionpack/lib/action_controller/routing_optimisation.rb (added)
• trunk/actionpack/lib/action_controller/routing.rb (modified) (10 diffs)
• trunk/actionpack/test/controller/resources_test.rb (modified) (1 diff)
• trunk/actionpack/test/controller/routing_test.rb (modified) (12 diffs)

trunk/actionpack/CHANGELOG

@@ -1 +1 @@
-*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]

trunk/actionpack/lib/action_controller/integration.rb

@@ -75 +75 @@
-        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)

trunk/actionpack/lib/action_controller/routing.rb

@@ -2 +2 @@
-require 'uri'
-require 'action_controller/polymorphic_routes'
+require 'action_controller/routing_optimisation'
-
-class Object
@@ -255 +256 @@
-    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.
@@ -326 +332 @@
-  
-    class Route #:nodoc:
-
+, :optimise
-      
-      def initialize
@@ -332 +338 @@
-        @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
-  
@@ -382 +400 @@
-        requirement_conditions * ' && ' unless requirement_conditions.empty?
-      end
+      
-      def generation_structure
-        segments.last.string_structure segments[0..-2]
@@ -978 +997 @@
-
-        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]
@@ -1052 +1077 @@
-      class NamedRouteCollection #:nodoc:
-        include Enumerable
-
+
-        attr_reader :routes, :helpers
-
@@ -1129 +1154 @@
-          def define_url_helper(route, name, kind, options)
-            selector = url_helper_name(name, kind)
+            # The segment keys used for positional paramters
-            
-            # 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 || {}
@@ -1155 +1180 @@
-                  #
-                  options = args.last.is_a?(Hash) ? args.pop : {}
-
+route.
-                    h[k] = v
-                    h
@@ -1168 +1193 @@
-            helpers << selector
-          end
-          
-      end
-  

trunk/actionpack/test/controller/resources_test.rb

@@ -27 +27 @@
-
-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,

trunk/actionpack/test/controller/routing_test.rb

@@ -48 +48 @@
-  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' }
@@ -167 +169 @@
-  def test_basic_named_route
-    rs.add_named_route :home, '', :controller => 'content', :action => 'list' 
-.new
-{:controller => 'content', :action => 'list', :use_route => :home, :only_path => false}
+
+"http://named.route.test"
-                 x.send(:home_url))
-  end
@@ -174 +176 @@
-  def test_named_route_with_option
-    rs.add_named_route :page, 'page/:title', :controller => 'content', :action => 'show_page'
-.new
-{:controller => 'content', :action => 'show_page', :title => 'new stuff', :use_route => :page, :only_path => false}
+
+"http://named.route.test/page/new%20stuff"
-                 x.send(:page_url, :title => 'new stuff'))
-  end
@@ -181 +183 @@
-  def test_named_route_with_default
-    rs.add_named_route :page, 'page/:title', :controller => 'content', :action => 'show_page', :title => 'AboutPage'
-
-
-
-
+
+
-                 x.send(:page_url, :title => "AboutRails"))
-
@@ -191 +191 @@
-  def test_named_route_with_nested_controller
-    rs.add_named_route :users, 'admin/user', :controller => '/admin/user', :action => 'index'
-.new
-{:controller => '/admin/user', :action => 'index', :use_route => :users, :only_path => false}
+
+"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
-
-
-
-
+
+
+
-  end
-
@@ -215 +226 @@
-      map.connect ':controller/:action/:id'
-    end
-
+
+
+
+
+
-    assert_equal(
-
-
-
-
-
+
-      x.send(:article_url, :title => 'hi', :day => 10, :year => 2005, :month => 6)
-    )
@@ -409 +420 @@
-    assert_equal '/test', rs.generate(:controller => 'post', :action => 'show', :year => nil)
-    
-.new
-{:controller => 'post', :action => 'show', :use_route => :blog, :only_path => false}
+
+"http://named.route.test/test"
-                 x.send(:blog_url))
-  end
@@ -456 +467 @@
-    assert_equal '/', rs.generate(:controller => 'content')
-    
-.new
-{:controller => 'content', :action => 'index', :use_route => :home, :only_path => false}
+
+"http://named.route.test"
-                 x.send(:home_url))
-  end
@@ -571 +582 @@
-  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
@@ -841 +863 @@
-
-uses_mocha 'RouteTest' do
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-class RouteTest < Test::Unit::TestCase
-
@@ -1303 +1364 @@
-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
-
-  def set
@@ -1459 +1498 @@
-  end
-  
-ordered_parameters_and_hash_ordered_parameters_override_hash
+no_positional_arguments
-    controller = setup_named_route_test
-/go/7/hello/joe/5
-multi_url, 7, "hello", 5, :foo => 666
+
+index_url
-  end
-