Refactor to support liquid-c VM compilation of variables#1294
Merged
Conversation
macournoyer
approved these changes
Sep 17, 2020
|
|
||
| test = %( {{ "some_string" | a_bad_filter }} ) | ||
| # MRI imprecisely marks objects found on the C stack, which can result | ||
| # in uninitialized memory being marked. This can even result in the test failing |
Contributor
There was a problem hiding this comment.
MRI's solution to marking VALUE references in running C functions is to scan the stack/registers for things that look like VALUE pointers heuristically. Uninitialized memory sometimes looks like a VALUE.
The ParseTreeVisitor exposes the liquid internals that won't be kept compatible with liquid-c, so move it out of the integration tests directory so that we can easily ignore it when testing liquid-c
since I would like to continue supporting these tests in liquid-c in the foreseeable future.
dylanahsmith
force-pushed
the
changes-for-liquid-c-vm-variable
branch
from
cddd8ea to
33760f0
Compare
pushrax
approved these changes
Sep 29, 2020
|
|
||
| test = %( {{ "some_string" | a_bad_filter }} ) | ||
| # MRI imprecisely marks objects found on the C stack, which can result | ||
| # in uninitialized memory being marked. This can even result in the test failing |
Contributor
There was a problem hiding this comment.
MRI's solution to marking VALUE references in running C functions is to scan the stack/registers for things that look like VALUE pointers heuristically. Uninitialized memory sometimes looks like a VALUE.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In liquid-c, I would like to stop maintaining full compatibility with Liquid::BlockBody#nodelist, but right now liquid-c is being tested against both the liquid unit and integration tests, both of which have some coupling to that nodelist. Specifically, for liquid-c compilation of variables into VM code (Shopify/liquid-c#59) I wanted to avoid creating Liquid::Variable objects from the VM code, where we currently have direct node class comparison in the liquid tests.
Also, as part of having a different internal representation of Liquid::Variable, I would also like to use garbage collectable symbols for filter names in liquid-c, which a liquid integration test prevents.
Solution
I moved the test for ParseTreeVisitor to the test/unit directory, since that interface is deeply coupled to the nodelist. I also looked through the unit tests to find any that seem useful in testing liquid-c and converted some of them to integration tests.
I changed SecurityTest#test_does_not_add_filters_to_symbol_table to run the garbage collector before asserting that no new symbols were added, to exclude symbols that can be garbage collected. However, that alone proved to be unreliable since MRI uses an imprecise GC to mark the C stack, which can cause deterministic failures for a given build of ruby, which I had actually debugged to confirm that this was the case. To make this reliable, I used a separate thread for the code that created the symbol, so the symbol VALUE would only be written on a separate stack that wouldn't be marked once the thread has finished.
I've also extracted some code from Liquid::BlockBody#render_node rescue code into a
rescue_render_nodemethod to re-use in liquid-c for simplicity and to reduce duplication, since it doesn't seem like it is on a hot code path.