store StringScanner in ParseContext and reuse it through parsing

lib/liquid.rb

@@ -49,7 +49,6 @@ end
 require "liquid/version"
 require "liquid/deprecations"
 require "liquid/const"
-require "liquid/string_scanner_pool"
 require 'liquid/standardfilters'
 require 'liquid/file_system'
 require 'liquid/parser_switching'

lib/liquid/context.rb

@@ -40,6 +40,10 @@ module Liquid
       @global_filter       = nil
       @disabled_tags       = {}
 
+      # Instead of constructing new StringScanner objects for each Expression parse,
+      # we recycle the same one.
+      @string_scanner = StringScanner.new("")
+
       @registers.static[:cached_partials] ||= {}
       @registers.static[:file_system] ||= environment.file_system
       @registers.static[:template_factory] ||= Liquid::TemplateFactory.new
@@ -176,7 +180,7 @@ module Liquid
     # Example:
     #   products == empty #=> products.empty?
     def [](expression)
-      evaluate(Expression.parse(expression))
+      evaluate(Expression.parse(expression, @string_scanner))
     end
 
     def key?(key)

lib/liquid/expression.rb

@@ -22,7 +22,7 @@ module Liquid
     # malicious input as described in https://github.com/Shopify/liquid/issues/1357
     RANGES_REGEX         = /\A\(\s*(?>(\S+)\s*\.\.)\s*(\S+)\s*\)\z/
 
-    def self.parse(markup)
+    def self.parse(markup, _ss = nil)
       return nil unless markup
 
       markup = markup.strip
@@ -35,14 +35,14 @@ module Liquid
       when INTEGERS_REGEX
         Regexp.last_match(1).to_i
       when RANGES_REGEX
-        RangeLookup.parse(Regexp.last_match(1), Regexp.last_match(2))
+        RangeLookup.parse(Regexp.last_match(1), Regexp.last_match(2), nil)
       when FLOATS_REGEX
         Regexp.last_match(1).to_f
       else
         if LITERALS.key?(markup)
           LITERALS[markup]
         else
-          VariableLookup.parse(markup)
+          VariableLookup.parse(markup, nil)
         end
       end
     end
@@ -58,7 +58,7 @@ module Liquid
       'false' => false,
       'blank' => '',
       'empty' => '',
-      '-' => VariableLookup.parse("-")
+      '-' => VariableLookup.parse("-", nil),
     }.freeze
 
     DOT = ".".ord
@@ -72,7 +72,7 @@ module Liquid
     CACHE = LruRedux::Cache.new(10_000) # most themes would have less than 2,000 unique expression
 
     class << self
-      def parse(markup)
+      def parse(markup, ss = StringScanner.new(""))
         return unless markup
 
         markup = markup.strip # markup can be a frozen string
@@ -86,33 +86,36 @@ module Liquid
 
         return CACHE[markup] if CACHE.key?(markup)
 
-        CACHE[markup] = inner_parse(markup)
+        CACHE[markup] = inner_parse(markup, ss)
       end
 
-      def inner_parse(markup)
+      def inner_parse(markup, ss)
         if (markup.start_with?("(") && markup.end_with?(")")) && markup =~ RANGES_REGEX
-          return RangeLookup.parse(Regexp.last_match(1), Regexp.last_match(2))
+          return RangeLookup.parse(
+            Regexp.last_match(1),
+            Regexp.last_match(2),
+            ss,
+          )
         end
 
-        if (num = parse_number(markup))
+        if (num = parse_number(markup, ss))
           num
         else
-          VariableLookup.parse(markup)
+          VariableLookup.parse(markup, ss)
         end
       end
 
-      def parse_number(markup)
-        ss = StringScannerPool.pop(markup)
-
-        is_integer = true
-        last_dot_pos = nil
-        num_end_pos = nil
-
+      def parse_number(markup, ss)
+        ss.string = markup
         # the first byte must be a digit, a period, or  a dash
         byte = ss.scan_byte
 
         return false if byte != DASH && byte != DOT && (byte < ZERO || byte > NINE)
 
+        is_integer = true
+        last_dot_pos = nil
+        num_end_pos = nil
+
         while (byte = ss.scan_byte)
           return false if byte != DOT && (byte < ZERO || byte > NINE)
 
@@ -136,14 +139,9 @@ module Liquid
         if num_end_pos
           # number ends with a number "123.123"
           markup.byteslice(0, num_end_pos).to_f
-        elsif last_dot_pos
-          markup.byteslice(0, last_dot_pos).to_f
         else
-          # we should never reach this point
-          false
+          markup.byteslice(0, last_dot_pos).to_f
         end
-      ensure
-        StringScannerPool.release(ss)
       end
     end
   end

lib/liquid/lexer.rb

@@ -26,8 +26,8 @@ module Liquid
     WHITESPACE_OR_NOTHING = /\s*/
 
     class << self
-      def tokenize(input)
-        ss = StringScanner.new(input)
+      def tokenize(input, ss = StringScanner.new(""))
+        ss.string = input
         output = []
 
         until ss.eos?
@@ -158,8 +158,8 @@ module Liquid
 
     # rubocop:disable Metrics/BlockNesting
     class << self
-      def tokenize(input)
-        ss = StringScannerPool.pop(input)
+      def tokenize(input, ss)
+        ss.string = input
         output = []
 
         until ss.eos?
@@ -220,8 +220,6 @@ module Liquid
         end
         # rubocop:enable Metrics/BlockNesting
         output << EOS
-      ensure
-        StringScannerPool.release(ss)
       end
 
       def raise_syntax_error(start_pos, ss)

lib/liquid/parse_context.rb

@@ -3,7 +3,7 @@
 module Liquid
   class ParseContext
     attr_accessor :locale, :line_number, :trim_whitespace, :depth
-    attr_reader :partial, :warnings, :error_mode, :environment
+    attr_reader :partial, :warnings, :error_mode, :environment, :string_scanner
 
     def initialize(options = Const::EMPTY_HASH)
       @environment = options.fetch(:environment, Environment.default)
@@ -12,6 +12,10 @@ module Liquid
       @locale   = @template_options[:locale] ||= I18n.new
       @warnings = []
 
+      # constructing new StringScanner in Lexer, Tokenizer, etc is expensive
+      # This StringScanner will be shared by all of them
+      @string_scanner = StringScanner.new("")
+
       self.depth   = 0
       self.partial = false
     end
@@ -24,12 +28,21 @@ module Liquid
       Liquid::BlockBody.new
     end
 
-    def new_tokenizer(markup, start_line_number: nil, for_liquid_tag: false)
-      Tokenizer.new(markup, line_number: start_line_number, for_liquid_tag: for_liquid_tag)
+    def new_parser(input)
+      Parser.new(input, @string_scanner)
+    end
+
+    def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false)
+      Tokenizer.new(
+        source: source,
+        string_scanner: @string_scanner,
+        line_number: start_line_number,
+        for_liquid_tag: for_liquid_tag,
+      )
     end
 
     def parse_expression(markup)
-      Expression.parse(markup)
+      Expression.parse(markup, string_scanner)
     end
 
     def partial=(value)

lib/liquid/parser.rb

@@ -2,8 +2,8 @@
 
 module Liquid
   class Parser
-    def initialize(input)
-      @tokens = Lexer.tokenize(input)
+    def initialize(input, string_scanner)
+      @tokens = Lexer.tokenize(input, string_scanner)
       @p      = 0 # pointer to current location
     end
 

lib/liquid/range_lookup.rb

@@ -2,9 +2,9 @@
 
 module Liquid
   class RangeLookup
-    def self.parse(start_markup, end_markup)
-      start_obj = Expression.parse(start_markup)
-      end_obj   = Expression.parse(end_markup)
+    def self.parse(start_markup, end_markup, string_scanner)
+      start_obj = Expression.parse(start_markup, string_scanner)
+      end_obj   = Expression.parse(end_markup, string_scanner)
       if start_obj.respond_to?(:evaluate) || end_obj.respond_to?(:evaluate)
         new(start_obj, end_obj)
       else

lib/liquid/string_scanner_pool.rb

@@ -1,24 +0,0 @@
-# frozen_string_literal: true
-
-module Liquid
-  class StringScannerPool
-    class << self
-      def pop(input)
-        @ss_pool ||= 5.times.each_with_object([]) { |_i, arr| arr << StringScanner.new("") }
-
-        if @ss_pool.empty?
-          StringScanner.new(input)
-        else
-          ss = @ss_pool.pop
-          ss.string = input
-          ss
-        end
-      end
-
-      def release(ss)
-        @ss_pool ||= []
-        @ss_pool << ss
-      end
-    end
-  end
-end

lib/liquid/tags/for.rb

@@ -88,7 +88,7 @@ module Liquid
     end
 
     def strict_parse(markup)
-      p = Parser.new(markup)
+      p = @parse_context.new_parser(markup)
       @variable_name = p.consume(:id)
       raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in')
 

lib/liquid/tags/if.rb

@@ -102,7 +102,7 @@ module Liquid
     end
 
     def strict_parse(markup)
-      p = Parser.new(markup)
+      p = @parse_context.new_parser(markup)
       condition = parse_binary_comparisons(p)
       p.consume(:end_of_string)
       condition

lib/liquid/tokenizer.rb

@@ -6,7 +6,13 @@ module Liquid
   class Tokenizer1
     attr_reader :line_number, :for_liquid_tag
 
-    def initialize(source, line_numbers = false, line_number: nil, for_liquid_tag: false)
+    def initialize(
+      source:,
+      string_scanner:, # this is not used
+      line_numbers: false,
+      line_number: nil,
+      for_liquid_tag: false
+    )
       @source         = source.to_s.to_str
       @line_number    = line_number || (line_numbers ? 1 : nil)
       @for_liquid_tag = for_liquid_tag
@@ -56,12 +62,22 @@ module Liquid
     CLOSE_CURLEY = "}".ord
     PERCENTAGE = "%".ord
 
-    def initialize(source, line_numbers = false, line_number: nil, for_liquid_tag: false)
-      @line_number    = line_number || (line_numbers ? 1 : nil)
+    def initialize(
+      source:,
+      string_scanner:,
+      line_numbers: false,
+      line_number: nil,
+      for_liquid_tag: false
+    )
+      @line_number = line_number || (line_numbers ? 1 : nil)
       @for_liquid_tag = for_liquid_tag
-      @source         = source
-      @offset         = 0
-      @tokens         = []
+      @source = source
+      @offset = 0
+      @tokens = []
+
+      @ss = string_scanner
+      @ss.string = @source
+
       tokenize
     end
 
@@ -85,13 +101,11 @@ module Liquid
       if @for_liquid_tag
         @tokens = @source.split("\n")
       else
-        @ss = StringScannerPool.pop(@source)
         @tokens << shift_normal until @ss.eos?
       end
 
       @source = nil
-    ensure
-      StringScannerPool.release(@ss) if @ss
+      @ss = nil
     end
 
     def shift_normal

lib/liquid/variable.rb

@@ -61,7 +61,7 @@ module Liquid
 
     def strict_parse(markup)
       @filters = []
-      p = Parser.new(markup)
+      p = @parse_context.new_parser(markup)
 
       return if p.look(:end_of_string)
 

lib/liquid/variable_lookup.rb

@@ -6,16 +6,19 @@ module Liquid
 
     attr_reader :name, :lookups
 
-    def self.parse(markup)
-      new(markup)
+    def self.parse(markup, string_scanner)
+      new(markup, string_scanner)
     end
 
-    def initialize(markup)
+    def initialize(markup, string_scanner = StringScanner.new(""))
       lookups = markup.scan(VariableParser)
 
       name = lookups.shift
       if name&.start_with?('[') && name&.end_with?(']')
-        name = Expression.parse(name[1..-2])
+        name = Expression.parse(
+          name[1..-2],
+          string_scanner,
+        )
       end
       @name = name
 
@@ -25,7 +28,10 @@ module Liquid
       @lookups.each_index do |i|
         lookup = lookups[i]
         if lookup&.start_with?('[') && lookup&.end_with?(']')
-          lookups[i] = Expression.parse(lookup[1..-2])
+          lookups[i] = Expression.parse(
+            lookup[1..-2],
+            string_scanner,
+          )
         elsif COMMAND_METHODS.include?(lookup)
           @command_flags |= 1 << i
         end

performance/theme_runner.rb

@@ -50,8 +50,13 @@ class ThemeRunner
 
   # `tokenize` will just test the tokenizen portion of liquid without any templates
   def tokenize
+    ss = StringScanner.new("")
     @tests.each do |test_hash|
-      tokenizer = Liquid::Tokenizer.new(test_hash[:liquid], true)
+      tokenizer = Liquid::Tokenizer.new(
+        source: test_hash[:liquid],
+        string_scanner: ss,
+        line_numbers: true,
+      )
       while tokenizer.shift; end
     end
   end

test/integration/expression_test.rb

@@ -43,7 +43,7 @@ class ExpressionTest < Minitest::Test
   end
 
   def test_quirky_negative_sign_expression_markup
-    result = Expression.parse("-")
+    result = Expression.parse("-", nil)
     assert(result.is_a?(VariableLookup))
     assert_equal("-", result.name)
 

test/unit/lexer_unit_test.rb

@@ -134,6 +134,6 @@ class LexerUnitTest < Minitest::Test
   private
 
   def tokenize(input)
-    Lexer.tokenize(input)
+    Lexer.tokenize(input, StringScanner.new(""))
   end
 end

test/unit/parser_unit_test.rb

@@ -6,20 +6,20 @@ class ParserUnitTest < Minitest::Test
   include Liquid
 
   def test_consume
-    p = Parser.new("wat: 7")
+    p = new_parser("wat: 7")
     assert_equal('wat', p.consume(:id))
     assert_equal(':', p.consume(:colon))
     assert_equal('7', p.consume(:number))
   end
 
   def test_jump
-    p = Parser.new("wat: 7")
+    p = new_parser("wat: 7")
     p.jump(2)
     assert_equal('7', p.consume(:number))
   end
 
   def test_consume?
-    p = Parser.new("wat: 7")
+    p = new_parser("wat: 7")
     assert_equal('wat', p.consume?(:id))
     assert_equal(false, p.consume?(:dot))
     assert_equal(':', p.consume(:colon))
@@ -27,7 +27,7 @@ class ParserUnitTest < Minitest::Test
   end
 
   def test_id?
-    p = Parser.new("wat 6 Peter Hegemon")
+    p = new_parser("wat 6 Peter Hegemon")
     assert_equal('wat', p.id?('wat'))
     assert_equal(false, p.id?('endgame'))
     assert_equal('6', p.consume(:number))
@@ -36,7 +36,7 @@ class ParserUnitTest < Minitest::Test
   end
 
   def test_look
-    p = Parser.new("wat 6 Peter Hegemon")
+    p = new_parser("wat 6 Peter Hegemon")
     assert_equal(true, p.look(:id))
     assert_equal('wat', p.consume(:id))
     assert_equal(false, p.look(:comparison))
@@ -46,12 +46,12 @@ class ParserUnitTest < Minitest::Test
   end
 
   def test_expressions
-    p = Parser.new("hi.there hi?[5].there? hi.there.bob")
+    p = new_parser("hi.there hi?[5].there? hi.there.bob")
     assert_equal('hi.there', p.expression)
     assert_equal('hi?[5].there?', p.expression)
     assert_equal('hi.there.bob', p.expression)
 
-    p = Parser.new("567 6.0 'lol' \"wut\"")
+    p = new_parser("567 6.0 'lol' \"wut\"")
     assert_equal('567', p.expression)
     assert_equal('6.0', p.expression)
     assert_equal("'lol'", p.expression)
@@ -59,7 +59,7 @@ class ParserUnitTest < Minitest::Test
   end
 
   def test_ranges
-    p = Parser.new("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)")
+    p = new_parser("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)")
     assert_equal('(5..7)', p.expression)
     assert_equal('(1.5..9.6)', p.expression)
     assert_equal('(young..old)', p.expression)
@@ -67,7 +67,7 @@ class ParserUnitTest < Minitest::Test
   end
 
   def test_arguments
-    p = Parser.new("filter: hi.there[5], keyarg: 7")
+    p = new_parser("filter: hi.there[5], keyarg: 7")
     assert_equal('filter', p.consume(:id))
     assert_equal(':', p.consume(:colon))
     assert_equal('hi.there[5]', p.argument)
@@ -77,8 +77,14 @@ class ParserUnitTest < Minitest::Test
 
   def test_invalid_expression
     assert_raises(SyntaxError) do
-      p = Parser.new("==")
+      p = new_parser("==")
       p.expression
     end
   end
+
+  private
+
+  def new_parser(str)
+    Parser.new(str, StringScanner.new(""))
+  end
 end

test/unit/tag_unit_test.rb

@@ -6,18 +6,18 @@ class TagUnitTest < Minitest::Test
   include Liquid
 
   def test_tag
-    tag = Tag.parse('tag', "", Tokenizer.new(""), ParseContext.new)
+    tag = Tag.parse('tag', "", new_tokenizer, ParseContext.new)
     assert_equal('liquid::tag', tag.name)
     assert_equal('', tag.render(Context.new))
   end
 
   def test_return_raw_text_of_tag
-    tag = Tag.parse("long_tag", "param1, param2, param3", Tokenizer.new(""), ParseContext.new)
+    tag = Tag.parse("long_tag", "param1, param2, param3", new_tokenizer, ParseContext.new)
     assert_equal("long_tag param1, param2, param3", tag.raw)
   end
 
   def test_tag_name_should_return_name_of_the_tag
-    tag = Tag.parse("some_tag", "", Tokenizer.new(""), ParseContext.new)
+    tag = Tag.parse("some_tag", "", new_tokenizer, ParseContext.new)
     assert_equal('some_tag', tag.tag_name)
   end
 
@@ -26,7 +26,16 @@ class TagUnitTest < Minitest::Test
   end
 
   def test_tag_render_to_output_buffer_nil_value
-    custom_tag = CustomTag.parse("some_tag", "", Tokenizer.new(""), ParseContext.new)
+    custom_tag = CustomTag.parse("some_tag", "", new_tokenizer, ParseContext.new)
     assert_equal('some string', custom_tag.render_to_output_buffer(Context.new, "some string"))
   end
+
+  private
+
+  def new_tokenizer
+    Tokenizer.new(
+      source: "",
+      string_scanner: StringScanner.new(""),
+    )
+  end
 end