refactor(ivy): generatePropertyAliases #22082

vicb · 2018-02-08T06:23:14Z

No description provided.

trotyl · 2018-02-08T07:47:59Z

packages/core/src/render3/instructions.ts

 *
- * @param index Index where data should be stored in tData
+ * @param number lNodeFlags node flags of the logical node
+ * @param Direction direction wheter to consider inputs or outputs


mary-poppins · 2018-02-08T16:19:50Z

You can preview ee65c12 at https://pr22082-ee65c12.ngbuilds.io/.

kara · 2018-02-08T18:16:30Z

packages/core/src/render3/instructions.ts

@@ -501,11 +506,11 @@ export function createTView(): TView {

 function setUpAttributes(native: RElement, attrs: string[]): void {
  ngDevMode && assertEqual(attrs.length % 2, 0, 'attrs.length % 2');
-  const isProceduralRenderer = (renderer as ProceduralRenderer3).setAttribute;
+
+  const isProc = isProceduralRenderer(renderer);


Nit: I'm not sure we're getting much benefit out of taking this out into a variable. The variable name is a bit obscure and it will minify slightly less well than if it were inlined.

I believe most minifiers are smart enough to inline it back in. So I think it is fine.

The call is here because the usage is inside a for loop (do not want the fn call to be repeated).

Ah, that makes sense.

kara · 2018-02-08T18:17:14Z

packages/core/src/render3/instructions.ts

@@ -113,6 +113,11 @@ let bindingIndex: number;
 */
 let cleanup: any[]|null;

+const enum Direction {


Add comment?

What kind of comments would you add here ? (anything I could think of would actually bring more noise or confusion - probably easier for a native speaker)

Without context (and esp floating here at the top), Direction could be anything. Might help to know where it is used and why.

I don't want to restrict the meaning of this to its current usage.
ie you can use this anywhere you need a Direction that could be input or output.

My XP is that when you name things / add comment about what a thing is for it will become outdated faster. I prefer to comment/name things according to what they do.

Concrete exemple in the code base:

/** * Returns the first DOM node following the given logical node in the same parent DOM element. * * ... */ function findBeforeNode(node: LNode | null, stopNode: LNode | null): RElement|RText|null { // }

Here the comment is right, this function returns the next node but IMO the name is misleading. It assumes that you will call this fn to get the next node so that you can insert before. I think it is too restrictive, I might call this fn in some other context and the name will be completely meaningless then.

kara · 2018-02-08T18:18:36Z

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

+   *
+   * - `undefined` means that the prop has not been initialized yet,
+   * - `null` means that the prop has been initialized but no outputs have been found.
+   */


Nice, thanks for fleshing this out 👍

kara · 2018-02-08T18:26:52Z

packages/core/src/render3/instructions.ts

+  const start = lNodeFlags >> LNodeFlags.INDX_SHIFT;
+  const size = (lNodeFlags & LNodeFlags.SIZE_MASK) >> LNodeFlags.SIZE_SHIFT;
+  const isInput = direction === Direction.Input;
+  const propStore: PropertyAliases = {};


I like the direction of this refactor, but I'm concerned it might be a tad slower because of unnecessary object creation (and thus increased GC pressure).

It was originally configured this way so that property alias objects were only created if a node had a directive with at least one input or at least one output defined. In this implementation, we will create a new property alias object for every node that has a binding or a listener, regardless of whether it has a directive or any inputs or outputs. So if you have a template with 10 nodes and 0 directives, you will still create 10 unnecessary objects for mapping directive inputs/outputs.

One optimization might be to check whether the size is greater than 0 before creating the object so they are only created on nodes that actually have directives. But we will still be creating wasted objects for directives that have no inputs/outputs. Thoughts?

+1 to initialize the store only when there is actually directives.

I often favor semantic over speed if there are no tests to prove that the speedy implementation is actually faster.

Agreed on adding performance tests, but in their absence, I think it's pretty well-established that creating objects unnecessarily will eventually hurt performance.

mary-poppins · 2018-02-08T19:39:30Z

You can preview bec0d06 at https://pr22082-bec0d06.ngbuilds.io/.

mary-poppins · 2018-02-10T01:17:26Z

You can preview 00ef46d at https://pr22082-00ef46d.ngbuilds.io/.

kara

LGTM

ngbot · 2018-02-10T01:22:47Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     conflicts with base branch "master"
     status "google3" is pending
     status "continuous-integration/travis-ci/pr" is pending
     status "code-review/pullapprove" 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.

PR Close #22082

PR Close angular#22082

angular-automatic-lock-bot · 2019-09-13T09:52:08Z

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 Feb 8, 2018

vicb added aio: preview 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 Feb 8, 2018

vicb force-pushed the 0207-ir branch from c3c3737 to 779fc1e Compare February 8, 2018 06:57

angular deleted a comment from mary-poppins Feb 8, 2018

vicb force-pushed the 0207-ir branch from 779fc1e to dbf59e5 Compare February 8, 2018 07:13

trotyl reviewed Feb 8, 2018

View reviewed changes

vicb force-pushed the 0207-ir branch from dbf59e5 to ee65c12 Compare February 8, 2018 16:10

angular deleted a comment from mary-poppins Feb 8, 2018

kara reviewed Feb 8, 2018

View reviewed changes

vicb removed the aio: preview label Feb 8, 2018

vicb added 4 commits February 9, 2018 16:54

refactor(ivy): generatePropertyAliases

cee3acf

refactor(ivy): code simplification

83cbb07

fixup! refactor(ivy): generatePropertyAliases

a581530

fixup! refactor(ivy): generatePropertyAliases

00ef46d

vicb force-pushed the 0207-ir branch from bec0d06 to 00ef46d Compare February 10, 2018 01:08

kara approved these changes Feb 10, 2018

View reviewed changes

vicb 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 Feb 10, 2018

mhevery closed this in 61341b2 Feb 12, 2018

mhevery pushed a commit that referenced this pull request Feb 12, 2018

refactor(ivy): code simplification (#22082)

62e7b9d

PR Close #22082

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

refactor(ivy): generatePropertyAliases (angular#22082)

793ee3b

PR Close angular#22082

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

refactor(ivy): code simplification (angular#22082)

68a946e

PR Close angular#22082

vicb deleted the 0207-ir branch March 6, 2018 22:25

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

refactor(ivy): generatePropertyAliases (angular#22082)

4ce172f

PR Close angular#22082

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

refactor(ivy): code simplification (angular#22082)

1503aa7

PR Close angular#22082

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

refactor(ivy): generatePropertyAliases #22082

refactor(ivy): generatePropertyAliases #22082

vicb commented Feb 8, 2018

trotyl Feb 8, 2018

vicb Feb 8, 2018

mary-poppins commented Feb 8, 2018

kara Feb 8, 2018

mhevery Feb 8, 2018

vicb Feb 8, 2018

kara Feb 8, 2018

kara Feb 8, 2018

vicb Feb 8, 2018 •

edited

kara Feb 8, 2018

vicb Feb 8, 2018

kara Feb 8, 2018

kara Feb 8, 2018 •

edited

vicb Feb 8, 2018

kara Feb 8, 2018

mary-poppins commented Feb 8, 2018

mary-poppins commented Feb 10, 2018

kara left a comment

ngbot bot commented Feb 10, 2018

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

refactor(ivy): generatePropertyAliases #22082

refactor(ivy): generatePropertyAliases #22082

Conversation

vicb commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 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

vicb Feb 8, 2018 • edited

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

kara Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 8, 2018

mary-poppins commented Feb 10, 2018

kara left a comment

Choose a reason for hiding this comment

ngbot bot commented Feb 10, 2018

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

vicb Feb 8, 2018 •

edited

kara Feb 8, 2018 •

edited