Refactored and optimized rendering #1005

christopheraue · 2018-04-16T12:50:53Z

Measures:

A while loop is faster than iterating with #each.
Check string tokens first. String tokens are far more frequent
than interrupt tokens. If a token is a string token, checking
for an interrupt token can be avoided.
String tokens just map to themselves and don't need the special
treatment of BlockBody#render_node (except the resource limit
check).

Benchmark

$ bundle exec rake benchmark:run

Before

Run 1)
              parse:     41.630  (± 0.0%) i/s -    420.000  in  10.089309s
             render:     75.962  (± 3.9%) i/s -    763.000  in  10.066823s
     parse & render:     25.497  (± 0.0%) i/s -    256.000  in  10.040862s

Run 2)
              parse:     42.130  (± 0.0%) i/s -    424.000  in  10.064738s
             render:     77.003  (± 1.3%) i/s -    777.000  in  10.093524s
     parse & render:     25.739  (± 0.0%) i/s -    258.000  in  10.024581s

Run 3)
              parse:     41.976  (± 2.4%) i/s -    420.000  in  10.021406s
             render:     76.184  (± 1.3%) i/s -    763.000  in  10.018104s
     parse & render:     25.641  (± 0.0%) i/s -    258.000  in  10.062549s

After

Run 1)
              parse:     42.198  (± 0.0%) i/s -    424.000  in  10.048737s
             render:     79.495  (± 2.5%) i/s -    798.000  in  10.042610s
     parse & render:     25.874  (± 3.9%) i/s -    260.000  in  10.053336s

Run 2)
              parse:     41.961  (± 0.0%) i/s -    420.000  in  10.009858s
             render:     78.623  (± 1.3%) i/s -    791.000  in  10.064043s
     parse & render:     25.927  (± 0.0%) i/s -    260.000  in  10.028472s

Run 3)
              parse:     42.321  (± 2.4%) i/s -    424.000  in  10.021159s
             render:     79.468  (± 2.5%) i/s -    798.000  in  10.048127s
     parse & render:     26.065  (± 0.0%) i/s -    262.000  in  10.053814s

fw42

Added a few questions, but I don't have any strong objections here. @dylanahsmith wdyt?

fw42 · 2018-04-18T09:43:13Z

lib/liquid/block_body.rb

-        # Break out if we have any unhanded interrupts.
+      idx = 0
+      while node = @nodelist[idx]
+        render_node_to_output(node, output, context)
        break if context.interrupt?


Hm. Not sure I understand why this break was at the beginning of the loop body before but now it's at the end of the loop body. Could you explain?

If an interrupt happens during render_node_to_output it seems unnecessary to wait for the next iteration of the loop to break out of it.

We could avoid this check entirely if token.is_a?(String) and we didn't extract code out into the render_node_to_output method. Perhaps we could instead move the exception handling into render_node to simplify this method without losing the ability to call break inside the case statement.

That's a good point. I'll try that.

fw42 · 2018-04-18T09:45:05Z

lib/liquid/block_body.rb

+    def render_node_to_output(node, output, context)
+      case node
+      when String
+        node_output = node


Is this extra variable necessary? Why not

check_resources(context, node) output << node

?

It's noI necessary. I probably left it there because of the symmetry with the other branches. Should I change it?

fw42 · 2018-04-18T09:46:02Z

lib/liquid/block_body.rb

+        context.push_interrupt(node.interrupt)
+      when Block
+        node_output = render_node(node, context)
+        output << node_output unless node.blank?


If node.blank? is true, did we actually have to call render_node? Or could we check that before?

render_node may have side effects for block tags. At least the test suite lights up, if blank? it's checked before.

fw42 · 2018-04-18T09:48:18Z

Thanks for your contributions!

dylanahsmith

Optimizing rending of strings is a good idea.

dylanahsmith · 2018-04-18T13:58:38Z

lib/liquid/block_body.rb

+        # Interrupt is any command that stops block execution such as {% break %}
+        # or {% continue %}
+        context.push_interrupt(node.interrupt)
+      when Block


Do we actually benefit from separating this out into its own case rather than doing

node_output = render_node(token, context) unless token.is_a?(Block) && token.blank? output << node_output end

in the else case as we did before? It seems like the same amount of work is done, since the only difference is whether token.is_a?(Block) is checked by the when Block or by unless token.is_a?(Block).

This was more "refactor" than "optimize". It's a little less DRY, but I find it cleaner and easier to read. It comes down to personal taste, I guess.

dylanahsmith · 2018-04-18T14:05:01Z

lib/liquid/block_body.rb

-      @nodelist.each do |token|
-        # Break out if we have any unhanded interrupts.
+      idx = 0
+      while node = @nodelist[idx]


A while loop is faster than iterating with #each.

This statement surprised me, so I created this microbenchmark

require 'benchmark' N = 100000 LIST = (0..1000).to_a Benchmark.bmbm do |x| x.report("while") do N.times do LIST.each do |item| end end end x.report("each") do N.times do idx = 0 while item = LIST[idx] idx += 1 end end end end

the results were

user system total real while 3.470000 0.010000 3.480000 ( 3.480269) each 2.230000 0.000000 2.230000 ( 2.232083)

so I think this change might have made things worse, even if the overall benchmark improved from the other changes in this PR.

When running your benchmark I get the same result.

But when running the liquid benchmark while appears to be faster than each. Here are three runs using each:

Run 1) parse: 41.466 (± 2.4%) i/s - 416.000 in 10.039060s render: 77.517 (± 3.9%) i/s - 777.000 in 10.036251s parse & render: 25.457 (± 3.9%) i/s - 256.000 in 10.060507s Run 2) parse: 41.188 (± 0.0%) i/s - 412.000 in 10.004006s render: 77.819 (± 2.6%) i/s - 784.000 in 10.081885s parse & render: 25.772 (± 3.9%) i/s - 258.000 in 10.015315s Run 3) parse: 41.479 (± 0.0%) i/s - 416.000 in 10.030267s render: 76.804 (± 6.5%) i/s - 770.000 in 10.071492s parse & render: 24.713 (± 4.0%) i/s - 248.000 in 10.059831s

Maybe one of you can check that too.

Interesting. I tried with only the while loop change it did improve performance on the benchmark.

dylanahsmith · 2018-04-18T14:27:31Z

lib/liquid/block_body.rb

-        # Break out if we have any unhanded interrupts.
+      idx = 0
+      while node = @nodelist[idx]
+        render_node_to_output(node, output, context)
        break if context.interrupt?


We could avoid this check entirely if token.is_a?(String) and we didn't extract code out into the render_node_to_output method. Perhaps we could instead move the exception handling into render_node to simplify this method without losing the ability to call break inside the case statement.

Measures: 1) A while loop is faster than iterating with #each. 2) Check string, variable and block tokens first. They are far more frequent than interrupt tokens. In their case, checking for an interrupt can be avoided. 3) String tokens just map to themselves and don't need the special treatment of BlockBody#render_node (except the resource limit check). Benchmark ========= $ bundle exec rake benchmark:run Before ------ Run 1) parse: 41.630 (± 0.0%) i/s - 420.000 in 10.089309s render: 75.962 (± 3.9%) i/s - 763.000 in 10.066823s parse & render: 25.497 (± 0.0%) i/s - 256.000 in 10.040862s Run 2) parse: 42.130 (± 0.0%) i/s - 424.000 in 10.064738s render: 77.003 (± 1.3%) i/s - 777.000 in 10.093524s parse & render: 25.739 (± 0.0%) i/s - 258.000 in 10.024581s Run 3) parse: 41.976 (± 2.4%) i/s - 420.000 in 10.021406s render: 76.184 (± 1.3%) i/s - 763.000 in 10.018104s parse & render: 25.641 (± 0.0%) i/s - 258.000 in 10.062549s After ----- Run 1) parse: 42.283 (± 0.0%) i/s - 424.000 in 10.028306s render: 83.158 (± 2.4%) i/s - 832.000 in 10.009201s parse & render: 26.417 (± 0.0%) i/s - 266.000 in 10.069718s Run 2) parse: 41.159 (± 4.9%) i/s - 412.000 in 10.031297s render: 81.591 (± 3.7%) i/s - 816.000 in 10.018225s parse & render: 25.924 (± 3.9%) i/s - 260.000 in 10.035653s Run 3) parse: 42.418 (± 2.4%) i/s - 424.000 in 10.003100s render: 84.183 (± 2.4%) i/s - 847.000 in 10.069781s parse & render: 26.726 (± 0.0%) i/s - 268.000 in 10.029857s

christopheraue · 2018-04-19T10:27:09Z

Taking all comments into account I updated the code. The case statement and break are now directly inside the loop again. This already increased performance. I then also moved the handling of Variable and Block tokens to the top of the case statement. That gave the performance another push.

These are the new results:

Run 1)
              parse:     42.283  (± 0.0%) i/s -    424.000  in  10.028306s
             render:     83.158  (± 2.4%) i/s -    832.000  in  10.009201s
     parse & render:     26.417  (± 0.0%) i/s -    266.000  in  10.069718s

Run 2)
              parse:     41.159  (± 4.9%) i/s -    412.000  in  10.031297s
             render:     81.591  (± 3.7%) i/s -    816.000  in  10.018225s
     parse & render:     25.924  (± 3.9%) i/s -    260.000  in  10.035653s

Run 3)
              parse:     42.418  (± 2.4%) i/s -    424.000  in  10.003100s
             render:     84.183  (± 2.4%) i/s -    847.000  in  10.069781s
     parse & render:     26.726  (± 0.0%) i/s -    268.000  in  10.029857s

fw42

Awesome

fw42 reviewed Apr 18, 2018

View reviewed changes

dylanahsmith reviewed Apr 18, 2018

View reviewed changes

fw42 approved these changes Apr 19, 2018

View reviewed changes

dylanahsmith merged commit 5f0b64c into Shopify:master Apr 19, 2018

christopheraue deleted the render_refactor branch April 19, 2018 21:38

shopify-shipit bot temporarily deployed to rubygems-4-0-stable January 11, 2023 15:52 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored and optimized rendering #1005

Refactored and optimized rendering #1005

christopheraue commented Apr 16, 2018

fw42 left a comment

fw42 Apr 18, 2018

christopheraue Apr 18, 2018

dylanahsmith Apr 18, 2018

christopheraue Apr 18, 2018

fw42 Apr 18, 2018

christopheraue Apr 18, 2018

fw42 Apr 18, 2018

christopheraue Apr 18, 2018

fw42 commented Apr 18, 2018

dylanahsmith left a comment

dylanahsmith Apr 18, 2018

christopheraue Apr 18, 2018

dylanahsmith Apr 18, 2018

christopheraue Apr 18, 2018

dylanahsmith Apr 18, 2018

dylanahsmith Apr 18, 2018

christopheraue commented Apr 19, 2018

fw42 left a comment

Refactored and optimized rendering #1005

Refactored and optimized rendering #1005

Conversation

christopheraue commented Apr 16, 2018

Benchmark

Before

After

fw42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fw42 commented Apr 18, 2018

dylanahsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopheraue commented Apr 19, 2018

fw42 left a comment

Choose a reason for hiding this comment