A couple fixes for Ivy JIT #24565

alxhub · 2018-06-18T16:24:05Z

Support inputs and outputs from metadata.
Remove option to use default value parameter for inject() in code generation.

inject() was changed in da31db7 to not take a default value parameter, so injectable_compiler_2 should not request the use of one when using inject().

mary-poppins · 2018-06-18T17:31:47Z

You can preview 744db24 at https://pr24565-744db24.ngbuilds.io/.

alfaproject · 2018-06-18T20:25:51Z

packages/core/src/render3/jit/directive.ts

@@ -213,3 +216,10 @@ function isHostBinding(value: any): value is HostBinding {
 function isHostListener(value: any): value is HostListener {
  return value.ngMetadataName === 'HostListener';
 }
+
+function parseInputOutputs(values: string[], map: {[field: string]: string}): void {
+  values.forEach(value => {


Should this be a standard for loop or doesn't matter?

It could be. I've been taking an approach where I use ES6/fluent code unless something is provably a performance bottleneck.

I'm with @alfaproject on this, may as well write a for loop. One less function allocation per call to this function.

I find the fluent form much more readable, and it's used throughout the compiler today. I would rather not optimize prematurely. If benchmarks show that these temporary allocations (which are short lived anyway) cause significant performance issues, then we should optimize it. Until then, my goal is to keep the code as readable as possible.

I meant:

for (const value of values) { const [field, property] = value.split(',').map(piece => piece.trim()); map[field] = property || field; }

which is just as readable (I think) with the benefit of being more performant.

benlesh

A few small things, overall looks good.

benlesh · 2018-06-18T22:25:53Z

packages/core/src/render3/jit/directive.ts

@@ -213,3 +216,10 @@ function isHostBinding(value: any): value is HostBinding {
 function isHostListener(value: any): value is HostListener {
  return value.ngMetadataName === 'HostListener';
 }
+
+function parseInputOutputs(values: string[], map: {[field: string]: string}): void {


You might considier making this into a pure function that reduces into the map. It can be used to create the map above.

benlesh · 2018-06-18T22:26:51Z

packages/core/src/render3/jit/directive.ts

+
+function parseInputOutputs(values: string[], map: {[field: string]: string}): void {
+  values.forEach(value => {
+    const [field, property] = value.split(',').map(piece => piece.trim());


remove the map() here, and do the trim() on the next line where you need to. One less Array allocation.

As we discussed, I'm not worried about optimizing the allocations at the moment.

benlesh · 2018-06-18T22:27:59Z

packages/core/src/render3/jit/directive.ts

@@ -213,3 +216,10 @@ function isHostBinding(value: any): value is HostBinding {
 function isHostListener(value: any): value is HostListener {
  return value.ngMetadataName === 'HostListener';
 }
+
+function parseInputOutputs(values: string[], map: {[field: string]: string}): void {
+  values.forEach(value => {


I'm with @alfaproject on this, may as well write a for loop. One less function allocation per call to this function.

benlesh · 2018-06-18T22:28:54Z

packages/core/src/render3/jit/directive.ts

@@ -149,6 +149,9 @@ function directiveMetadata(type: Type<any>, metadata: Directive): R3DirectiveMet

  const host = extractHostBindings(metadata, propMetadata);

+  parseInputOutputs(metadata.inputs || [], inputs);
+  parseInputOutputs(metadata.outputs || [], outputs);
+
  for (let field in propMetadata) {
    propMetadata[field].forEach(ann => {


I know you didn't touch this, but is the omission of a hasOwnProperty check here intentional?

And as below, this could be a for..of loop, that will save a function allocation per turn through this loop.

The same questions/problems exist in other code below (that you didn't touch) in extractHostBindings

I know you didn't touch this, but is the omission of a hasOwnProperty check here intentional.

Can you elaborate on what the hasOwnProperty check would check? I don't follow.

Most comments addressed, please re-review.

mary-poppins · 2018-06-21T16:38:55Z

You can preview f96f81b at https://pr24565-f96f81b.ngbuilds.io/.

PR Close #24565

angular-automatic-lock-bot · 2019-09-13T18:22:45Z

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 Jun 18, 2018

fix(ivy): inject() no longer uses default value parameters

9b94685

inject() was changed in da31db7 to not take a default value parameter, so injectable_compiler_2 should not request the use of one when using inject().

alxhub force-pushed the ivy-jit-fixes branch from dd0f678 to 744db24 Compare June 18, 2018 17:21

alfaproject reviewed Jun 18, 2018

View reviewed changes

alxhub requested a review from benlesh June 18, 2018 21:05

benlesh previously requested changes Jun 18, 2018

View reviewed changes

fix(ivy): support inputs/outputs in decorator metadata in JIT

f96f81b

alxhub force-pushed the ivy-jit-fixes branch from 744db24 to f96f81b Compare June 19, 2018 18:04

benlesh approved these changes Jun 21, 2018

View reviewed changes

alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Jun 21, 2018

mhevery approved these changes Jun 21, 2018

View reviewed changes

mhevery closed this in 7d3fd4d Jun 21, 2018

mhevery pushed a commit that referenced this pull request Jun 21, 2018

fix(ivy): support inputs/outputs in decorator metadata in JIT (#24565)

7b2b1af

PR Close #24565

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

A couple fixes for Ivy JIT #24565

A couple fixes for Ivy JIT #24565

alxhub commented Jun 18, 2018

mary-poppins commented Jun 18, 2018

alfaproject Jun 18, 2018

alxhub Jun 18, 2018

benlesh Jun 18, 2018

alxhub Jun 18, 2018

alfaproject Jun 19, 2018

benlesh left a comment

benlesh Jun 18, 2018

alxhub Jun 19, 2018

benlesh Jun 18, 2018

alxhub Jun 19, 2018

benlesh Jun 18, 2018

benlesh Jun 18, 2018

benlesh Jun 18, 2018

benlesh Jun 18, 2018

alxhub Jun 18, 2018

mary-poppins commented Jun 21, 2018

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

A couple fixes for Ivy JIT #24565

A couple fixes for Ivy JIT #24565

Conversation

alxhub commented Jun 18, 2018

mary-poppins commented Jun 18, 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

benlesh left a 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

Choose a reason for hiding this comment

mary-poppins commented Jun 21, 2018

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