diff --git a/lib/liquid/loom.rb b/lib/liquid/loom.rb index cf08d868..13bf5e45 100644 --- a/lib/liquid/loom.rb +++ b/lib/liquid/loom.rb @@ -2,6 +2,9 @@ module Liquid class Loom + MERGABLE_IF_OPERATORS = ["==", ">", "<", "!="].freeze + EQUAL_OP = "==".freeze + class << self def optimize(template) new(template).optimize @@ -37,6 +40,59 @@ module Liquid private + def mergable_if_blocks?(target_if, next_if) + target_left = target_if.blocks.first.left + target_right = target_if.blocks.first.right + next_left = next_if.blocks.first.left + next_right = next_if.blocks.first.right + + used_variables = Hash.new { |h, k| h[k] = 0 } + + [ + target_if.blocks.first.left, + target_if.blocks.first.right, + next_if.blocks.first.left, + next_if.blocks.first.right + ].each do |var| + if var.is_a?(VariableLookup) + used_variables[var.name] += 1 + end + end + + return if used_variables.keys.count > 1 + + most_used_variable_name = used_variables.keys[0] + + # TODO: I probably can't do this + # It might be possible to get different result between a > b and b < a + # Move most commonly used variable to the left side + if (target_left.is_a?(VariableLookup) && target_left.name != most_used_variable_name) || (target_right.is_a?(VariableLookup) && target_right.name == most_used_variable_name) + target_left, target_right = target_right, target_left + end + + if (next_left.is_a?(VariableLookup) && next_left.name != most_used_variable_name) || (next_right.is_a?(VariableLookup) && next_right.name == most_used_variable_name) + next_left, next_right = next_right, next_left + end + + return false unless target_left.is_a?(VariableLookup) && next_left.is_a?(VariableLookup) + return false if target_left.name != next_left.name + + return false if target_right.nil? || next_right.nil? + + + # we need to be conversative here and only can merge ==, >, <, and != operators + target_operator = target_if.blocks.first.operator + next_operator = next_if.blocks.first.operator + + return false unless MERGABLE_IF_OPERATORS.include?(target_operator) && MERGABLE_IF_OPERATORS.include?(next_operator) + + return false if target_operator == next_operator && target_right == next_right + + return false if target_right.is_a?(VariableLookup) || next_right.is_a?(VariableLookup) + + true + end + def chain_if_blocks(nodelist, first_if_node, first_if_index) used_variables = Set.new @@ -52,11 +108,7 @@ module Liquid break unless node.is_a?(If) # check if the variables used in the current block are used in the previous block - first_if_node.blocks.each do |condition| - if used_variables.include?(condition.left) || (condition.right && used_variables.include?(condition.right)) - break - end - end + break unless mergable_if_blocks?(first_if_node, node) if_blocks << node end diff --git a/test/unit/eager_optimize_test.rb b/test/unit/eager_optimize_test.rb index 144b8a1d..dabc3662 100644 --- a/test/unit/eager_optimize_test.rb +++ b/test/unit/eager_optimize_test.rb @@ -63,7 +63,6 @@ class EagerOptimizeTest < Minitest::Test def test_merge_if_blocks # for now, work with consecutive if blocks without any String nodes in between source = <<~LIQUID.gsub(/\n/, '') - {% assign foo = 1 %} {% if foo == 1 %} foo: {{ foo }} {% endif %} @@ -75,24 +74,97 @@ class EagerOptimizeTest < Minitest::Test {% endif %} LIQUID - original_template = Liquid::Template.parse(source, eager_optimize: false) - template = Template.parse(source, eager_optimize: true) + assert_optimization([Liquid::If], source, { "foo" => nil }) + assert_optimization([Liquid::If], source, { "foo" => 1 }) + assert_optimization([Liquid::If], source, { "foo" => 2 }) + assert_optimization([Liquid::If], source, { "foo" => 5 }) - assert_equal( - [Liquid::Assign, Liquid::If], - template.root.nodelist.map(&:class), - ) + source = <<~LIQUID.gsub(/\n/, '') + {% assign bar = "application" %} + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + {% if foo == 2 and bar contains "app" %} + foo: {{ foo }} + {% endif %} + {% if 3 == foo and bar == "application" %} + foo: {{ foo }} + {% endif %} + LIQUID - [nil, 1, 2, 3, 4].each do |foo| - assert_equal( - original_template.render('foo' => foo), - template.render('foo' => foo), - ) - end + assert_optimization([Liquid::Assign, Liquid::If], source) + end + + def test_does_not_merge_if_blocks + assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, '')) + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + {% if k == 1 %} + foo: {{ foo }} + {% endif %} + LIQUID + + assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, '')) + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + LIQUID + + assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, '')) + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + {% if a == foo %} + foo: {{ foo }} + {% endif %} + LIQUID + + assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, '')) + {% if foo %} + foo: {{ foo }} + {% endif %} + {% if foo %} + foo: {{ foo }} + {% endif %} + LIQUID + + assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, '')) + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + {% if foo >= 1 %} + foo: {{ foo }} + {% endif %} + LIQUID + + assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, '')) + {% if foo == 1 %} + foo: {{ foo }} + {% endif %} + {% if 1 %} + foo: {{ foo }} + {% endif %} + LIQUID end private + def assert_optimization(expected, source, context = { "foo" => 1 }) + template = Template.parse(source, eager_optimize: true) + assert_equal(expected, template.root.nodelist.map(&:class),) + + baseline_template = Template.parse(source, eager_optimize: false) + + assert_equal( + baseline_template.render(context), + template.render(context), + ) + end + def total_node_count(template) root = template.root children = root.nodelist