Fix timing of expression evaluation in blocks #337
Merged
Conversation
This file contains 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
|
TODOs:
|
added 5 commits
October 25, 2016 16:09
This commit fixes a compiler bug where certain built-in syntaxes would re-evaluate its arguments when the block's assertion causes a re-entry into the append VM, instead of reusing the argument references that were created the first time. In practice, this is observable when a "stateful helper" is used as an argument to a syntax with an assertion (currently `#if`, `#unless`, `#with`, `#each`, `partial`, `InElementSyntax` and dynamic components). Because the argument references are created every time, any state stored on the original instance of the helper will be observably wiped away when used as an argument to an asserting block helper. Of course, it is also observable by comparing the JavaScript identity of the helper in question over time. This commit fixes the compiler bug by moving the evaluation of the expressions passed as arguments to the block syntaxes in question above the `Try` block, and capturing the relevant registers (args and operand, which store the results of evaluating the arguments) so they can be restored when the append VM is re-entered for the same block.
The `condition` register is now saved and restored, like the operand and args registers.
This commit:
Switched partials to use the new opcode building style.
Moved the evaluation of the test opcode, and the production of the
condition register, outside of the block that could be re-entered. This
means that when the expression passed to a {{partial expr}} becomes
invalidated, we no longer generate a brand new reference for the
to-boolean operation on the operand (which produces the condition).
Previously, the code that captured and restored VM state manually captured a few registers and manually restored them. This commit puts the Frame in charge of capturing and restoring, which means that if we want to capture more registers in the future, it will be easy to remember to update the restoring code at the same time.
Now that we capture the conditional register, we have to also simulate that effect when we lazily deopt (since we are essentially doing an OSR)
tilde-engineering
force-pushed
the
fix-assert
branch
from
51ccc8c
to
1ed17a1
Compare
tilde-engineering
pushed a commit
to tildeio/ember.js
that referenced
this pull request
Oct 26, 2016
This commit fixes a bug where `OutletComponentReference` inadvertently relied on broken behavior: it returned `null`, assuming that that would cause the reference to be rebuilt. But glimmerjs/glimmer-vm#337 fixed the underlying bug, causing the reference to be properly cached. As a result, we need to remember that we rendered a `null` component, so that swapping back to the original component replaces the empty component with the rendered component.
chancancode
added a commit
to emberjs/ember.js
that referenced
this pull request
Oct 26, 2016
This commit fixes a bug where `OutletComponentReference` inadvertently relied on broken behavior: it returned `null`, assuming that that would cause the reference to be rebuilt. But glimmerjs/glimmer-vm#337 fixed the underlying bug, causing the reference to be properly cached. As a result, we need to remember that we rendered a `null` component, so that swapping back to the original component replaces the empty component with the rendered component. (cherry picked from commit 8cc93b6)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit fixes a compiler bug where certain built-in syntaxes would re-evaluate its arguments when the block's assertion causes a re-entry into the append VM, instead of reusing the argument references that were created the first time.
In practice, this is observable when a "stateful helper" is used as an argument to a syntax with an assertion (currently
#if,#unless,#with,#each,partial,InElementSyntaxand dynamic components).Because the argument references are created every time, any state stored on the original instance of the helper will be observably wiped away when used as an argument to an asserting block helper. Of course, it is also observable by comparing the JavaScript identity of the helper in question over time.
This commit fixes the compiler bug by moving the evaluation of the expressions passed as arguments to the block syntaxes in question above the
Tryblock, and capturing the relevant registers (args and operand, which store the results of evaluating the arguments) so they can be restored when the append VM is re-entered for the same block.Fixes #336