Strict parse Liquid::Variable.new objects to a Liquid::C::Expression

[dylanahsmith]

This solves the problem that #96 intended to solve in a simpler way, so that PR can be focused on introducing support for compiling tags to VM code. Instead, this PR compiles the variable expression and filtering into a Liquid::C::Expression object that is stored in the Liquid::Variable object's @name instance variable, which will be evaluated on render.

Benchmark

The liquid benchmark only uses the assign tag {% assign article = pages.frontpage %} and doesn't use the echo tag, so the benchmark is mostly for unaffected code. However, I wrote a micro-benchmark that shows the performance improvement

# frozen_string_literal: true

require 'bundler/setup'
require 'benchmark/ips'
require 'liquid/c'

PARSE_CONTEXT = Liquid::ParseContext.new
CONTEXT = Liquid::Context.new('x' => 1, 'y' => 2)

def parse
  Liquid::Variable.new("x | plus: y", PARSE_CONTEXT)
end

Benchmark.ips do |x|
  x.report("parse") { parse }
  variable = parse
  x.report("render") { variable.render(CONTEXT) }
  x.report("parse & render") { parse.render(CONTEXT) }
end

result on master

               parse     88.381k (± 3.2%) i/s -    445.068k in   5.041305s
              render    510.144k (± 2.3%) i/s -      2.577M in   5.053979s
      parse & render     67.412k (± 4.0%) i/s -    342.414k in   5.088448s

result on this branch

               parse    631.565k (± 3.0%) i/s -      3.160M in   5.009016s
              render    961.840k (± 4.0%) i/s -      4.858M in   5.060674s
      parse & render    355.364k (± 3.3%) i/s -      1.790M in   5.041629s

[peterzhu2118]

I'm not sure why this is needed. How is markup at risk of being GC'd?

[dylanahsmith]

isn't needed before try_variable_strict_parse is called, so the markup local variable could get re-used by the compiler and remove the reference to the object from the C stack. If the only reference to the markup object were passed into this method, then in theory, it seems like the GC could end up cleanup the string before we are done using it.

It probably isn't needed in practice, but it makes the code more obviously correct.

[peterzhu2118]

Thanks for clarifying!

[macournoyer]

🤯

Just unsure about that push_nil at the end.

[macournoyer]

Not sure I understand why you need to push nil here? Is it to make an empty variable leave something on the stack? Since expressions must always return a value.

[dylanahsmith]

Yeah, we need something left on the stack since it will be followed by a pop to get the return value