-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Profiling the rendering of a liquid template #364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| module Liquid | ||
|
|
||
| # Profiler enables support for profiling template rendering to help track down performance issues. | ||
| # | ||
| # To enable profiling, pass the <tt>profile: true</tt> option to <tt>Liquid::Template.parse</tt>. Then, after | ||
| # <tt>Liquid::Template#render</tt> is called, the template object makes available an instance of this | ||
| # class via the <tt>Liquid::Template#profiler</tt> method. | ||
| # | ||
| # template = Liquid::Template.parse(template_content, profile: true) | ||
| # output = template.render | ||
| # profile = template.profiler | ||
| # | ||
| # This object contains all profiling information, containing information on what tags were rendered, | ||
| # where in the templates these tags live, and how long each tag took to render. | ||
| # | ||
| # This is a tree structure that is Enumerable all the way down, and keeps track of tags and rendering times | ||
| # inside of <tt>{% include %}</tt> tags. | ||
| # | ||
| # profile.each do |node| | ||
| # # Access to the token itself | ||
| # node.code | ||
| # | ||
| # # Which template and line number of this node. | ||
| # # If top level, this will be "<root>". | ||
| # node.partial | ||
| # node.line_number | ||
| # | ||
| # # Render time in seconds of this node | ||
| # node.render_time | ||
| # | ||
| # # If the template used {% include %}, this node will also have children. | ||
| # node.children.each do |child2| | ||
| # # ... | ||
| # end | ||
| # end | ||
| # | ||
| # Profiler also exposes the total time of the template's render in <tt>Liquid::Profiler#total_render_time</tt>. | ||
| # | ||
| # All render times are in seconds. There is a small performance hit when profiling is enabled. | ||
| # | ||
| class Profiler | ||
| include Enumerable | ||
|
|
||
| class Timing | ||
| attr_reader :code, :partial, :line_number, :children | ||
|
|
||
| def initialize(token, partial) | ||
| @code = token.respond_to?(:raw) ? token.raw : token | ||
| @partial = partial | ||
| @line_number = token.respond_to?(:line_number) ? token.line_number : nil | ||
| @children = [] | ||
| end | ||
|
|
||
| def self.start(token, partial) | ||
| new(token, partial).tap do |t| | ||
| t.start | ||
| end | ||
| end | ||
|
|
||
| def start | ||
| @start_time = Time.now | ||
| end | ||
|
|
||
| def finish | ||
| @end_time = Time.now | ||
| end | ||
|
|
||
| def render_time | ||
| @end_time - @start_time | ||
| end | ||
| end | ||
|
|
||
| def self.profile_token_render(token) | ||
| if Profiler.current_profile && token.respond_to?(:render) | ||
| Profiler.current_profile.start_token(token) | ||
| output = yield | ||
| Profiler.current_profile.end_token(token) | ||
| output | ||
| else | ||
| yield | ||
| end | ||
| end | ||
|
|
||
| def self.profile_children(template_name) | ||
| if Profiler.current_profile | ||
| Profiler.current_profile.push_partial(template_name) | ||
| output = yield | ||
| Profiler.current_profile.pop_partial | ||
| output | ||
| else | ||
| yield | ||
| end | ||
| end | ||
|
|
||
| def self.current_profile | ||
| Thread.current[:liquid_profiler] | ||
| end | ||
|
|
||
| def initialize | ||
| @partial_stack = ["<root>"] | ||
|
|
||
| @root_timing = Timing.new("", current_partial) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's handled in the String monkey patch in lib/liquid/extensions.rb. That said this is an internal only object that the users of this object will never directly interact with. I just needed something to start with so the infinitely nestable setup worked right. |
||
| @timing_stack = [@root_timing] | ||
|
|
||
| @render_start_at = Time.now | ||
| @render_end_at = @render_start_at | ||
| end | ||
|
|
||
| def start | ||
| Thread.current[:liquid_profiler] = self | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you do it like this to make sure you are thread safe? I don't think the way you install profiling hooks is thread safe (monkey patching the methods in one thread will also add them to the classes in all other threads. might even be racy.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I'm okay with profiling not being thread safe, but we should probably mention that somewhere.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I think it will be properly thread-safe if we just install the hooks at load and leave them there, as per my other comment. Thoughts?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say that's probably okay? Not sure. @jasonhl?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, should be fine methinks. I don't think we really care about the thread safety of profiling, though. |
||
| @render_start_at = Time.now | ||
| end | ||
|
|
||
| def stop | ||
| Thread.current[:liquid_profiler] = nil | ||
| @render_end_at = Time.now | ||
| end | ||
|
|
||
| def total_render_time | ||
| @render_end_at - @render_start_at | ||
| end | ||
|
|
||
| def each(&block) | ||
| @root_timing.children.each(&block) | ||
| end | ||
|
|
||
| def [](idx) | ||
| @root_timing.children[idx] | ||
| end | ||
|
|
||
| def length | ||
| @root_timing.children.length | ||
| end | ||
|
|
||
| def start_token(token) | ||
| @timing_stack.push(Timing.start(token, current_partial)) | ||
| end | ||
|
|
||
| def end_token(token) | ||
| timing = @timing_stack.pop | ||
| timing.finish | ||
|
|
||
| @timing_stack.last.children << timing | ||
| end | ||
|
|
||
| def current_partial | ||
| @partial_stack.last | ||
| end | ||
|
|
||
| def push_partial(partial_name) | ||
| @partial_stack.push(partial_name) | ||
| end | ||
|
|
||
| def pop_partial | ||
| @partial_stack.pop | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| module Liquid | ||
| class Block < Tag | ||
| def render_token_with_profiling(token, context) | ||
| Profiler.profile_token_render(token) do | ||
| render_token_without_profiling(token, context) | ||
| end | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the fact that you have the same conditional before and after rendering a bit odd,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I can probably clean this up a bit. |
||
| end | ||
|
|
||
| alias_method :render_token_without_profiling, :render_token | ||
| alias_method :render_token, :render_token_with_profiling | ||
| end | ||
|
|
||
| class Include < Tag | ||
| def render_with_profiling(context) | ||
| Profiler.profile_children(@template_name) do | ||
| render_without_profiling(context) | ||
| end | ||
| end | ||
|
|
||
| alias_method :render_without_profiling, :render | ||
| alias_method :render, :render_with_profiling | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module Liquid | ||
| class Token < String | ||
|
|
||
| attr_reader :line_number | ||
|
|
||
| def initialize(content, line_number) | ||
| super(content) | ||
| @line_number = line_number | ||
| end | ||
|
|
||
| def raw | ||
| "<raw>" | ||
| end | ||
|
|
||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,8 @@ def lookup_class(name) | |
| end | ||
| end | ||
|
|
||
| attr_reader :profiler | ||
|
|
||
| class << self | ||
| # Sets how strict the parser should be. | ||
| # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases. | ||
|
|
@@ -85,6 +87,8 @@ def register_filter(mod) | |
| end | ||
|
|
||
| # creates a new <tt>Template</tt> object from liquid source code | ||
| # To enable profiling, pass in <tt>profile: true</tt> as an option. | ||
| # See Liquid::Profiler for more information | ||
| def parse(source, options = {}) | ||
| template = Template.new | ||
| template.parse(source, options) | ||
|
|
@@ -99,6 +103,7 @@ def initialize | |
| # Parse source code. | ||
| # Returns self for easy chaining | ||
| def parse(source, options = {}) | ||
| @profiling = options.delete(:profile) | ||
| @root = Document.parse(tokenize(source), DEFAULT_OPTIONS.merge(options)) | ||
| @warnings = nil | ||
| self | ||
|
|
@@ -130,6 +135,9 @@ def errors | |
| # if you use the same filters over and over again consider registering them globally | ||
| # with <tt>Template.register_filter</tt> | ||
| # | ||
| # if profiling was enabled in <tt>Template#parse</tt> then the resulting profiling information | ||
| # will be available via <tt>Template#profiler</tt> | ||
| # | ||
| # Following options can be passed: | ||
| # | ||
| # * <tt>filters</tt> : array with local filters | ||
|
|
@@ -183,7 +191,9 @@ def render(*args) | |
| begin | ||
| # render the nodelist. | ||
| # for performance reasons we get an array back here. join will make a string out of it. | ||
| result = @root.render(context) | ||
| result = with_profiling do | ||
| @root.render(context) | ||
| end | ||
| result.respond_to?(:join) ? result.join : result | ||
| rescue Liquid::MemoryError => e | ||
| context.handle_error(e) | ||
|
|
@@ -203,13 +213,40 @@ def render!(*args) | |
| def tokenize(source) | ||
| source = source.source if source.respond_to?(:source) | ||
| return [] if source.to_s.empty? | ||
| tokens = source.split(TemplateParser) | ||
|
|
||
| tokens = calculate_line_numbers(source.split(TemplateParser)) | ||
|
|
||
| # removes the rogue empty element at the beginning of the array | ||
| tokens.shift if tokens[0] and tokens[0].empty? | ||
|
|
||
| tokens | ||
| end | ||
|
|
||
| def calculate_line_numbers(raw_tokens) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How much of a performance hit would it be to always do this (calculate line numbers), not just with profiling enabled? I think having error messages with line numbers would be very valuable for people. Or at least being able to turn it on (independent of profiling).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made it completely dependent on profiling because it is a noticeable performance hit, either in just performance (when I was monkey-patching String) and also now in object management as I'm turning every parsed token into a Token object. It shouldn't be difficult to add a flag that turns on line numbers without profiling, but the best solution would be a parser that figures out the line numbers while parsing, though I understand building a new parser hasn't been trivial.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had asked @jasonroelofs to make the line number calculation only happen when profiling is turned on. There is a noticeable hit (at least 10% -- I forget the actual number) on the benchmark and I don't think that's acceptable. Profiling options should have zero impact when they are turned off.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also meant that the config flag had to move from a render option to a parse option.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jasonhl: We need this behind it's own config flag because we want to use it in the Shopify admin to display warnings with line numbers. But that's outside of the scope of this PR, we can change that later. |
||
| return raw_tokens unless @profiling | ||
|
|
||
| current_line = 1 | ||
| raw_tokens.map do |token| | ||
| Token.new(token, current_line).tap do | ||
| current_line += token.count("\n") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def with_profiling | ||
| if @profiling | ||
| @profiler = Profiler.new | ||
| @profiler.start | ||
|
|
||
| begin | ||
| yield | ||
| ensure | ||
| @profiler.stop | ||
| end | ||
| else | ||
| yield | ||
| end | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ class Variable | |
| FilterParser = /(?:#{FilterSeparator}|(?:\s*(?:#{QuotedFragment}|#{ArgumentSeparator})\s*)+)/o | ||
| EasyParse = /\A *(\w+(?:\.\w+)*) *\z/ | ||
| attr_accessor :filters, :name, :warnings | ||
| attr_accessor :line_number | ||
|
|
||
| def initialize(markup, options = {}) | ||
| @markup = markup | ||
|
|
@@ -34,6 +35,10 @@ def initialize(markup, options = {}) | |
| end | ||
| end | ||
|
|
||
| def raw | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just an attr_accessor for this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stylistic consistency with
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, that's fine -- just wondered :-) |
||
| @markup | ||
| end | ||
|
|
||
| def lax_parse(markup) | ||
| @filters = [] | ||
| if markup =~ /\s*(#{QuotedFragment})(.*)/om | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you define
each, I'd suggest includingEnumerablehere. I'm giving it a try on my Shopify dev environment and that change makes it easier to slice up theTimingobjects to, say, log any code that took > 1s to render or log the 5 slowest tags for each template.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I meant to, must have slipped my mind. Definitely will add this.