refactor(core): Replace `ExpandoInstructions` with `HostBindingOpCodes` #39301

mhevery · 2020-10-16T03:01:11Z

Review last change only.
This PR builds on #39233 and #39003

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

AndrewKushnir

LGTM, just left a few minor comments, thanks @mhevery 👍

packages/core/src/render3/component.ts

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

AndrewKushnir · 2020-10-17T01:08:32Z

packages/core/src/render3/interfaces/node.ts

@@ -409,11 +409,17 @@ export interface TNode {

  /**
   * Stores starting index of the directives.
+   *
+   * The first directive is always component.


That's an important comment, thanks for adding it! 👍

Can we expand it a bit by adding a reference to the corresponding function/logic?

Can you suggest what you would like to see here.

packages/core/src/render3/interfaces/view.ts

packages/core/src/render3/component.ts

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

packages/core/src/render3/node_manipulation_i18n.ts

mhevery · 2020-10-17T23:18:31Z

presubmit

petebacondarwin · 2020-10-18T19:17:05Z

packages/core/src/render3/interfaces/view.ts

+ *  for (let i = 0; i < hostBindingOpCodes.length; i++) {
+ *    const opCode = hostBindingOpCodes[i] as number;
+ *    if (opCode < 0) {
+ *      // Negative numbers are element indexes.
+ *      setSelectedIndex(~opCode);
+ *    } else {
+ *      // Positive numbers are NumberTuple which store bindingRootIndex and directiveIndex.
+ *      const directiveIdx = opCode;
+ *      const bindingRootIndx = hostBindingOpCodes[++i] as number;
+ *      const hostBindingFn = hostBindingOpCodes[++i] as HostBindingsFunction<any>;
+ *      setBindingRootForHostBindings(bindingRootIndx, directiveIdx);
+ *      const context = lView[directiveIdx];
+ *      hostBindingFn(RenderFlags.Update, context);
+ *    }
+ *  }


NIT - you can dedent these lines, since the if statement above does not create a block.

By the way, this pseudo code indicates that the items after the elementIdx can be 0 or more, rather than the 1 or more mentioned further up.

dedented.

I see your point about 1 or more vs 0 or more. But in practice it would be 1 or more, even thought you are right the code supports it. I don't want to suggest that there would ever be situation followed by 0 functions. That would be just wasteful. Any suggestion on how to make that more clear?

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

integration/README.md

packages/core/src/render3/node_manipulation.ts

petebacondarwin

Reviewed-for: size-tracking

IgorMinar

LGTM with just a few minor comments. This technique to optionally pull in part of the core is interesting (but admittedly it makes my head hurt a bit).

Thanks for tracking down most of the size regression and documenting the debugging steps.

Reviewed-for: global-approvers, size-tracking

packages/core/src/render3/i18n/i18n_tree_shaking.ts

packages/core/src/render3/i18n/i18n_util.ts

integration/README.md

mhevery · 2020-10-21T22:35:39Z

presubmit

mhevery · 2020-10-21T22:43:22Z

presubmit

mhevery · 2020-10-22T00:52:33Z

presubmit

The `ExpandoInstructions` was unnecessarily convoluted way to solve the problem of calling the `HostBindingFunction`s on components and directives. The code was complicated and hard to fallow. The replacement is a simplified way to achieve the same thing, which is also more efficient in space and speed.

mhevery · 2020-10-22T03:11:28Z

presubmit

`TNode.insertBeforeIndex` is only populated when i18n is present. This change puts all code which reads `insertBeforeIndex` behind a dynamically loaded functions which are set only when i18n code executes.

…39301) PR Close #39301

`TNode.insertBeforeIndex` is only populated when i18n is present. This change puts all code which reads `insertBeforeIndex` behind a dynamically loaded functions which are set only when i18n code executes. PR Close #39301

…s` (#39301) The `ExpandoInstructions` was unnecessarily convoluted way to solve the problem of calling the `HostBindingFunction`s on components and directives. The code was complicated and hard to fallow. The replacement is a simplified way to achieve the same thing, which is also more efficient in space and speed. PR Close #39301

…39301) PR Close #39301

`TNode.insertBeforeIndex` is only populated when i18n is present. This change puts all code which reads `insertBeforeIndex` behind a dynamically loaded functions which are set only when i18n code executes. PR Close #39301

This commit removes a workaround to calculate the `expandoStartIndex` value. That workaround was needed because the `expandoStartIndex` was updated previously, so it pointed at the wrong location. The PR angular#39301 (angular#39301) fixed that problem and the workaround is no longer needed.

This commit removes a workaround to calculate the `expandoStartIndex` value. That workaround was needed because the `expandoStartIndex` was updated previously, so it pointed at the wrong location. The problem was fixed in PR angular#39301 and the workaround is no longer needed.

This commit removes a workaround to calculate the `expandoStartIndex` value. That workaround was needed because the `expandoStartIndex` was updated previously, so it pointed at the wrong location. The problem was fixed in PR #39301 and the workaround is no longer needed. PR Close #39416

angular-automatic-lock-bot · 2020-11-22T16:26:22Z

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

pullapprove bot requested review from alxhub, IgorMinar, jelbourn and petebacondarwin October 16, 2020 03:01

google-cla bot added the cla: yes label Oct 16, 2020

mhevery requested a review from AndrewKushnir October 16, 2020 03:02

mhevery force-pushed the i18n_cleanup_6 branch 3 times, most recently from 5380eee to 2f7bb31 Compare October 16, 2020 20:20

AndrewKushnir approved these changes Oct 17, 2020

View reviewed changes

mhevery force-pushed the i18n_cleanup_6 branch 2 times, most recently from eefd762 to edc7b59 Compare October 17, 2020 21:02

petebacondarwin reviewed Oct 18, 2020

View reviewed changes

pullapprove bot requested a review from petebacondarwin October 18, 2020 19:43

petebacondarwin approved these changes Oct 18, 2020

View reviewed changes

pullapprove bot requested a review from atscott October 18, 2020 19:44

petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 18, 2020

mhevery force-pushed the i18n_cleanup_6 branch 2 times, most recently from d51b241 to 41d1b35 Compare October 19, 2020 15:28

mhevery added target: minor This PR is targeted for the next minor release and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 19, 2020

IgorMinar approved these changes Oct 19, 2020

View reviewed changes

packages/core/src/render3/i18n/i18n_tree_shaking.ts Outdated Show resolved Hide resolved

packages/core/src/render3/i18n/i18n_util.ts Outdated Show resolved Hide resolved

integration/README.md Outdated Show resolved Hide resolved

integration/README.md Outdated Show resolved Hide resolved

mhevery added the target: rc This PR is targeted for the next release-candidate label Oct 19, 2020

IgorMinar removed the target: minor This PR is targeted for the next minor release label Oct 19, 2020

mhevery force-pushed the i18n_cleanup_6 branch from 41d1b35 to d2e3728 Compare October 21, 2020 18:22

AndrewKushnir added area: core Issues related to the framework runtime refactoring Issue that involves refactoring or code-cleanup labels Oct 21, 2020

ngbot bot modified the milestone: needsTriage Oct 21, 2020

mhevery force-pushed the i18n_cleanup_6 branch from d2e3728 to 872af70 Compare October 21, 2020 22:32

mhevery force-pushed the i18n_cleanup_6 branch from 872af70 to 993c487 Compare October 21, 2020 22:41

mhevery force-pushed the i18n_cleanup_6 branch from 993c487 to 214459a Compare October 22, 2020 00:13

AndrewKushnir removed request for atscott, jelbourn and alxhub October 22, 2020 00:43

mhevery force-pushed the i18n_cleanup_6 branch from 214459a to feeac7a Compare October 22, 2020 00:44

mhevery added 2 commits October 21, 2020 20:01

docs(core): Add documentation on debugging size regression failures

121ba5c

mhevery force-pushed the i18n_cleanup_6 branch from feeac7a to 96a513a Compare October 22, 2020 03:02

refactor(core): Improve tree shakability of i18n code.

af78a0f

`TNode.insertBeforeIndex` is only populated when i18n is present. This change puts all code which reads `insertBeforeIndex` behind a dynamically loaded functions which are set only when i18n code executes.

mhevery force-pushed the i18n_cleanup_6 branch from 96a513a to af78a0f Compare October 22, 2020 03:37

alxhub added the action: merge The PR is ready for merge by the caretaker label Oct 22, 2020

alxhub closed this in 68d4de6 Oct 22, 2020

alxhub pushed a commit that referenced this pull request Oct 22, 2020

docs(core): Add documentation on debugging size regression failures (#…

ffd4161

…39301) PR Close #39301

alxhub pushed a commit that referenced this pull request Oct 22, 2020

docs(core): Add documentation on debugging size regression failures (#…

44f245a

…39301) PR Close #39301

AndrewKushnir mentioned this pull request Oct 25, 2020

refactor(core): remove expandoStartIndex workaround #39416

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Replace `ExpandoInstructions` with `HostBindingOpCodes` #39301

refactor(core): Replace `ExpandoInstructions` with `HostBindingOpCodes` #39301

mhevery commented Oct 16, 2020 •

edited

AndrewKushnir left a comment

AndrewKushnir Oct 17, 2020

mhevery Oct 17, 2020

mhevery commented Oct 17, 2020

petebacondarwin Oct 18, 2020

petebacondarwin Oct 18, 2020

mhevery Oct 19, 2020

petebacondarwin left a comment

IgorMinar left a comment

mhevery commented Oct 21, 2020

mhevery commented Oct 21, 2020

mhevery commented Oct 22, 2020

mhevery commented Oct 22, 2020

angular-automatic-lock-bot bot commented Nov 22, 2020

refactor(core): Replace ExpandoInstructions with HostBindingOpCodes #39301

refactor(core): Replace ExpandoInstructions with HostBindingOpCodes #39301

Conversation

mhevery commented Oct 16, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Oct 17, 2020

Choose a reason for hiding this comment

mhevery Oct 17, 2020

Choose a reason for hiding this comment

mhevery commented Oct 17, 2020

petebacondarwin Oct 18, 2020

Choose a reason for hiding this comment

petebacondarwin Oct 18, 2020

Choose a reason for hiding this comment

mhevery Oct 19, 2020

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

mhevery commented Oct 21, 2020

mhevery commented Oct 21, 2020

mhevery commented Oct 22, 2020

mhevery commented Oct 22, 2020

angular-automatic-lock-bot bot commented Nov 22, 2020

refactor(core): Replace `ExpandoInstructions` with `HostBindingOpCodes` #39301

refactor(core): Replace `ExpandoInstructions` with `HostBindingOpCodes` #39301

mhevery commented Oct 16, 2020 •

edited