From 1fc735e5f584b481eba85670c519731271ac1796 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 11 Apr 2019 17:14:16 -0700 Subject: De-dup Templates, introduce UnboundTemplate Previously it's possible to have multiple copies of the "same" Template. For example, if index.html.erb is found both the :en and :fr locale, it will return a different Template object for each. The same can happen with formats, variants, and handlers. This commit de-duplicates templates, there will now only be one template per file/virtual_path/locals tuple. We need to consider virtual_path because both `render "index"`, and `render "index.html"` can both find the same file but will have different virtual_paths. IMO this is rare and should be deprecated/removed, but it exists now so we need to consider it in order to cache correctly. This commit introduces a new UnboundTemplate class, which represents a template with unknown locals. Template objects can be built from it by using `#with_locals`. Currently, this is just a convenience around caching templates, but I hope it's a helpful concept that could have more utility in the future. --- actionview/lib/action_view.rb | 1 + actionview/lib/action_view/template/resolver.rb | 29 ++++++++++++++-------- actionview/lib/action_view/unbound_template.rb | 32 +++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 actionview/lib/action_view/unbound_template.rb (limited to 'actionview/lib') diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb index 5ee14bfc78..7f85bf2a5e 100644 --- a/actionview/lib/action_view.rb +++ b/actionview/lib/action_view.rb @@ -44,6 +44,7 @@ module ActionView autoload :Rendering autoload :RoutingUrlFor autoload :Template + autoload :UnboundTemplate autoload :ViewPaths autoload_under "renderer" do diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index e291dc268a..1be82f5df5 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -169,6 +169,12 @@ module ActionView else @pattern = DEFAULT_PATTERN end + @unbound_templates = Concurrent::Map.new + super() + end + + def clear_cache + @unbound_templates.clear super() end @@ -189,16 +195,19 @@ module ActionView end def build_template(template, virtual_path, locals) - handler, format, variant = extract_handler_and_format_and_variant(template) - - filename = File.expand_path(template) - source = Template::Sources::File.new(filename) - Template.new(source, filename, handler, - virtual_path: virtual_path, - format: format, - variant: variant, - locals: locals - ) + @unbound_templates.compute_if_absent([template, virtual_path]) do + handler, format, variant = extract_handler_and_format_and_variant(template) + source = Template::Sources::File.new(template) + + UnboundTemplate.new( + source, + template, + handler, + virtual_path: virtual_path, + format: format, + variant: variant, + ) + end.bind_locals(locals) end def reject_files_external_to_app(files) diff --git a/actionview/lib/action_view/unbound_template.rb b/actionview/lib/action_view/unbound_template.rb new file mode 100644 index 0000000000..db69b6d016 --- /dev/null +++ b/actionview/lib/action_view/unbound_template.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require "concurrent/map" + +module ActionView + class UnboundTemplate + def initialize(source, identifer, handler, options) + @source = source + @identifer = identifer + @handler = handler + @options = options + + @templates = Concurrent::Map.new(initial_capacity: 2) + end + + def bind_locals(locals) + @templates[locals] ||= build_template(locals) + end + + private + + def build_template(locals) + options = @options.merge(locals: locals) + Template.new( + @source, + @identifer, + @handler, + options + ) + end + end +end -- cgit v1.2.3 From 53e4055a75fc16bf31aeb295ec18c07a3059db57 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 12 Apr 2019 10:20:04 -0700 Subject: Support disabling cache for Digestor This adds a bit of complexity, but is necessary for now to avoid holding extra copies of templates which are resolved from ActionView::Digestor after disabling cache on the lookup context. --- actionview/lib/action_view/template/resolver.rb | 55 ++++++++++++++++--------- actionview/lib/action_view/testing/resolvers.rb | 4 +- 2 files changed, 37 insertions(+), 22 deletions(-) (limited to 'actionview/lib') diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 1be82f5df5..6a9beede8c 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -178,36 +178,51 @@ module ActionView super() end + def find_all(name, prefix = nil, partial = false, details = {}, key = nil, locals = []) + locals = locals.map(&:to_s).sort!.freeze + + cached(key, [name, prefix, partial], details, locals) do + find_templates(name, prefix, partial, details, locals, cache: !!key) + end + end + private - def find_templates(name, prefix, partial, details, locals) + def find_templates(name, prefix, partial, details, locals, cache: true) path = Path.build(name, prefix, partial) - query(path, details, details[:formats], locals) + query(path, details, details[:formats], locals, cache: cache) end - def query(path, details, formats, locals) + def query(path, details, formats, locals, cache:) template_paths = find_template_paths_from_details(path, details) template_paths = reject_files_external_to_app(template_paths) template_paths.map do |template| - build_template(template, path.virtual, locals) + unbound_template = + if cache + @unbound_templates.compute_if_absent([template, path.virtual]) do + build_unbound_template(template, path.virtual) + end + else + build_unbound_template(template, path.virtual) + end + + unbound_template.bind_locals(locals) end end - def build_template(template, virtual_path, locals) - @unbound_templates.compute_if_absent([template, virtual_path]) do - handler, format, variant = extract_handler_and_format_and_variant(template) - source = Template::Sources::File.new(template) - - UnboundTemplate.new( - source, - template, - handler, - virtual_path: virtual_path, - format: format, - variant: variant, - ) - end.bind_locals(locals) + def build_unbound_template(template, virtual_path) + handler, format, variant = extract_handler_and_format_and_variant(template) + source = Template::Sources::File.new(template) + + UnboundTemplate.new( + source, + template, + handler, + virtual_path: virtual_path, + format: format, + variant: variant, + ) end def reject_files_external_to_app(files) @@ -372,8 +387,8 @@ module ActionView [new(""), new("/")] end - def build_template(template, virtual_path, locals) - super(template, nil, locals) + def build_unbound_template(template, _) + super(template, nil) end def reject_files_external_to_app(files) diff --git a/actionview/lib/action_view/testing/resolvers.rb b/actionview/lib/action_view/testing/resolvers.rb index a97fb71b26..1bedf44934 100644 --- a/actionview/lib/action_view/testing/resolvers.rb +++ b/actionview/lib/action_view/testing/resolvers.rb @@ -23,7 +23,7 @@ module ActionView #:nodoc: private - def query(path, exts, _, locals) + def query(path, exts, _, locals, cache:) query = +"" EXTENSIONS.each do |ext, prefix| query << "(" << exts[ext].map { |e| e && Regexp.escape("#{prefix}#{e}") }.join("|") << "|)" @@ -47,7 +47,7 @@ module ActionView #:nodoc: end class NullResolver < PathResolver - def query(path, exts, _, locals) + def query(path, exts, _, locals, cache:) handler, format, variant = extract_handler_and_format_and_variant(path) [ActionView::Template.new("Template generated by Null Resolver", path.virtual, handler, virtual_path: path.virtual, format: format, variant: variant, locals: locals)] end -- cgit v1.2.3 From 0b26cc41dbd8c57eb382318a63c4272de3d2e4e9 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 12 Apr 2019 15:40:33 -0700 Subject: Avoid duplication using _find_all --- actionview/lib/action_view/template/resolver.rb | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) (limited to 'actionview/lib') diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 6a9beede8c..ce53eb046d 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -118,7 +118,7 @@ module ActionView locals = locals.map(&:to_s).sort!.freeze cached(key, [name, prefix, partial], details, locals) do - find_templates(name, prefix, partial, details, locals) + _find_all(name, prefix, partial, details, key, locals) end end @@ -131,6 +131,10 @@ module ActionView private + def _find_all(name, prefix, partial, details, key, locals) + find_templates(name, prefix, partial, details, locals) + end + delegate :caching?, to: :class # This is what child classes implement. No defaults are needed @@ -178,19 +182,11 @@ module ActionView super() end - def find_all(name, prefix = nil, partial = false, details = {}, key = nil, locals = []) - locals = locals.map(&:to_s).sort!.freeze - - cached(key, [name, prefix, partial], details, locals) do - find_templates(name, prefix, partial, details, locals, cache: !!key) - end - end - private - def find_templates(name, prefix, partial, details, locals, cache: true) + def _find_all(name, prefix, partial, details, key, locals) path = Path.build(name, prefix, partial) - query(path, details, details[:formats], locals, cache: cache) + query(path, details, details[:formats], locals, cache: !!key) end def query(path, details, formats, locals, cache:) -- cgit v1.2.3