Description
Summary
The render method inside the Jekyll::Tags::Link will call Liquid rendering many times, even when it had rendered the object before.
Motivation
While working on #9776 , @ashmaroli noticed a possible performance regression (#9776 (comment)) in my PR.
The current code for rendering a {% link %} element suffers from the same thing.
Reference-level explanation
The idea is to avoid rendering an already rendered object, and to avoid calling rendering Liquid unnecessarily.
The current render for {% link %} is:
def render(context)
@context = context
site = context.registers[:site]
relative_path = Liquid::Template.parse(@relative_path).render(context)
relative_path_with_leading_slash = PathManager.join("", relative_path)
site.each_site_file do |item|
return relative_url(item) if item.relative_path == relative_path
# This takes care of the case for static files that have a leading /
return relative_url(item) if item.relative_path == relative_path_with_leading_slash
end
raise ArgumentError, <<~MSG
Could not find document '#{relative_path}' in tag '#{self.class.tag_name}'.
Make sure the document exists and the path is correct.
MSG
endIf it is detected that this is first rendering, then do this. Else, just reuse previous value. For instance, something like this:
def render(context)
@context = context
return @rendered_value unless @rendered_value.nil?
@rendered_value = first_render
return @rendered_value
end
private
def first_render
site = @context.registers[:site]
relative_path = expand_path
relative_path_with_leading_slash = PathManager.join("", relative_path)
site.each_site_file do |item|
return relative_url(item) if item.relative_path == relative_path
# This takes care of the case for static files that have a leading /
return relative_url(item) if item.relative_path == relative_path_with_leading_slash
end
raise ArgumentError, <<~MSG
Could not find document '#{relative_path}' in tag '#{self.class.tag_name}'.
Make sure the document exists and the path is correct.
MSG
end
def expand_path
content = @orig_post
return content if content.nil? || content.empty?
return content unless content.include?("{%") || content.include?("{{")
Liquid::Template.parse(content).render(@context)
end
def solve_path
content = @relative_path
return content if content.nil? || content.empty?
return content unless content.include?("{%") || content.include?("{{")
Liquid::Template.parse(content).render(@context)
endIt should not cause any changes to the outcome, this is purely a performance optimization opportunity that needs no change in API.
For example (based on #9776 (comment)), consider the following code in an include_file:
{% for entry in site.data.archives %}
<a href="{% link {{ entry.linkpath }} %}" title="{{ entry.title }}">
{{ entry.title }}
</a>
{% endfor %}Lets assume this include_file is used in two different layouts to render a sidebar.
During the build process, Liquid will parse the above code just once regardless of how many pages utilize the two layouts linking the include_file. This means that, in this particular scenario, there will only be a single instance of Jekyll::Tags::Link class but, this one instance will call upon it's render() method as many times as the number of pages dependent on the above include_file via layouts.
Note that I, the reporter of this issue, am not an expert in Jekyll and it's rendering process/pipeline. There may be something off with the optimization. Trying to give different contexts for the same tag object may break stuff?
Drawbacks
Adding a "rendering" instance variable means that each node will have a new field, which will increase a more "long lived" memory.
The wrong optimization will lead to the wrong information being presented, this should be taken extremely carefuly.