-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start compiling tags into liquid VM code #96
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work once again 👏
I'm very excited to see the impact this will have on performance.
0ed4a1d
to
c123315
Compare
ext/liquid_c/tag.c
Outdated
block_body_t *body; | ||
BlockBody_Get_Struct(block_body_obj, body); | ||
|
||
if (rb_reg_match(liquid_assign_syntax, markup) == Qnil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is out of scope for this PR, but it might be more efficient to this in C rather than using a regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, but I chose this for simplicity, since I was just focused on parsing the variable directly to VM code.
Note that I haven't merged this yet because I realized that it is missing exception handling for the assign tag or ruby compiled tags in general. This isn't needed for the raw tag, since the only StandardError from OP_WRITE_RAW is Liquid::MemoryError, which gets re-raised anyways. The comment tag doesn't need any exception handling because it doesn't compile to anything. The echo tag re-uses the variable compiling code, so leverages its exception handling. In the general case, we need two instructions surrounding the tag to setup and reset the state for exception handling. As an optimization, we can use a combined final instruction to avoid the extra instruction at the end of rendering a tag, similar to what |
👍 combined final instructions do sound like a good approach.
Do you mean emitting |
We could do it in the same pass by keeping track of the position of the last instruction. If we did it in an optimization pass, it would be pretty similar, since there we would also be keeping track of the previous instruction during iteration in order to effectively lookahead an instruction. |
adc62b9
to
fcbd69b
Compare
I've extracted the assign tag compilation to the draft PR #139 to unblock this PR, since the rest of it shouldn't cause a problem and has already been reviewed. |
Problem
Variable tags are now compiled directly into the block body, but Liquid::Variable is still being used in the assign and echo tags, which means that aren't leveraging liquid-c for parsing (which they did before #60) or rendering.
Solution
Add some initial support (still limited number of instructions) for compiling tags into the liquid VM code, by delegating the compilation to the tag class, which is defined on the Liquid::Tag base class to continue delegating to ruby for parsing and then writing the OP_WRITE_NODE VM instruction.
This lets tag classes to define a compile instance method to compile their tag object into VM code. In order to avoid breaking application code from adding compilation to a tag that the application inherits from, I've defined
Liquid::Tag.inherited
to reset the compile method back to the base compile method defined in Liquid::Tag.A tag can alternatively define a compile class method in order to compile directly to VM code without creating the tag object.
The initial tags I've added compilation for in this PR are the
assign,echo, raw and comment tags.