From 68cda695da27f57cae682d160a13dab4dacb1ef8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Mon, 8 Mar 2010 16:32:40 +0100 Subject: [PATCH] Speed up performance in resolvers by adding fallbacks just when required. --- .../lib/abstract_controller/rendering.rb | 14 ++---- actionpack/lib/action_view/base.rb | 46 ++++++++----------- actionpack/lib/action_view/lookup_context.rb | 16 +++++++ actionpack/lib/action_view/paths.rb | 2 +- actionpack/lib/action_view/render/layouts.rb | 3 +- .../lib/action_view/render/rendering.rb | 2 +- .../lib/action_view/template/resolver.rb | 31 +++---------- .../test/template/compiled_templates_test.rb | 2 +- actionpack/test/template/render_test.rb | 6 +-- 9 files changed, 53 insertions(+), 69 deletions(-) diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 356f1ec7f6..840f0b6c78 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -142,8 +142,8 @@ module AbstractController private - # Normalize options, by converting render "foo" to render :template => "prefix/foo" - # and render "/foo" to render :file => "/foo". + # Normalize options by converting render "foo" to render :action => "foo" and + # render "foo/bar" to render :file => "foo/bar". def _normalize_args(action=nil, options={}) case action when NilClass @@ -151,14 +151,8 @@ module AbstractController options, action = action, nil when String, Symbol action = action.to_s - case action.index("/") - when NilClass - options[:action] = action - when 0 - options[:file] = action - else - options[:template] = action - end + key = action.include?(?/) ? :file : :action + options[key] = action else options.merge!(:partial => action) end diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index d18f524060..d2b77b2560 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -173,48 +173,41 @@ module ActionView #:nodoc: module Subclasses end - include Helpers, Rendering, Partials, Layouts, ::ERB::Util - - extend ActiveSupport::Memoizable - - attr_accessor :base_path, :assigns, :template_extension - attr_internal :captures - - class << self - delegate :erb_trim_mode=, :to => 'ActionView::Template::Handlers::ERB' - delegate :logger, :to => 'ActionController::Base', :allow_nil => true - end + include Helpers, Rendering, Partials, Layouts, ::ERB::Util, Context + extend ActiveSupport::Memoizable # Specify whether RJS responses should be wrapped in a try/catch block # that alert()s the caught exception (and then re-raises it). cattr_accessor :debug_rjs @@debug_rjs = false - # :nodoc: - def self.xss_safe? - true + class_attribute :helpers + attr_reader :helpers + + class << self + delegate :erb_trim_mode=, :to => 'ActionView::Template::Handlers::ERB' + delegate :logger, :to => 'ActionController::Base', :allow_nil => true end - attr_internal :request, :layout + attr_accessor :base_path, :assigns, :template_extension, :lookup_context + attr_internal :captures, :request, :layout, :controller, :template, :config - def controller_path - @controller_path ||= controller && controller.controller_path - end + delegate :find_template, :template_exists?, :formats, :formats=, + :view_paths, :view_paths=, :with_fallbacks, :update_details, :to => :lookup_context delegate :request_forgery_protection_token, :template, :params, :session, :cookies, :response, :headers, :flash, :action_name, :controller_name, :to => :controller delegate :logger, :to => :controller, :allow_nil => true - include Context + def self.xss_safe? #:nodoc: + true + end def self.process_view_paths(value) ActionView::PathSet.new(Array(value)) end - class_attribute :helpers - attr_reader :helpers - def self.for_controller(controller) @views ||= {} @@ -260,12 +253,9 @@ module ActionView #:nodoc: @lookup_context.formats = formats if formats end - attr_internal :controller, :template, :config - - attr_reader :lookup_context - - delegate :find_template, :template_exists?, :formats, :formats=, - :view_paths, :view_paths=, :update_details, :to => :lookup_context + def controller_path + @controller_path ||= controller && controller.controller_path + end def punctuate_body!(part) flush_output_buffer diff --git a/actionpack/lib/action_view/lookup_context.rb b/actionpack/lib/action_view/lookup_context.rb index 82aebe1678..e259a78c5c 100644 --- a/actionpack/lib/action_view/lookup_context.rb +++ b/actionpack/lib/action_view/lookup_context.rb @@ -6,6 +6,9 @@ module ActionView class LookupContext #:nodoc: attr_reader :details, :view_paths + mattr_accessor :fallbacks + @@fallbacks = [FileSystemResolver.new(""), FileSystemResolver.new("/")] + class DetailsKey #:nodoc: attr_reader :details alias :eql? :equal? @@ -69,6 +72,19 @@ module ActionView end end + # Added fallbacks to the view paths. Useful in cases you are rendering a file. + def with_fallbacks + added_resolvers = 0 + self.class.fallbacks.each do |resolver| + next if view_paths.include?(resolver) + view_paths.push(resolver) + added_resolvers += 1 + end + yield + ensure + added_resolvers.times { view_paths.pop } + end + def find_template(name, prefix = nil, partial = false) @view_paths.find(name, details, prefix, partial || false, details_key) end diff --git a/actionpack/lib/action_view/paths.rb b/actionpack/lib/action_view/paths.rb index 154a79a8f1..628546e443 100644 --- a/actionpack/lib/action_view/paths.rb +++ b/actionpack/lib/action_view/paths.rb @@ -33,7 +33,7 @@ module ActionView #:nodoc: def typecast! each_with_index do |path, i| next unless path.is_a?(String) - self[i] = FileSystemResolverWithFallback.new(path) + self[i] = FileSystemResolver.new(path) end end end diff --git a/actionpack/lib/action_view/render/layouts.rb b/actionpack/lib/action_view/render/layouts.rb index b9c63e0090..91a92a833a 100644 --- a/actionpack/lib/action_view/render/layouts.rb +++ b/actionpack/lib/action_view/render/layouts.rb @@ -48,7 +48,8 @@ module ActionView # the given name exists across all details before raising the error. def _find_layout(layout) #:nodoc: begin - find_template(layout) + layout =~ /^\// ? + with_fallbacks { find_template(layout) } : find_template(layout) rescue ActionView::MissingTemplate => e update_details(:formats => nil) do raise unless template_exists?(layout) diff --git a/actionpack/lib/action_view/render/rendering.rb b/actionpack/lib/action_view/render/rendering.rb index 96c0b4fe6a..d11f1fa2a7 100644 --- a/actionpack/lib/action_view/render/rendering.rb +++ b/actionpack/lib/action_view/render/rendering.rb @@ -63,7 +63,7 @@ module ActionView elsif options.key?(:text) Template::Text.new(options[:text], self.formats.try(:first)) elsif options.key?(:file) - find_template(options[:file], options[:_prefix]) + with_fallbacks { find_template(options[:file], options[:_prefix]) } elsif options.key?(:template) find_template(options[:template], options[:_prefix]) end diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index 40326f9193..fde13a4bdf 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -4,7 +4,6 @@ require "active_support/core_ext/array/wrap" require "action_view/template" module ActionView - # Abstract superclass class Resolver class_inheritable_accessor(:registered_details) @@ -30,7 +29,7 @@ module ActionView find_all(*args).first end - # Normalizes the arguments and passes it on to find_template + # Normalizes the arguments and passes it on to find_template. def find_all(name, details = {}, prefix = nil, partial = nil) details = normalize_details(details) name, prefix = normalize_name(name, prefix) @@ -82,21 +81,20 @@ module ActionView end class PathResolver < Resolver - EXTENSION_ORDER = [:locale, :formats, :handlers] def to_s @path.to_s end - alias to_path to_s + alias :to_path :to_s + + private def find_templates(name, details, prefix, partial) path = build_path(name, details, prefix, partial) query(path, EXTENSION_ORDER.map { |ext| details[ext] }) end - private - def build_path(name, details, prefix, partial) path = "" path << "#{prefix}/" unless prefix.empty? @@ -141,25 +139,10 @@ module ActionView super() @path = Pathname.new(path).expand_path end - end - # TODO: remove hack - class FileSystemResolverWithFallback < Resolver - def initialize(path) - super() - @paths = [FileSystemResolver.new(path), FileSystemResolver.new(""), FileSystemResolver.new("/")] - end - - def find_templates(*args) - @paths.each do |p| - template = p.find_templates(*args) - return template unless template.empty? - end - [] - end - - def to_s - @paths.first.to_s + def eql?(resolver) + self.class.equal?(resolver.class) && to_path == resolver.to_path end + alias :== :eql? end end diff --git a/actionpack/test/template/compiled_templates_test.rb b/actionpack/test/template/compiled_templates_test.rb index 632988bb2e..0a3409064f 100644 --- a/actionpack/test/template/compiled_templates_test.rb +++ b/actionpack/test/template/compiled_templates_test.rb @@ -44,7 +44,7 @@ class CompiledTemplatesTest < Test::Unit::TestCase end def render_without_cache(*args) - path = ActionView::FileSystemResolverWithFallback.new(FIXTURE_LOAD_PATH) + path = ActionView::FileSystemResolver.new(FIXTURE_LOAD_PATH) view_paths = ActionView::Base.process_view_paths(path) ActionView::Base.new(view_paths, {}).render(*args) end diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index 6dbadc9304..37a3975a54 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -270,7 +270,7 @@ class CachedViewRenderTest < ActiveSupport::TestCase # Ensure view path cache is primed def setup view_paths = ActionController::Base.view_paths - assert_equal ActionView::FileSystemResolverWithFallback, view_paths.first.class + assert_equal ActionView::FileSystemResolver, view_paths.first.class setup_view(view_paths) end end @@ -281,9 +281,9 @@ class LazyViewRenderTest < ActiveSupport::TestCase # Test the same thing as above, but make sure the view path # is not eager loaded def setup - path = ActionView::FileSystemResolverWithFallback.new(FIXTURE_LOAD_PATH) + path = ActionView::FileSystemResolver.new(FIXTURE_LOAD_PATH) view_paths = ActionView::Base.process_view_paths(path) - assert_equal ActionView::FileSystemResolverWithFallback, view_paths.first.class + assert_equal ActionView::FileSystemResolver.new(FIXTURE_LOAD_PATH), view_paths.first setup_view(view_paths) end end