Permalink
Browse files

Resolvers now consider timestamps.

Before this patch, every request in development caused the template
to be compiled, regardless if it was updated in the filesystem or not.
This patch now checks the timestamp and only compiles it again if
any change was done.

While this probably won't show any difference for current setups,
but it will be useful for asset template handlers (like SASS), as
compiling their templates is slower than ERb, Haml, etc.
  • Loading branch information...
1 parent c7408a0 commit 38d78f99d52801d8392a7229b40edae74cc3d142 @josevalim josevalim committed Oct 10, 2010
View
25 actionpack/lib/action_view/template.rb
@@ -98,10 +98,9 @@ class Template
extend Template::Handlers
- attr_accessor :locals
+ attr_accessor :locals, :formats, :virtual_path
- attr_reader :source, :identifier, :handler, :virtual_path, :formats,
- :original_encoding
+ attr_reader :source, :identifier, :handler, :original_encoding, :updated_at
# This finalizer is needed (and exactly with a proc inside another proc)
# otherwise templates leak in development.
@@ -114,15 +113,17 @@ class Template
end
def initialize(source, identifier, handler, details)
- @source = source
- @identifier = identifier
- @handler = handler
- @original_encoding = nil
- @method_names = {}
- @locals = details[:locals] || []
- @formats = Array.wrap(details[:format] || :html).map(&:to_sym)
- @virtual_path = details[:virtual_path]
- @compiled = false
+ format = details[:format] || (handler.default_format if handler.respond_to?(:default_format))
+
+ @source = source
+ @identifier = identifier
+ @handler = handler
+ @compiled = false
+ @original_encoding = nil
+ @locals = details[:locals] || []
+ @virtual_path = details[:virtual_path]
+ @updated_at = details[:updated_at] || Time.now
+ @formats = Array.wrap(format).map(&:to_sym)
end
# Render a template. If the template was not compiled yet, it is done
View
67 actionpack/lib/action_view/template/resolver.rb
@@ -16,7 +16,7 @@ def clear_cache
# Normalizes the arguments and passes it on to find_template.
def find_all(name, prefix=nil, partial=false, details={}, locals=[], key=nil)
- cached(key, prefix, name, partial, locals) do
+ cached(key, [name, prefix, partial], details, locals) do
find_templates(name, prefix, partial, details)
end
end
@@ -35,37 +35,55 @@ def find_templates(name, prefix, partial, details)
end
# Helpers that builds a path. Useful for building virtual paths.
- def build_path(name, prefix, partial, details)
+ def build_path(name, prefix, partial)
path = ""
path << "#{prefix}/" unless prefix.empty?
path << (partial ? "_#{name}" : name)
path
end
- # Get the handler and format from the given parameters.
- def retrieve_handler_and_format(handler, format, default_formats=nil)
- handler = Template.handler_class_for_extension(handler)
- format = format && Mime[format]
- format ||= handler.default_format if handler.respond_to?(:default_format)
- format ||= default_formats
- [handler, format]
- end
-
- def cached(key, prefix, name, partial, locals)
+ # Hnadles templates caching. If a key is given and caching is on
+ # always check the cache before hitting the resolver. Otherwise,
+ # it always hits the resolver but check if the resolver is fresher
+ # before returning it.
+ def cached(key, path_info, details, locals) #:nodoc:
+ name, prefix, partial = path_info
locals = sort_locals(locals)
- unless key && caching?
- yield.each { |t| t.locals = locals }
+
+ if key && caching?
+ @cached[key][name][prefix][partial][locals] ||= decorate(yield, path_info, details, locals)
else
- @cached[key][prefix][name][partial][locals] ||= yield.each { |t| t.locals = locals }
+ fresh = decorate(yield, path_info, details, locals)
+ return fresh unless key
+
+ scope = @cached[key][name][prefix][partial]
+ cache = scope[locals]
+ mtime = cache && cache.map(&:updated_at).max
+
+ if !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime }
+ scope[locals] = fresh
+ else
+ cache
+ end
+ end
+ end
+
+ # Ensures all the resolver information is set in the template.
+ def decorate(templates, path_info, details, locals) #:nodoc:
+ cached = nil
+ templates.each do |t|
+ t.locals = locals
+ t.formats = details[:formats] || [:html] if t.formats.empty?
+ t.virtual_path ||= (cached ||= build_path(*path_info))
end
end
- if :locale.respond_to?("<=>")
- def sort_locals(locals)
+ if :symbol.respond_to?("<=>")
+ def sort_locals(locals) #:nodoc:
locals.sort.freeze
end
else
- def sort_locals(locals)
+ def sort_locals(locals) #:nodoc:
locals = locals.map{ |l| l.to_s }
locals.sort!
locals.freeze
@@ -79,7 +97,7 @@ class PathResolver < Resolver
private
def find_templates(name, prefix, partial, details)
- path = build_path(name, prefix, partial, details)
+ path = build_path(name, prefix, partial)
query(path, EXTENSION_ORDER.map { |ext| details[ext] }, details[:formats])
end
@@ -96,17 +114,24 @@ def query(path, exts, formats)
contents = File.open(p, "rb") {|io| io.read }
Template.new(contents, File.expand_path(p), handler,
- :virtual_path => path, :format => format)
+ :virtual_path => path, :format => format, :updated_at => mtime(p))
end
end
+ # Returns the file mtime from the filesystem.
+ def mtime(p)
+ File.stat(p).mtime
+ end
+
# Extract handler and formats from path. If a format cannot be a found neither
# from the path, or the handler, we should return the array of formats given
# to the resolver.
def extract_handler_and_format(path, default_formats)
pieces = File.basename(path).split(".")
pieces.shift
- retrieve_handler_and_format(pieces.pop, pieces.pop, default_formats)
+ handler = Template.handler_class_for_extension(pieces.pop)
+ format = pieces.last && Mime[pieces.last]
+ [handler, format]
end
end
View
5 actionpack/lib/action_view/testing/resolvers.rb
@@ -23,11 +23,12 @@ def query(path, exts, formats)
query = /^(#{Regexp.escape(path)})#{query}$/
templates = []
- @hash.each do |_path, source|
+ @hash.each do |_path, array|
+ source, updated_at = array
next unless _path =~ query
handler, format = extract_handler_and_format(_path, formats)
templates << Template.new(source, _path, handler,
- :virtual_path => $1, :format => format)
+ :virtual_path => $1, :format => format, :updated_at => updated_at)
end
templates.sort_by {|t| -t.identifier.match(/^#{query}$/).captures.reject(&:blank?).size }
View
45 actionpack/test/template/lookup_context_test.rb
@@ -184,4 +184,49 @@ def teardown
test "can have cache disabled on initialization" do
assert !ActionView::LookupContext.new(FIXTURE_LOAD_PATH, :cache => false).cache
end
+end
+
+class LookupContextWithFalseCaching < ActiveSupport::TestCase
+ def setup
+ @resolver = ActionView::FixtureResolver.new("test/_foo.erb" => ["Foo", Time.utc(2000)])
+ @resolver.stubs(:caching?).returns(false)
+ @lookup_context = ActionView::LookupContext.new(@resolver, {})
+ end
+
+ test "templates are always found in the resolver but timestamp is checked before being compiled" do
+ template = @lookup_context.find("foo", "test", true)
+ assert_equal "Foo", template.source
+
+ # Now we are going to change the template, but it won't change the returned template
+ # since the timestamp is the same.
+ @resolver.hash["test/_foo.erb"][0] = "Bar"
+ template = @lookup_context.find("foo", "test", true)
+ assert_equal "Foo", template.source
+
+ # Now update the timestamp.
+ @resolver.hash["test/_foo.erb"][1] = Time.now.utc
+ template = @lookup_context.find("foo", "test", true)
+ assert_equal "Bar", template.source
+ end
+
+ test "if no template was found in the second lookup, give it higher preference" do
+ template = @lookup_context.find("foo", "test", true)
+ assert_equal "Foo", template.source
+
+ @resolver.hash.clear
+ assert_raise ActionView::MissingTemplate do
+ @lookup_context.find("foo", "test", true)
+ end
+ end
+
+ test "if no template was cached in the first lookup, do not use the cache in the second" do
+ @resolver.hash.clear
+ assert_raise ActionView::MissingTemplate do
+ @lookup_context.find("foo", "test", true)
+ end
+
+ @resolver.hash["test/_foo.erb"] = ["Foo", Time.utc(2000)]
+ template = @lookup_context.find("foo", "test", true)
+ assert_equal "Foo", template.source
+ end
end

2 comments on commit 38d78f9

@jfirebaugh

José, this is causing an incompatibility for the Erector templating engine. Erector templates are pure Ruby classes, and the Erector TemplateHandler uses require_dependency to load the template .rb file so that the class constant gets unloaded after each request in development mode. With this change, the class constant still gets unloaded, but the template no longer gets recompiled, so on the next request the constant is missing.

Any thoughts on how to fix this?

@josevalim
Ruby on Rails member

I believe the best option for erector would be to define an object space finalized that would remove the class when the template object is garbage collected, this would at least allow you to have the performance benefits of this commit and get rid of require_dependency.

Please sign in to comment.