Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dylanahsmith
Copy link
Contributor

@dylanahsmith dylanahsmith commented Oct 20, 2020

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.

Copy link
Contributor

@macournoyer macournoyer left a 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.

ext/liquid_c/vm.c Outdated Show resolved Hide resolved
ext/liquid_c/vm_assembler.h Outdated Show resolved Hide resolved
block_body_t *body;
BlockBody_Get_Struct(block_body_obj, body);

if (rb_reg_match(liquid_assign_syntax, markup) == Qnil)
Copy link
Member

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.

Copy link
Contributor Author

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.

@dylanahsmith
Copy link
Contributor Author

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 OP_POP_WRITE is implicitly doing. However, that OP_POP_WRITE instruction should really be called something like OP_POP_WRITE_END_RESCUE, where we would need another instruction to not reset the exception handling state. E.g. we could have an OP_ASSIGN and an OP_ASSIGN_END_RESCUE, then convert from the former to the latter at the end of tag compilation, instead of having to additional instruction (e.g. OP_END_RESCUE).

@macournoyer
Copy link
Contributor

👍 combined final instructions do sound like a good approach.

then convert from the former to the latter at the end of tag compilation, instead of having to additional instruction

Do you mean emitting OP_ASSIGN & OP_END_RESCUE during compilation, and combine those into a OP_ASSIGN_END_RESCUE in a subsequent optimization pass after?

@dylanahsmith
Copy link
Contributor Author

Do you mean emitting OP_ASSIGN & OP_END_RESCUE during compilation, and combine those into a OP_ASSIGN_END_RESCUE in a subsequent optimization pass after?

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.

@dylanahsmith
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants