fix(ivy): fix views manipulation logic #22656

pkozlowski-opensource · 2018-03-08T16:29:29Z

This commit fixes a bug that would result in views insert / remove
even if a view needed only refresh operation.

The crux of the bug was that we were looking for a view to update
only in the LContainer.nextIndex position. This is incorrect as a
view with a given block id could be present later in the views
array (this happens if we about to remove a view in the middle of
the views array).

The code in this fix searches for a view to update in the views array and
can remove views in the middle of the views collection. Previously we
would remove views at the end of the collection only.

vicb · 2018-03-08T16:40:43Z

packages/core/src/render3/di.ts

@@ -576,6 +576,10 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
    const lView = (viewRef as EmbeddedViewRef<any>)._lViewNode;
    insertView(this._node, lView, index);

+    // TODO(pk): this is super-hackish, but we need to adjust nextIndex so the containerRefreshEnd


Not clear what should be done here ?

It would be nice to have an explanation of under what conditions this arrises and why we are fixing it this way.

Always answer: what, when, why questions in your comments.

Ha! Me neither at this point :-) To be discussed!

Updated this comment

vicb · 2018-03-08T16:43:17Z

packages/core/src/render3/instructions.ts


  if (viewUpdateMode) {
-    previousOrParentNode = views[lContainer.nextIndex++];
+    const existingViewNode = previousOrParentNode = views[existingViewIdx];


Why introducing a new const here ?
If this is to improve readability:

add a type const existingViewNode: LViewNode = ... ? remove type cast on l 1199

use the new const l 1197

Yep, readability

Refactored this part in the fixup commit

vicb · 2018-03-08T16:46:08Z

packages/core/src/render3/instructions.ts

+      // no views to delete between expected and actual position of a view being processed
+      if (existingViewIdx > containerState.nextIndex) {
+        for (let i = containerState.nextIndex; i < existingViewIdx; i++) {
+          removeView(container, i);


Could it happen that the views are moved ? Would remove + insert be efficient in this case ?

No, views can't move based on instructions in a template. They can be created / removed but always stay in the same order.

For the VCRef support we will need proper move, but at this point I'm not sure if we are going to use exact same code for VCRef support.

I don't think that insert / remove is a good substitute for move - too many data structures to re-create for nothing....

No, views can't move based on instructions in a template. They can be created / removed but always stay in the same order.

How does this play with NgFor then ?

mhevery · 2018-03-08T17:10:41Z

packages/core/test/render3/control_flow_spec.ts

+    */
+    function Template(ctx: any, cm: boolean) {
+      if (cm) {
+        container(0);


Thinking about it:

We should have 3 containers not 1.

BlockID was a mistake and should be removed.

Case for 3 containers

I don't believe this is a valid test. Each % block should have its own container so I think we should have

container(0); container(1); container(2);

each if should be surrounded with a separate containerRefreshStart and containerRefreshEnd.

Think of it this way.

% if (exp) { text1 % } text2 % if (exp) { text3 % }

The above clearly has two containers:

container(1); text('text2'); container(2);

Now remove the text2 and you should have:

container(1); container(2);

But our test shows that we have:

container(1);

and that is wrong since inserting extra text nodes should not fundamentally change the behavior of the code.

I believe that this issue would not show up with multiple containers.

Case for removing BlockID.

Each block has a blockId as seen in embeddedViewStart(blockId). This is needed because in the case of if the then and else block are fundamentally different shape (ie they can have different bindings). To be able to differentiate between the two cases we came up with the blockId solution. This test demonstrates to me that that solution was wrong because it is a solution for the else block. No other situation has the else block problem.

Assume if (exp) { } else {} is same as if (exp) {} if (!exp) {} and I have already argued that the second case should have two containers. So we should drop blockId and instead generate more containers. The issue is that } else { requires us to insert code into the else part which we can't do in else (and in the case of this test would require us to insert code into } /*HERE*/ if (exp) { which we don't want to do.

% if (exp) { textTrue % } else { textFalse % }

should compile to:

function (ctx, cm) { if (cm) { container(0); container(1); } containerRefreshStartRange(/*startContainer*/0, /*endContainer*/1); if (ctx.exp) { embeddedViewStart(/*containerId*/ 0); embeddedViewEnd(); } else { embeddedViewStart(/*containerId*/ 1); embeddedViewEnd(); } containerRefreshEndRange(/*startContainer*/0, /*endContainer*/1); }

@mhevery I've discussed at length with @marclaval and @b-laporte pros and cons of one vs. multiple containers and the conclusion was that we don't see one approach being superior to another.

We see those things more like a tradeoff, each solution having pros and cons. If we go with multiple containers we will create more data structures so slow down creation mode. Also, I don't think that we are introducing any perf penalty with one container, see: #22656 (comment)

In any case we are talking about a corner cases really (if / else and blocks being opened / closed on the same line).

Given all the above could we re-evaluate the decision to make the refactoring at this point of time? An alternative curse of action would be to:

merge this fix (we've got a bug that we should fix);

use the time to work on ViewContainerRef / ViewRef to get better understanding of all problems / use-cases around views and containers;

review the one vs. multiple containers decision after ViewContainerRef / ViewRef is done.

WDYT?

Per our discussion in the meeting, I agree with your assessment.

mhevery · 2018-03-08T17:14:13Z

packages/core/test/render3/control_flow_spec.ts

+    *     1
+    *   % }; if(ctx.condition2) {
+    *     2
+    *   % } ; if(ctx.condition3) {


nit: extra space } ;

mhevery · 2018-03-08T17:16:02Z

packages/core/src/render3/di.ts

@@ -576,6 +576,10 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
    const lView = (viewRef as EmbeddedViewRef<any>)._lViewNode;
    insertView(this._node, lView, index);

+    // TODO(pk): this is super-hackish, but we need to adjust nextIndex so the containerRefreshEnd
+    // instruction is not removing dynamically inserted views


what do you mean by dynamically inserted What other inserts are there?

mhevery · 2018-03-08T17:17:02Z

packages/core/src/render3/di.ts

@@ -576,6 +576,10 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
    const lView = (viewRef as EmbeddedViewRef<any>)._lViewNode;
    insertView(this._node, lView, index);

+    // TODO(pk): this is super-hackish, but we need to adjust nextIndex so the containerRefreshEnd


It would be nice to have an explanation of under what conditions this arrises and why we are fixing it this way.

Always answer: what, when, why questions in your comments.

mhevery · 2018-03-08T17:22:56Z

packages/core/src/render3/instructions.ts

-        containerState.views[containerState.nextIndex - 1] as LViewNode :
-        null;
-    const viewIdChanged = previousView == null ? true : previousView.data.id !== viewNode.data.id;
+    const existingViewIdx = viewIndex(containerState, containerState.nextIndex, viewNode.data.id);


I am not sure I like the fact that we have to do linear scan here. This will be slow with large container sets.

@mhevery my current understanding is that we need a loop in certain, rare conditions. I've refactored the code to make it obvious. Let me explain in more details.

Case where views need to be refreshed

In the update case we will find a view at the next possible position so there is no iteration.

Case where a view need to be inserted

In this case we will find a view with a higher block id or reach the end of the views collection. No loop in this case.

Case where views need to be removed

This is the only case where we can have a real scan. Let's consider the "worst-case" scenario. Given the following template:

% if (true) { start % }; for (let i = 0; i < collection.length; i++) { item {{i}} % }; if (true) { end % }

and a collection shrinking from, say, 10k items to 0 items, we would have to scan 10k items.

But we must do this loop anyway to remove obsolete views!

We need to have this loop no matter what and this PR doesn't add any additional processing in the current form (I've refactored the code to make it more clear).

mhevery · 2018-03-13T17:36:18Z

Can you get lint to pass?

ngbot · 2018-03-13T17:36:36Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "ci/circleci: lint" is failing
status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

This commit fixes a bug that would result in views insert / remove even if a view needed only refresh operation. The crux of the bug was that we were looking for a view to update only in the LContainer.nextIndex position. This is incorrect as a view with a given block id could be present later in the views array (this happens if we about to remove a view in the middle of the views array). The code in this fix searches for a view to update in the views array and can remove views in the middle of the views collection. Previously we would remove views at the end of the collection only.

This commit fixes a bug that would result in views insert / remove even if a view needed only refresh operation. The crux of the bug was that we were looking for a view to update only in the LContainer.nextIndex position. This is incorrect as a view with a given block id could be present later in the views array (this happens if we about to remove a view in the middle of the views array). The code in this fix searches for a view to update in the views array and can remove views in the middle of the views collection. Previously we would remove views at the end of the collection only. PR Close angular#22656

angular-automatic-lock-bot · 2019-09-13T10:22:35Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Mar 8, 2018

pkozlowski-opensource added target: major This PR is targeted for the next major release comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 8, 2018

pkozlowski-opensource force-pushed the views_with_different_id_in_a_container branch from 1af57fb to 239ea2d Compare March 8, 2018 16:40

vicb reviewed Mar 8, 2018

View reviewed changes

mhevery suggested changes Mar 8, 2018

View reviewed changes

pkozlowski-opensource force-pushed the views_with_different_id_in_a_container branch from 239ea2d to e852902 Compare March 13, 2018 11:04

mhevery approved these changes Mar 13, 2018

View reviewed changes

mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 13, 2018

pkozlowski-opensource force-pushed the views_with_different_id_in_a_container branch 2 times, most recently from f10162b to f01d4c1 Compare March 14, 2018 13:58

fixup! fix(ivy): fix views manipulation logic

6d5eede

pkozlowski-opensource force-pushed the views_with_different_id_in_a_container branch from f01d4c1 to 6d5eede Compare March 14, 2018 14:10

kara closed this in c09bd67 Mar 14, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): fix views manipulation logic #22656

fix(ivy): fix views manipulation logic #22656

pkozlowski-opensource commented Mar 8, 2018

vicb Mar 8, 2018

mhevery Mar 8, 2018

pkozlowski-opensource Mar 8, 2018

pkozlowski-opensource Mar 13, 2018

vicb Mar 8, 2018

pkozlowski-opensource Mar 8, 2018

pkozlowski-opensource Mar 13, 2018

vicb Mar 8, 2018

pkozlowski-opensource Mar 8, 2018

vicb Mar 13, 2018

mhevery Mar 8, 2018 •

edited

pkozlowski-opensource Mar 13, 2018

mhevery Mar 13, 2018

mhevery Mar 8, 2018

pkozlowski-opensource Mar 13, 2018

mhevery Mar 8, 2018

pkozlowski-opensource Mar 13, 2018

mhevery Mar 8, 2018

mhevery Mar 8, 2018

pkozlowski-opensource Mar 13, 2018 •

edited

mhevery commented Mar 13, 2018

ngbot bot commented Mar 13, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(ivy): fix views manipulation logic #22656

fix(ivy): fix views manipulation logic #22656

Conversation

pkozlowski-opensource commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery Mar 8, 2018 • edited

Choose a reason for hiding this comment

Case for 3 containers

Case for removing BlockID.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource Mar 13, 2018 • edited

Choose a reason for hiding this comment

Case where views need to be refreshed

Case where a view need to be inserted

Case where views need to be removed

mhevery commented Mar 13, 2018

ngbot bot commented Mar 13, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

mhevery Mar 8, 2018 •

edited

pkozlowski-opensource Mar 13, 2018 •

edited