Closed ggmichaelgo closed 2 years ago
I've added @peterzhu2118 as a reviewer, since this work would ideally fit in nicely with his unmerged work on supporting serializing compiled templates. Similarly, I've added you as a reviewer for his first unmerged PR in that chain of work.
What are you trying to accomplish?
Main issue: https://github.com/Shopify/liquid-c/issues/84
In order for us to implement
If
tag andFor
tag in Liquid-C, we need to be able to freely travel the instruction pointer. Currently, Liquid-C uses an instruction pointer and a constants pointer, and this PR will remove the constants pointer.What approach did you choose and why?
Currently, the instruction row is structured like this:
With this refactor, we will be storing the constant's index value in the instruction:
** We need to keep our instruction to be or below 24 bit in order to store compiled Liquid template in storage services.
Performance Improvement on simple rendering
With this refactor, we are seeing a no-speed improvement with simple benchmark testing. (gist link)
main branch
PR branch
Affect on Performance
While parsing, we will be storing the
constant
's index value intoconstants_table
, therefore we won't be storing duplicate values in theconstants
array. With this change, we are looking up and inserting value to a hashtable whilevm_assembler_write_ruby_constant
.This will slightly affect our parsing time:
Before:
Performance side-effect:
Notes: Ops to update to use constant index over constant pointer:
TODO:
const_ptr
constants
andconstants_table
getting out of sync (this was happening whenvariable_strict_parse_rescue
rolling backconstants
array)