Skip to content

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

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.

Sign up for GitHub

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

Merged
dylanahsmith merged 3 commits into from
Oct 30, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e4733c
Refactor variable compilation to allow evaluation without write
dylanahsmith Sep 29, 2020
54a2d72
Refactor variable compilation to allow compilation to an expression o…
dylanahsmith Oct 29, 2020
ef0aea5
Strict parse Liquid::Variable.new objects to a Liquid::C::Expression
dylanahsmith Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ext/liquid_c/block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,11 @@ static tag_markup_t internal_block_body_parse(block_body_t *body, parse_context_
variable_parse_args_t parse_args = {
.markup = token.str_trimmed,
.markup_end = token.str_trimmed + token.len_trimmed,
.body = body,
.code = &body->code,
.code_obj = body->obj,
.parse_context = parse_context->ruby_obj,
.line_number = token_start_line_number,
};
internal_variable_parse(&parse_args);
internal_variable_compile(&parse_args, token_start_line_number);
render_score_increment += 1;
body->blank = false;
break;
Expand Down
72 changes: 60 additions & 12 deletions ext/liquid_c/variable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,23 @@

static ID id_rescue_strict_parse_syntax_error;

static ID id_ivar_parse_context;
static ID id_ivar_name;
static ID id_ivar_filters;

static VALUE frozen_empty_array;

static VALUE try_variable_strict_parse(VALUE uncast_args)
{
variable_parse_args_t *parse_args = (void *)uncast_args;
parser_t p;
init_parser(&p, parse_args->markup, parse_args->markup_end);
vm_assembler_t *code = &parse_args->body->code;
vm_assembler_t *code = parse_args->code;

if (p.cur.type == TOKEN_EOS)
if (p.cur.type == TOKEN_EOS) {
vm_assembler_add_push_nil(code);
Copy link
Contributor

@macournoyer macournoyer Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dylanahsmith dylanahsmith Oct 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return Qnil;

vm_assembler_add_render_variable_rescue(code, parse_args->line_number);
}

parse_and_compile_expression(&p, code);

Expand Down Expand Up @@ -70,8 +76,6 @@ static VALUE try_variable_strict_parse(VALUE uncast_args)
vm_assembler_add_filter(code, filter_name, arg_count);
}

vm_assembler_add_pop_write(code);

parser_must_consume(&p, TOKEN_EOS);

return Qnil;
Expand All @@ -88,7 +92,7 @@ static VALUE variable_strict_parse_rescue(VALUE uncast_args, VALUE exception)
{
variable_strict_parse_rescue_t *rescue_args = (void *)uncast_args;
variable_parse_args_t *parse_args = rescue_args->parse_args;
vm_assembler_t *code = &parse_args->body->code;
vm_assembler_t *code = parse_args->code;

// undo partial strict parse
code->instructions.data_end = code->instructions.data + rescue_args->instructions_size;
Expand All @@ -106,19 +110,17 @@ static VALUE variable_strict_parse_rescue(VALUE uncast_args, VALUE exception)

// lax parse
code->protected_stack_size = code->stack_size;
vm_assembler_add_render_variable_rescue(code, parse_args->line_number);
rb_funcall(variable_obj, id_compile_evaluate, 1, parse_args->body->obj);
rb_funcall(variable_obj, id_compile_evaluate, 1, parse_args->code_obj);
if (code->stack_size != code->protected_stack_size + 1) {
rb_raise(rb_eRuntimeError, "Liquid::Variable#compile_evaluate didn't leave exactly 1 new element on the stack");
}
vm_assembler_add_pop_write(code);

return Qnil;
}

void internal_variable_parse(variable_parse_args_t *parse_args)
void internal_variable_compile_evaluate(variable_parse_args_t *parse_args)
{
vm_assembler_t *code = &parse_args->body->code;
vm_assembler_t *code = parse_args->code;
variable_strict_parse_rescue_t rescue_args = {
.parse_args = parse_args,
.instructions_size = c_buffer_size(&code->instructions),
Expand All @@ -128,8 +130,54 @@ void internal_variable_parse(variable_parse_args_t *parse_args)
rb_rescue(try_variable_strict_parse, (VALUE)parse_args, variable_strict_parse_rescue, (VALUE)&rescue_args);
}

void internal_variable_compile(variable_parse_args_t *parse_args, unsigned int line_number)
{
vm_assembler_t *code = parse_args->code;
vm_assembler_add_render_variable_rescue(code, line_number);
internal_variable_compile_evaluate(parse_args);
vm_assembler_add_pop_write(code);
}

static VALUE variable_strict_parse_method(VALUE self, VALUE markup)
{
StringValue(markup);
check_utf8_encoding(markup, "markup");

VALUE parse_context = rb_ivar_get(self, id_ivar_parse_context);

expression_t *expression;
VALUE expression_obj = expression_new(&expression);

variable_parse_args_t parse_args = {
.markup = RSTRING_PTR(markup),
.markup_end = RSTRING_END(markup),
.code = &expression->code,
.code_obj = expression_obj,
.parse_context = parse_context,
};
try_variable_strict_parse((VALUE)&parse_args);
RB_GC_GUARD(markup);
Copy link
Contributor

@peterzhu2118 peterzhu2118 Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dylanahsmith dylanahsmith Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@peterzhu2118 peterzhu2118 Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying!

assert(expression->code.stack_size == 1);
vm_assembler_add_leave(&expression->code);

rb_ivar_set(self, id_ivar_name, expression_obj);
rb_ivar_set(self, id_ivar_filters, frozen_empty_array);

return Qnil;
}

void init_liquid_variable(void)
{
id_rescue_strict_parse_syntax_error = rb_intern("rescue_strict_parse_syntax_error");

id_ivar_parse_context = rb_intern("@parse_context");
id_ivar_name = rb_intern("@name");
id_ivar_filters = rb_intern("@filters");

frozen_empty_array = rb_ary_new();
rb_ary_freeze(frozen_empty_array);
rb_global_variable(&frozen_empty_array);

rb_define_method(cLiquidVariable, "c_strict_parse", variable_strict_parse_method, 1);
}

7 changes: 4 additions & 3 deletions ext/liquid_c/variable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@
typedef struct variable_parse_args {
const char *markup;
const char *markup_end;
block_body_t *body;
vm_assembler_t *code;
VALUE code_obj;
VALUE parse_context;
unsigned int line_number;
} variable_parse_args_t;

void init_liquid_variable(void);
void internal_variable_parse(variable_parse_args_t *parse_args);
void internal_variable_compile(variable_parse_args_t *parse_args, unsigned int line_number);
void internal_variable_compile_evaluate(variable_parse_args_t *parse_args);

#endif

13 changes: 11 additions & 2 deletions lib/liquid/c.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ def new_block_body
end
end

private

# @api private
def disable_liquid_c_nodes
# Liquid::Profiler exposes the internal parse tree that we don't want to build when
# parsing with liquid-c, so consider liquid-c to be disabled when using it.
Expand Down Expand Up @@ -140,6 +139,16 @@ def set_error_mode(parse_context, mode)
parse_context.instance_variable_set(:@error_mode, mode)
end
end

alias_method :ruby_strict_parse, :strict_parse

def strict_parse(markup)
if parse_context.disable_liquid_c_nodes
ruby_strict_parse(markup)
else
c_strict_parse(markup)
end
end
end

Liquid::StrainerTemplate.class_eval do
Expand Down