feat(ivy): add support for short-circuiting #24039

vicb · 2018-05-21T23:00:03Z

Short-circuitable expressions (using ternary & binary operators) could not use
the regular binding mechanism as it relies on the bindings being checked every
single time - the index is incremented as part of checking the bindings.

Then for pure function kind of bindings we use a different mechanism with a
fixed index. As such short circuiting a binding check does not mess with the
expected binding index.

Note that all pure function bindings are handled the same wether or not they
actually are short-circuitable. This allows to keep the compiler and compiled
code simple - and there is no runtime perf cost anyway.

vicb · 2018-05-21T23:03:50Z

packages/compiler/test/render3/r3_compiler_compliance_spec.ts

@@ -494,11 +546,12 @@ describe('compiler compliance', () => {
              if (rf & 1) {
                $r3$.ɵE(0, 'my-comp');
                $r3$.ɵe();
+                i0.ɵrS(10);


i0 -> $r3$ - everywhere updated

vicb · 2018-05-21T23:04:18Z

packages/core/src/render3/instructions.ts

+ * Binding for pure functions are store after the LNodes in the data array but before the binding.
+ *
+ *  ---------------------------------------------------------------------------
+ *  |  LNodes ... | pure function bindings | regular binding / interpolations |


reg bindingS

mary-poppins · 2018-05-21T23:12:25Z

You can preview e81bc71 at https://pr24039-e81bc71.ngbuilds.io/.

mary-poppins · 2018-05-21T23:34:37Z

You can preview 28e7953 at https://pr24039-28e7953.ngbuilds.io/.

vicb · 2018-05-21T23:34:52Z

packages/core/test/render3/compiler_canonical/component_directives_spec.ts

            }
            if (rf & 2) {
+              // TODO(vicb): Shouldn't bind be the outer call ?


@mhevery the code needs to be updated here, right ?

correct, it needs to be wrapped in bind

mary-poppins · 2018-05-21T23:45:52Z

You can preview f399591 at https://pr24039-f399591.ngbuilds.io/.

mary-poppins · 2018-05-22T00:15:53Z

You can preview 8662643 at https://pr24039-8662643.ngbuilds.io/.

mhevery · 2018-05-22T04:36:01Z

packages/compiler/test/render3/r3_compiler_compliance_spec.ts

+            i0.ɵp(0, 'ternary', i0.ɵb((ctx.cond? i0.ɵf1(2, _c0, ctx.a): _c1)));
+            i0.ɵp(0, 'pipe', i0.ɵb(i0.ɵpb3(6, 1, ctx.value, 1, 2)));
+            i0.ɵp(0, 'and', i0.ɵb((ctx.cond && i0.ɵf1(4, _c0, ctx.b))));
+            i0.ɵp(0, 'or', i0.ɵb((ctx.cond || i0.ɵf1(6, _c0, ctx.c))));


This does not look right to me. 6 is already used 2 lines above. Are we double using the slots?

2 lines above is i0.ɵpb3(6, 1, ctx.value, 1, 2)

6 is the pipe

1 is the offset of the reserved slot <- this one compares to this 6. No error here.

However i0 should be $r3$ . I'll update that part.

mhevery · 2018-05-22T04:39:40Z

packages/compiler/test/render3/r3_compiler_compliance_spec.ts

              }
              if (rf & 2) {
                $r3$.ɵp(
                    0, 'names',
-                    $r3$.ɵb($r3$.ɵfV($e0_ff$, ctx.n0, ctx.n1, ctx.n2, ctx.n3, ctx.n4, ctx.n5, ctx.n6, ctx.n7, ctx.n8)));
+                    $r3$.ɵb($r3$.ɵfV(10, $e0_ff$, ctx.n0, ctx.n1, ctx.n2, ctx.n3, ctx.n4, ctx.n5, ctx.n6, ctx.n7, ctx.n8)));


This seems wrong. ɵfV(10, $e0_ff$, [ctx.n0, ctx.n1, ctx.n2, ctx.n3, ctx.n4, ctx.n5, ctx.n6, ctx.n7, ctx.n8])) (note that args should be in array)

I think at some point Chuck used that to prevent building an array. But good catch. I'll update (compiler + test).
This error is not introduced by the current PR but was here before

mhevery · 2018-05-22T04:41:59Z

packages/core/src/render3/index.ts

@@ -64,6 +64,8 @@ export {
  text as T,
  textBinding as t,

+  reserveSlots as rS,


I am starting to think that maybe we should have a general instruction

function reserve( elementCount: number, bindingCount: number, pureFnCount?: number, pipeCount?: number, ): void;

This instruction would be always required as first instruction and it would allocate the data. What do you think?

Might be.
I think we discussed reserving the bindingCount part today (current issue with undefined not being detected as a change).
Should be designed/agreed, I would tend to do this in a follow up PR. WDYT ?

mhevery · 2018-05-22T04:45:31Z

packages/core/src/render3/pure_function.ts

-export function pureFunctionV(pureFn: (...v: any[]) => any, exps: any[], thisArg?: any): any {
-  let different = false;
+export function pureFunctionV(
+    slotOffset: number, pureFn: (...v: any[]) => any, exps: any[], thisArg?: any): any {


Note exps is any[] hence pureFunctionV must take args as [arg0, arg1, ...]

Agreed, will fix (this code was not changed by this PR).

mhevery · 2018-05-22T04:47:55Z

packages/core/test/render3/compiler_canonical/component_directives_spec.ts

            }
            if (rf & 2) {
+              // TODO(vicb): Shouldn't bind be the outer call ?


correct, it needs to be wrapped in bind

mhevery · 2018-05-22T04:49:53Z

packages/core/test/render3/compiler_canonical/pipes_spec.ts

-            $r3$.ɵt(0, $r3$.ɵi1('', $r3$.ɵpb2(1, $r3$.ɵpb2(2, ctx.name, ctx.size), ctx.size), ''));
+            $r3$.ɵt(
+                0,
+                $r3$.ɵi1('', $r3$.ɵpb2(1, 6, $r3$.ɵpb2(2, 3, ctx.name, ctx.size), ctx.size), ''));


same here, missing bind.

Thinking more about it, I think we should just inline bind into text, property, attribute instruction. This way it will be less code to generate. Maybe follow up PR?

This one is actually correct. interpolationX returns NO_CHANGE.

I don't want to change the behavior or textBinding, ... in this PR. Out of scope.

@mhevery I was thinking the same thing, but that isn't trivial for cases where interpolation is relevant. Please refer to #23881 and #23923.

vicb · 2018-05-22T05:37:22Z

@mhevery Thanks for the review. I'll update this PR first thing tomorrow morning.

mary-poppins · 2018-05-22T18:20:47Z

You can preview a973202 at https://pr24039-a973202.ngbuilds.io/.

mary-poppins · 2018-05-22T18:25:12Z

You can preview fd9a07a at https://pr24039-fd9a07a.ngbuilds.io/.

vicb · 2018-05-22T21:01:26Z

http://test/OCL:197619135:BASE:197619176:1527022871189:d520265

mary-poppins · 2018-05-22T21:04:24Z

You can preview 4adff90 at https://pr24039-4adff90.ngbuilds.io/.

Short-circuitable expressions (using ternary & binary operators) could not use the regular binding mechanism as it relies on the bindings being checked every single time - the index is incremented as part of checking the bindings. Then for pure function kind of bindings we use a different mechanism with a fixed index. As such short circuiting a binding check does not mess with the expected binding index. Note that all pure function bindings are handled the same wether or not they actually are short-circuitable. This allows to keep the compiler and compiled code simple - and there is no runtime perf cost anyway.

mary-poppins · 2018-05-23T17:45:55Z

You can preview bb679b3 at https://pr24039-bb679b3.ngbuilds.io/.

kara

FYI found some minor typos (probably for a follow-up, since this already has a merge label).

kara · 2018-05-24T17:08:23Z

packages/core/src/render3/instructions.ts

+}
+
+/**
+ * Sets up the binding index before execute any `pureFunctionX` instructions.


before execute -> before executing

kara · 2018-05-24T17:08:37Z

packages/core/src/render3/instructions.ts

+/**
+ * Reserves slots for pure functions (`pureFunctionX` instructions)
+ *
+ * Binding for pure functions are store after the LNodes in the data array but before the binding.


Binding for pure functions -> Bindings for pure functions
are store -> are stored

kara · 2018-05-24T17:09:31Z

packages/core/src/render3/instructions.ts

+ */
+export function reserveSlots(numSlots: number) {
+  // Init the slots with a unique `NO_CHANGE` value so that the first change is always detected
+  // whether is happens or not during the first change detection pass - pure functions checks


is happens -> it happens

kara · 2018-05-24T17:10:53Z

packages/core/src/render3/instructions.ts

@@ -2365,6 +2414,22 @@ function assertDataNext(index: number, arr?: any[]) {
      arr.length, index, `index ${index} expected to be at the end of arr (length ${arr.length})`);
 }

+/**
+ * On the first template pass the reserved slots should be set `NO_CHANGE`.


On the first template pass the reserved slots -> On the first template pass, the reserved slots

PR Close #24039

angular-automatic-lock-bot · 2019-09-13T22:21:40Z

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 May 21, 2018

vicb requested a review from mhevery May 21, 2018 23:00

vicb added feature Issue that requests a new feature comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels May 21, 2018

vicb commented May 21, 2018

View reviewed changes

vicb force-pushed the 0516-shc branch 2 times, most recently from 28e7953 to f399591 Compare May 21, 2018 23:34

vicb commented May 21, 2018

View reviewed changes

vicb force-pushed the 0516-shc branch from f399591 to 8662643 Compare May 22, 2018 00:04

mhevery suggested changes May 22, 2018

View reviewed changes

vicb force-pushed the 0516-shc branch from fd9a07a to 4adff90 Compare May 22, 2018 20:49

vicb added 7 commits May 23, 2018 10:25

fixup! feat(ivy): add support for short-circuiting

e807be0

refactor(ivy): remove insignificant '...' in the compiler tests

74b4cdf

fix(ivy): pureFunctionV takes an array of values

05c3be8

fix(ivy): pipeBindV takes an array of values

ba9cb83

fixup! fix(ivy): pureFunctionV takes an array of values

ec82b12

fixup! fix(ivy): pipeBindV takes an array of values

f0f85ab

fixup! fix(ivy): pureFunctionV takes an array of values

bb679b3

vicb force-pushed the 0516-shc branch from 4adff90 to bb679b3 Compare May 23, 2018 17:33

mhevery approved these changes May 23, 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 May 23, 2018

kara reviewed May 24, 2018

View reviewed changes

matsko closed this in 4f36340 May 25, 2018

matsko pushed a commit that referenced this pull request May 25, 2018

refactor(ivy): remove insignificant '...' in the compiler tests (#24039)

280a784

PR Close #24039

matsko pushed a commit that referenced this pull request May 25, 2018

fix(ivy): pureFunctionV takes an array of values (#24039)

188ff84

PR Close #24039

matsko pushed a commit that referenced this pull request May 25, 2018

fix(ivy): pipeBindV takes an array of values (#24039)

729c797

PR Close #24039

kara mentioned this pull request Jun 1, 2018

refactor(ivy): convert TNode.index to number, general cleanup #24260

Closed

vicb deleted the 0516-shc branch August 3, 2018 19:08

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

feat(ivy): add support for short-circuiting #24039

feat(ivy): add support for short-circuiting #24039

vicb commented May 21, 2018 •

edited

vicb May 21, 2018

vicb May 21, 2018

mary-poppins commented May 21, 2018

mary-poppins commented May 21, 2018

vicb May 21, 2018 •

edited

mhevery May 22, 2018

mary-poppins commented May 21, 2018

mary-poppins commented May 22, 2018

mhevery May 22, 2018

vicb May 22, 2018 •

edited

mhevery May 22, 2018

vicb May 22, 2018

mhevery May 22, 2018

vicb May 22, 2018

mhevery May 23, 2018

mhevery May 22, 2018

vicb May 22, 2018

mhevery May 22, 2018

mhevery May 22, 2018

vicb May 22, 2018 •

edited

JoostK May 22, 2018

vicb commented May 22, 2018

mary-poppins commented May 22, 2018

mary-poppins commented May 22, 2018

vicb commented May 22, 2018

mary-poppins commented May 22, 2018

mary-poppins commented May 23, 2018

kara left a comment

kara May 24, 2018

kara May 24, 2018

kara May 24, 2018

kara May 24, 2018

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

feat(ivy): add support for short-circuiting #24039

feat(ivy): add support for short-circuiting #24039

Conversation

vicb commented May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented May 21, 2018

mary-poppins commented May 21, 2018

vicb May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented May 21, 2018

mary-poppins commented May 22, 2018

Choose a reason for hiding this comment

vicb May 22, 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

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

vicb May 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented May 22, 2018

mary-poppins commented May 22, 2018

mary-poppins commented May 22, 2018

vicb commented May 22, 2018

mary-poppins commented May 22, 2018

mary-poppins commented May 23, 2018

kara 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

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

vicb commented May 21, 2018 •

edited

vicb May 21, 2018 •

edited

vicb May 22, 2018 •

edited

vicb May 22, 2018 •

edited