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.

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

Jump to bottom

store constant index in instruction #155

Merged
merged 1 commit into from
Feb 1, 2022
Merged

store constant index in instruction #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 11 additions & 7 deletions ext/liquid_c/block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,6 @@ static VALUE block_body_remove_blank_strings(VALUE self)
rb_raise(rb_eRuntimeError, "remove_blank_strings only support being called on a blank block body");
}

VALUE *const_ptr = (VALUE *)body->as.intermediate.code->constants.data;
uint8_t *ip = body->as.intermediate.code->instructions.data;

while (*ip != OP_LEAVE) {
Expand All @@ -380,7 +379,7 @@ static VALUE block_body_remove_blank_strings(VALUE self)
body->as.intermediate.render_score--;
}
}
liquid_vm_next_instruction((const uint8_t **)&ip, (const VALUE **)&const_ptr);
liquid_vm_next_instruction((const uint8_t **)&ip);
}

return Qnil;
Expand Down Expand Up @@ -410,7 +409,7 @@ static VALUE block_body_nodelist(VALUE self)

VALUE nodelist = rb_ary_new_capa(body_header->render_score);

const VALUE *const_ptr = document_body_get_constants_ptr(entry);
const VALUE *constants = &entry->body->constants;
const uint8_t *ip = block_body_instructions_ptr(body_header);
while (true) {
switch (*ip) {
Expand All @@ -434,15 +433,17 @@ static VALUE block_body_nodelist(VALUE self)
}
case OP_WRITE_NODE:
{
rb_ary_push(nodelist, const_ptr[0]);
uint16_t constant_index = (ip[1] << 8) | ip[2];
VALUE node = RARRAY_AREF(*constants, constant_index);
rb_ary_push(nodelist, node);
break;
}

case OP_RENDER_VARIABLE_RESCUE:
rb_ary_push(nodelist, variable_placeholder);
break;
}
liquid_vm_next_instruction(&ip, &const_ptr);
liquid_vm_next_instruction(&ip);
}
loop_break:

Expand All @@ -458,8 +459,11 @@ static VALUE block_body_disassemble(VALUE self)
document_body_entry_t *entry = &body->as.compiled.document_body_entry;
block_body_header_t *header = document_body_get_block_body_header_ptr(entry);
const uint8_t *start_ip = block_body_instructions_ptr(header);
return vm_assembler_disassemble(start_ip, start_ip + header->instructions_bytes,
document_body_get_constants_ptr(entry));
return vm_assembler_disassemble(
start_ip,
start_ip + header->instructions_bytes,
&entry->body->constants
);
}


Expand Down
12 changes: 10 additions & 2 deletions ext/liquid_c/expression.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,16 @@ static VALUE expression_disassemble(VALUE self)
{
expression_t *expression;
Expression_Get_Struct(self, expression);
return vm_assembler_disassemble(expression->code.instructions.data, expression->code.instructions.data_end,
(const VALUE *)expression->code.constants.data);

VALUE constants = rb_ary_new();
uint32_t constants_len = (uint32_t)(c_buffer_size(&expression->code.constants) / sizeof(VALUE));
rb_ary_cat(constants, (VALUE *)expression->code.constants.data, constants_len);

return vm_assembler_disassemble(
expression->code.instructions.data,
expression->code.instructions.data_end,
&constants
);
}

void liquid_define_expression(void)
Expand Down
12 changes: 11 additions & 1 deletion ext/liquid_c/variable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,18 @@ static VALUE variable_strict_parse_rescue(VALUE uncast_args, VALUE exception)
vm_assembler_t *code = parse_args->code;

// undo partial strict parse
uint8_t *last_constants_data_end = (uint8_t *)code->constants.data + rescue_args->constants_size;
VALUE *const_ptr = (VALUE *)last_constants_data_end;
st_table *constants_table = code->constants_table;

while((uint8_t *)const_ptr < code->constants.data_end) {
st_data_t key = (st_data_t)const_ptr[0];
st_delete(constants_table, &key, 0);
const_ptr++;
}

code->instructions.data_end = code->instructions.data + rescue_args->instructions_size;
code->constants.data_end = code->constants.data + rescue_args->constants_size;
code->constants.data_end = last_constants_data_end;
code->stack_size = rescue_args->stack_size;

if (rb_obj_is_kind_of(exception, cLiquidSyntaxError) == Qfalse)
Expand Down
76 changes: 50 additions & 26 deletions ext/liquid_c/vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,20 +231,18 @@ static void hash_bulk_insert(long argc, const VALUE *argv, VALUE hash)
static VALUE vm_render_until_error(VALUE uncast_args)
{
vm_render_until_error_args_t *args = (void *)uncast_args;
const VALUE *const_ptr = args->const_ptr;
const VALUE *constants = args->const_ptr;
const uint8_t *ip = args->ip;
vm_t *vm = args->vm;
VALUE output = args->output;
uint16_t constant_index;
VALUE constant = Qnil;
args->ip = NULL; // used by vm_render_rescue, NULL to indicate that it isn't in a rescue block

while (true) {
switch (*ip++) {
case OP_LEAVE:
return false;

case OP_PUSH_CONST:
vm_stack_push(vm, *const_ptr++);
break;
case OP_PUSH_NIL:
vm_stack_push(vm, Qnil);
break;
Expand All @@ -268,8 +266,14 @@ static VALUE vm_render_until_error(VALUE uncast_args)
break;
}
case OP_FIND_STATIC_VAR:
vm_stack_push(vm, *const_ptr++);
/* fallthrough */
{
constant_index = (ip[0] << 8) | ip[1];
constant = constants[constant_index];
ip += 2;
VALUE value = context_find_variable(&vm->context, constant, Qtrue);
vm_stack_push(vm, value);
break;
}
case OP_FIND_VAR:
{
VALUE key = vm_stack_pop(vm);
Expand All @@ -279,11 +283,16 @@ static VALUE vm_render_until_error(VALUE uncast_args)
}
case OP_LOOKUP_CONST_KEY:
case OP_LOOKUP_COMMAND:
vm_stack_push(vm, *const_ptr++);
/* fallthrough */
{
constant_index = (ip[0] << 8) | ip[1];
constant = constants[constant_index];
ip += 2;
vm_stack_push(vm, constant);
}
/* fallthrough */
case OP_LOOKUP_KEY:
{
bool is_command = ip[-1] == OP_LOOKUP_COMMAND;
bool is_command = ip[-3] == OP_LOOKUP_COMMAND;
VALUE key = vm_stack_pop(vm);
VALUE object = vm_stack_pop(vm);
VALUE result = variable_lookup_key(vm->context.self, object, key, is_command);
Expand Down Expand Up @@ -313,13 +322,20 @@ static VALUE vm_render_until_error(VALUE uncast_args)
case OP_BUILTIN_FILTER:
{
VALUE filter_name;
uint8_t num_args;

if (ip[-1] == OP_FILTER) {
filter_name = *const_ptr++;
constant_index = (ip[0] << 8) | ip[1];
constant = constants[constant_index];
filter_name = RARRAY_AREF(constant, 0);
num_args = RARRAY_AREF(constant, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think num_args should be stored in the constants table, it should be in the instruction (like before). The same filter can be called with a different number of args in a given template:

{{ 'a.jpg' | img_tag }} // 0
{{ 'a.jpg' | img_tag: '10x10', preload: true }} // 2

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this looks like something a test should have caught. Perhaps a test is missing for ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling the same filter with a different number of arguements should be fine...

I had to store the num_args in the constants_table since I couldn't fit it in the instruction...
I had to come up with a cheeky solution where I store the filter_name and num_args together in the constants_table:
Before:

Insturction: [OP_FILTER][num_args] # 8 + 8 bit
Constants:   [filter_name] # 8 bit

After:

Insturction: [OP_FILTER][constant_index] # 8 + 16 bit (24 bit limit)
Constants: [[filter_name, num_args]]

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaaah got it! I had the wrong assumption that filter names were reused in the constant table 👍

ip += 2;
} else {
assert(ip[-1] == OP_BUILTIN_FILTER);
filter_name = builtin_filters[*ip++].sym;
num_args = *ip++; // includes input argument
}
uint8_t num_args = *ip++; // includes input argument

VALUE *args_ptr = vm_stack_pop_n_use_in_place(vm, num_args);
VALUE result = vm_invoke_filter(vm, filter_name, num_args, args_ptr);
vm_stack_push(vm, result);
Expand Down Expand Up @@ -360,21 +376,36 @@ static VALUE vm_render_until_error(VALUE uncast_args)
break;
}

case OP_PUSH_CONST:
{
constant_index = (ip[0] << 8) | ip[1];
constant = constants[constant_index];
ip += 2;
vm_stack_push(vm, constant);
break;
}

case OP_WRITE_NODE:
rb_funcall(cLiquidBlockBody, id_render_node, 3, vm->context.self, output, (VALUE)*const_ptr++);
{
constant_index = (ip[0] << 8) | ip[1];
constant = constants[constant_index];
ip += 2;
rb_funcall(cLiquidBlockBody, id_render_node, 3, vm->context.self, output, constant);

if (RARRAY_LEN(vm->context.interrupts)) {
return false;
}

resource_limits_increment_write_score(vm->context.resource_limits, output);
break;
}
case OP_RENDER_VARIABLE_RESCUE:
// Save state used by vm_render_rescue to rescue from a variable rendering exception
args->node_line_number = ip;
// vm_render_rescue will iterate from this instruction to the instruction
// following OP_POP_WRITE_VARIABLE to resume rendering from
ip += 3;
args->ip = ip;
args->const_ptr = const_ptr;
break;
case OP_POP_WRITE:
{
Expand Down Expand Up @@ -414,7 +445,7 @@ VALUE liquid_vm_evaluate(VALUE context, vm_assembler_t *code)
return ret;
}

void liquid_vm_next_instruction(const uint8_t **ip_ptr, const VALUE **const_ptr_ptr)
void liquid_vm_next_instruction(const uint8_t **ip_ptr)
{
const uint8_t *ip = *ip_ptr;

Expand All @@ -436,26 +467,19 @@ void liquid_vm_next_instruction(const uint8_t **ip_ptr, const VALUE **const_ptr_

case OP_BUILTIN_FILTER:
case OP_PUSH_INT16:
ip += 2;
break;

case OP_WRITE_NODE:
case OP_PUSH_CONST:
case OP_WRITE_NODE:
case OP_FIND_STATIC_VAR:
case OP_LOOKUP_CONST_KEY:
case OP_LOOKUP_COMMAND:
(*const_ptr_ptr)++;
case OP_FILTER:
ip += 2;
break;

case OP_RENDER_VARIABLE_RESCUE:
ip += 3;
break;

case OP_FILTER:
ip++;
(*const_ptr_ptr)++;
break;

case OP_WRITE_RAW_W:
case OP_JUMP_FWD_W:
{
Expand Down Expand Up @@ -514,7 +538,7 @@ static VALUE vm_render_rescue(VALUE uncast_args, VALUE exception)
enum opcode last_op;
do {
last_op = *ip;
liquid_vm_next_instruction(&ip, &render_args->const_ptr);
liquid_vm_next_instruction(&ip);
} while (last_op != OP_POP_WRITE);
render_args->ip = ip;
// remove temporary stack values from variable evaluation
Expand Down
2 changes: 1 addition & 1 deletion ext/liquid_c/vm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ typedef struct vm {
void liquid_define_vm(void);
vm_t *vm_from_context(VALUE context);
void liquid_vm_render(block_body_header_t *block, const VALUE *const_ptr, VALUE context, VALUE output);
void liquid_vm_next_instruction(const uint8_t **ip_ptr, const size_t **const_ptr_ptr);
void liquid_vm_next_instruction(const uint8_t **ip_ptr);
bool liquid_vm_filtering(VALUE context);
VALUE liquid_vm_evaluate(VALUE context, vm_assembler_t *code);

Expand Down
Loading