refactor(ivy): don't create TNodes for native projectable nodes #28275
Conversation
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
|
The legacy test failure looks legit! We are now adding an |
pkozlowski-opensource
force-pushed
the
projectable_nodes_as_native_nodes
branch
2 times, most recently
from
78b3726 to
037e7e3
Compare
Before this commit we were creating a "fake" TNode for each and every projectable node passed during dynamic component creation. This approach had several problems: - the existing TView structure had to be mutated to accomodate new TNodes and it was very easy to "corrupt" TView / TNode data structures; - TNodes are not really needed to fully support projectable nodes so we were creating objects and updating existing data structures for nothing. This commit changes the approach so we don't create "fake" TNodes for projectable nodes but instead we process projectable nodes directly in the projection instruction. As a result we've got less code, less object allocation and - as a bonus - we fix few bugs where TView / TNode data structures were corrupted when using projectable nodes.
pkozlowski-opensource
force-pushed
the
projectable_nodes_as_native_nodes
branch
from
62971f5 to
aa0a135
Compare
pkozlowski-opensource
added
target: major
|
@petebacondarwin I think the test failure you mentioned has been fixed. Can you take another look? |
|
@kara yep, the error @petebacondarwin mentioned was present only in the WIP version of this PR. So all is good here. |
kara
removed
the
action: review
kara
added
action: presubmit
|
merge-assistance: global approval, since the test failure has been fixed and the only changes in upgrade are to turn on the tests. |
vetom
pushed a commit
to vetom/angular
that referenced
this pull request
Jan 31, 2019
…8275) Before this commit we were creating a "fake" TNode for each and every projectable node passed during dynamic component creation. This approach had several problems: - the existing TView structure had to be mutated to accomodate new TNodes and it was very easy to "corrupt" TView / TNode data structures; - TNodes are not really needed to fully support projectable nodes so we were creating objects and updating existing data structures for nothing. This commit changes the approach so we don't create "fake" TNodes for projectable nodes but instead we process projectable nodes directly in the projection instruction. As a result we've got less code, less object allocation and - as a bonus - we fix few bugs where TView / TNode data structures were corrupted when using projectable nodes. PR Close angular#28275
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Before this commit we were creating a "fake" TNode for each and every
projectable node passed during dynamic component creation. This approach
had several problems:
it was very easy to "corrupt" TView / TNode data structures;
creating objects and updating existing data structures for nothing.
This commit changes the approach so we don't create "fake" TNodes for projectable
nodes but instead we process projectable nodes directly in the projection instruction.
As a result we've got less code, less object allocation and - as a bonus - we fix few
bugs where TView / TNode data structures were corrupted when using projectable nodes.
NOTE A REVIEWER: there is still one remaining test marked with FW-873 - this is because the root cause is different to the one fixed in this PR (LView / TView corruption by TNodes created for projectable nodes). I've investigated the other root cause and I know what is going on plus I think I know how to fix it. But the fix is not trivial so it will be done in a separate PR.