-
Notifications
You must be signed in to change notification settings - Fork 25
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
store constant index in instruction #155
Conversation
23302b6
to
9d6bdd6
Compare
0f0f8af
to
e9fd3c5
Compare
06f440e
to
a9c841e
Compare
49368f9
to
7f78392
Compare
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.
WOWOWOWOW! THIS IS AMAZING @ggmichaelgo 🔥 🤯 🔥
I have a couple comments/questions. But the OP_FILTER
num_args
looks like a blocker to me (which is why I marked as "Request changes").
I still need to wrap my head around the assembler merging logic (vm_assembler_concat
), I didn't know about that.
But overall... This is awesome!
(Did I mention this is amazing?)
ext/liquid_c/vm.c
Outdated
if (vm_assembler_opcode_has_constant(*ip)) { | ||
uint16_t constant_index = (ip[1] << 8) | ip[2]; | ||
constant = constants[constant_index]; | ||
} | ||
|
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.
This is a hot spot, the VM loop is the most critical part of the code. You should try to avoid executing this if
on each turn in the loop.
I would move this inside the instructions that need it. Perhaps you could refactor to an inlined function. Or even code duplication would be OK here IMO, because perf is so important inside the loop.
if (ip[-1] == OP_FILTER) { | ||
filter_name = *const_ptr++; | ||
filter_name = RARRAY_AREF(constant, 0); | ||
num_args = RARRAY_AREF(constant, 1); |
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.
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
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.
Also, this looks like something a test should have caught. Perhaps a test is missing for ^
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.
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]]
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.
Aaaah got it! I had the wrong assumption that filter names were reused in the constant table 👍
*instructions++ = OP_BUILTIN_FILTER; | ||
*instructions++ = builtin_index; | ||
*instructions++ = arg_count + 1; // include input |
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.
Just a note: you'll need to move this back outside the if
when moving back arg_count
into the instruction.
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.
This will be specifically for built_in
filters.
Built in filter instruction:
[OP_FILTER][BUILT_IN_INDEX][ARG_COUNT] # 8 + 8 + 8 bit
Filter instruction:
[OP_FILTER][Constant Index] # 8 + 16 bit (constant = [filter_name, num_args])
@@ -43,6 +43,7 @@ extern filter_desc_t builtin_filters[]; | |||
typedef struct vm_assembler { | |||
c_buffer_t instructions; | |||
c_buffer_t constants; |
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.
Do we still need this?
Seems to me it's duplicating data already in constants_table
.
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.
ah... good call... I used to store the hash value of the constants, but now I am using constant
as key in the constants_table
.
Since the keys in st_table
are sorted by entries, we can remove constants
array. (I will have to double check how st_table
works)
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.
I have started to remove the constants
, and it is becoming a large PR just by removing it... Let's create a separate PR to remove the constants
.
Also, we are storing the VALUE
as keys in the constants_table
, and those are pointers to actual objects, so we won't be leaving many memory footprints.
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.
actually... I am a bit afraid if JRuby or TruffleRuby will behave the same...
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. |
2602e6d
to
00fc47a
Compare
3df6bfd
to
2a64a2c
Compare
2f3e5e3
to
d1a7367
Compare
2a64a2c
to
87e6fde
Compare
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.
LGTM! 🚀
if (ip[-1] == OP_FILTER) { | ||
filter_name = *const_ptr++; | ||
filter_name = RARRAY_AREF(constant, 0); | ||
num_args = RARRAY_AREF(constant, 1); |
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.
Aaaah got it! I had the wrong assumption that filter names were reused in the constant table 👍
87e6fde
to
d28a941
Compare
What are you trying to accomplish?
Main issue: #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 while
vm_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)