refactor(ivy): remove TStylingContext locking in favor of firstUpdatePass flag #33521

matsko · 2019-10-31T21:01:20Z

This patch removes the need to lock the style and class context
instances to track when bindings can be added. What happens now is
that the tNode.firstUpdatePass is used to track when bindings are
registered on the context instances.

matsko · 2019-11-05T00:08:02Z

Presubmit = http://test/OCL:278486043:BASE:278486088:1572912430975:d781cb2

Ivy Presubmit = https://test.corp.google.com/ui#id=OCL:278486043:BASE:278486218:1572912468593:40ac370f

mhevery · 2019-11-05T04:33:55Z

packages/core/src/render3/util/styling_utils.ts

@@ -93,8 +92,8 @@ export function allowDirectStyling(context: TStylingContext, hostBindingsMode: b
    // `ngDevMode` is required to be checked here because tests/debugging rely on the context being
    // populated. If things are in production mode then there is no need to build a context
    // therefore the direct apply can be allowed (even on the first update).
-    allow = ngDevMode ? contextIsLocked : true;
-  } else if (contextIsLocked) {
+    allow = ngDevMode ? !firstUpdatePass : true;


This line and comment looks suspicious. It seems to me that we are changing runtime behavior depending on the ngDevMode? That does not seem right to me. Can you better explain to me what is going on?

We do not need to populate a context whilst in prod mode. The only only reason we do this is in devMode is so that debugNode.styles.debug and debugNode.classes.debug can properly list out all classes/styles on the element (otherwise it doesn't know because the direct styling apply mode doesn't keep track of anything).

Does this not imply that we could accidentally rely on that information in ngDevMode??? I think it would be better if we did not have such code.

The fix for this is to design a debugging layer that works independent of the underling style/class application algorithms.

mhevery · 2019-11-05T04:37:27Z

packages/core/src/render3/styling/bindings.ts

+ * provided `TStylingContext` (which is an instance of a `StylingMapArray`). This inner map will
+ * be updated each time a host binding applies its static styling values (via `elementHostAttrs`)
+ * so these values are only read at this point because this is the very last point before the
+ * first style/class values are flushed to the element.


The above is confusing. Could we have a small code snippet / Concrete example showing why this is needed.

kara

Minor comments

packages/core/src/render3/util/styling_utils.ts

packages/core/src/render3/styling/styling_debug.ts

packages/core/src/render3/styling/bindings.ts

packages/core/src/render3/instructions/styling.ts

mhevery

Please take care of @kara comments

kara

LGTM

ngbot · 2019-11-05T22:39:58Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: material-unit-tests" is failing
     status "ci/circleci: setup" is pending
     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.

…Pass flag This patch removes the need to lock the style and class context instances to track when bindings can be added. What happens now is that the `tNode.firstUpdatePass` is used to track when bindings are registered on the context instances.

…tUpdatePass flag

matsko · 2019-11-05T23:25:43Z

presubmit = http://test/OCL:278486043:BASE:278720559:1572996273861:979d4f59

ivy presubmit = https://test.corp.google.com/ui#id=OCL:278486043:BASE:278720755:1572996333664:1410ab39

…Pass flag (#33521) This patch removes the need to lock the style and class context instances to track when bindings can be added. What happens now is that the `tNode.firstUpdatePass` is used to track when bindings are registered on the context instances. PR Close #33521

…Pass flag (angular#33521) This patch removes the need to lock the style and class context instances to track when bindings can be added. What happens now is that the `tNode.firstUpdatePass` is used to track when bindings are registered on the context instances. PR Close angular#33521

angular-automatic-lock-bot · 2019-12-07T16:04:24Z

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.}

matsko added target: major This PR is targeted for the next major release comp: ivy labels Oct 31, 2019

ngbot bot modified the milestone: needsTriage Oct 31, 2019

googlebot added the cla: yes label Oct 31, 2019

AndrewKushnir added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Oct 31, 2019

matsko mentioned this pull request Nov 1, 2019

refactor(ivy): introduce a firstUpdatePass flag into TView #31270

Closed

matsko force-pushed the first_update_pass_styling_new branch 3 times, most recently from 695a0b2 to a2527ec Compare November 4, 2019 21:44

matsko marked this pull request as ready for review November 4, 2019 21:45

matsko requested a review from a team as a code owner November 4, 2019 21:45

matsko requested review from kara and mhevery November 4, 2019 21:45

matsko force-pushed the first_update_pass_styling_new branch from a2527ec to 7c18290 Compare November 4, 2019 22:14

matsko requested a review from a team as a code owner November 4, 2019 22:14

mhevery suggested changes Nov 5, 2019

View reviewed changes

matsko force-pushed the first_update_pass_styling_new branch from 7c18290 to 53ecac9 Compare November 5, 2019 06:07

kara requested changes Nov 5, 2019

View reviewed changes

kara reviewed Nov 5, 2019

View reviewed changes

matsko force-pushed the first_update_pass_styling_new branch 2 times, most recently from 03f6b01 to 1a51126 Compare November 5, 2019 22:15

mhevery approved these changes Nov 5, 2019

View reviewed changes

kara approved these changes Nov 5, 2019

View reviewed changes

matsko added the action: merge The PR is ready for merge by the caretaker label Nov 5, 2019

matsko force-pushed the first_update_pass_styling_new branch from 1a51126 to 6f78c1c Compare November 5, 2019 22:43

matsko added 3 commits November 5, 2019 15:10

fixup! refactor(ivy): remove TStylingContext locking in favor of firs…

23dedb0

…tUpdatePass flag

fixup! refactor(ivy): remove TStylingContext locking in favor of firs…

6ec6224

…tUpdatePass flag

fixup! refactor(ivy): remove TStylingContext locking in favor of firs…

9be823a

…tUpdatePass flag

matsko force-pushed the first_update_pass_styling_new branch from 6f78c1c to 9be823a Compare November 5, 2019 23:10

atscott closed this in 3297a76 Nov 6, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ivy): remove TStylingContext locking in favor of firstUpdatePass flag #33521

refactor(ivy): remove TStylingContext locking in favor of firstUpdatePass flag #33521

matsko commented Oct 31, 2019

matsko commented Nov 5, 2019

mhevery Nov 5, 2019

matsko Nov 5, 2019

mhevery Nov 5, 2019

matsko Nov 5, 2019

mhevery Nov 5, 2019

matsko Nov 5, 2019

kara left a comment

mhevery left a comment

kara left a comment

ngbot bot commented Nov 5, 2019

matsko commented Nov 5, 2019

angular-automatic-lock-bot bot commented Dec 7, 2019

refactor(ivy): remove TStylingContext locking in favor of firstUpdatePass flag #33521

refactor(ivy): remove TStylingContext locking in favor of firstUpdatePass flag #33521

Conversation

matsko commented Oct 31, 2019

matsko commented Nov 5, 2019

mhevery Nov 5, 2019

Choose a reason for hiding this comment

matsko Nov 5, 2019

Choose a reason for hiding this comment

mhevery Nov 5, 2019

Choose a reason for hiding this comment

matsko Nov 5, 2019

Choose a reason for hiding this comment

mhevery Nov 5, 2019

Choose a reason for hiding this comment

matsko Nov 5, 2019

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

mhevery left a comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

ngbot bot commented Nov 5, 2019

matsko commented Nov 5, 2019

angular-automatic-lock-bot bot commented Dec 7, 2019