Fix duplicate view template loading

[nhasselmeyer]

Motivation / Background

Currently, when rendering an ActionView::Template, the template source is loaded from the disk twice.

During one rendering process, Template#source is called at least twice (once in encode! and once in strict_locals!), each of these calls runs @source.to_s. As @source is usually a Template::Sources::File, #to_s hits the disk to load the template.

It is unnecessary to load the file from disk more than once just to check whether the # locals: comment is present.

Detail

This Pull Request changes the Template#source so that it memoizes the loaded source.

Additional information

This pull request introduces a small behavior change: The # locals: (...) comment is now removed from the source before passing the source to the handler. It looks like this was the intended behavior all along. Previously the line

self.source.sub!(STRICT_LOCALS_REGEX, "")

had no effect, because the source was reloaded from disk afterwards and the magic comment was present again.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[willianveiga]

The tests are passing with the following alternative implementation:

diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb
index 9b7dfc2b56..6d48c55ba3 100644
--- a/actionview/lib/action_view/template.rb
+++ b/actionview/lib/action_view/template.rb
@@ -186,26 +186,26 @@ def mime_types_implementation=(implementation)
         # This method isn't thread-safe, but it's not supposed
         # to be called after initialization
         if self::Types != implementation
           remove_const(:Types)
           const_set(:Types, implementation)
         end
       end
     end
 
     attr_reader :identifier, :handler
-    attr_reader :variable, :format, :variant, :virtual_path
+    attr_reader :variable, :source, :format, :variant, :virtual_path
 
     NONE = Object.new
 
     def initialize(source, identifier, handler, locals:, format: nil, variant: nil, virtual_path: nil)
-      @source            = source.dup
+      @source            = source.to_s.dup
       @identifier        = identifier
       @handler           = handler
       @compiled          = false
       @locals            = locals
       @virtual_path      = virtual_path
 
       @variable = if @virtual_path
         base = @virtual_path.end_with?("/") ? "" : ::File.basename(@virtual_path)
         base =~ /\A_?(.*?)(?:\.\w+)*\z/
         $1.to_sym
@@ -285,24 +285,20 @@ def type
     end
 
     def short_identifier
       @short_identifier ||= defined?(Rails.root) ? identifier.delete_prefix("#{Rails.root}/") : identifier
     end
 
     def inspect
       "#<#{self.class.name} #{short_identifier} locals=#{locals.inspect}>"
     end
 
-    def source
-      @source.to_s
-    end
-
     LEADING_ENCODING_REGEXP = /\A#{ENCODING_FLAG}/
     private_constant :LEADING_ENCODING_REGEXP
 
     # This method is responsible for properly setting the encoding of the
     # source. Until this point, we assume that the source is BINARY data.
     # If no additional information is supplied, we assume the encoding is
     # the same as <tt>Encoding.default_external</tt>.
     #
     # The user can also specify the encoding via a comment on the first
     # line of the template (<tt># encoding: NAME-OF-ENCODING</tt>). This will work

Does it make sense, @nhasselmeyer? Thanks.

[nhasselmeyer]

@willianveiga I thought about implementing that, too, but there is a problem: Not every template that is initialized will need the loaded source.

If, for example, there exists a foo.html.erb and a foo+mobile.html.erb, then a

render 'foo', variants: [:mobile]`

will initialize a ActionView::Template for both of them. Later code will select the mobile variant and render only that. But with your implementation, the source for the template without a variant would be loaded, too, negating the performance win.

[skipkayhil]

I think this is a good change since we call template.source other places, like 2-3 times for the same template in the RubyTracker:

rails/actionview/lib/action_view/dependency_tracker/ruby_tracker.rb

Lines 28 to 40 in 3a91c80

	def render_dependencies
	return [] unless template.source.include?("render")
	compiled_source = template.handler.call(template, template.source)
	@parser_class.new(@name, compiled_source).render_calls.filter_map do |render_call|
	render_call.gsub(%r|/_|, "/")
	end
	end
	def explicit_dependencies
	template.source.scan(EXPLICIT_DEPENDENCY).flatten.uniq
	end

I left some suggestions for variable names, but other than that I'll try to get this merged 👍

[nhasselmeyer]

@skipkayhil Thank you for your suggestions! I edited them in and rebased the branch.

[skipkayhil]

I spoke to John about this PR and he said we shouldn't be memoizing the rendered source. After thinking about it more, this makes sense because it would greatly increase memory usage if we keep the rendered source around forever (we only need it to compile the view method and maybe calculate render dependencies and then not really after that).

Since the current approach isn't going to work, I'm going to have to close this PR. I appreciate your willingness to stick with it for so long and I'm sorry it took so long to get feedback. Thank you for contributing and I hope to see more contributions in the future 🙂