feat(ivy): support object and array literals in render3 #21973

kara · 2018-02-01T20:57:05Z

Currently in render3, if you pass an array literal into a binding, it will be re-created every change detection run and its binding will be re-set every change detection run.

This PR adds a new set of o1()...o8() instructions that ensure the property is only set when one of the bound expressions in the array actually changes.

This template:

<my-comp [names]="['one', myClass, 'two']"></my-comp>

currently generates this instruction:

p(0, 'names', b(['one', ctx.myClass, 'two']));

will generate this instruction:

p(0, 'names', o1(0, e0_literal, 1, ctx.myClass));
...
const e0_literal = ['one', null, 'two'];

mary-poppins · 2018-02-01T21:06:10Z

You can preview 02b6bd8 at https://pr21973-02b6bd8.ngbuilds.io/.

mhevery · 2018-02-01T21:25:52Z

packages/core/src/render3/instructions.ts

@@ -1832,6 +1832,221 @@ export function bind8(
      NO_CHANGE;
 }

+/**


instructions.ts is getting large as is. Can we move these into a separate file?

mhevery · 2018-02-01T21:28:13Z

packages/core/src/render3/instructions.ts

+}
+
+/** Updates four expressions in an array literal if they have changed. */
+function updateBinding4(


Building it up like this is clever. :-)

mhevery · 2018-02-01T21:30:45Z

packages/core/src/render3/instructions.ts

+ */
+export function memoize2(arr: any[], index1: number, exp1: any, index2: number, exp2: any): any[]|
+    NO_CHANGE {
+  return updateBinding2(arr, index1, exp1, index2, exp2) ? arr.slice() : NO_CHANGE;


I don't think this is right. If I am reading correctly you update the arr and than you make a copy. But arr is the prototype, which means that you are updating it for everyone. I think the correct behavior is to 1) determine if there are changes 2) clone the arr and 3) assign the changes to the array.

Discussed offline. For anyone curious: the current algorithm works because we are comparing bindings against themselves instead of against the array that is changing. But changing the const array that is generated by the compiler will cause headaches in the compiler code, so instead we will save a copy on TView.

mhevery · 2018-02-01T21:32:42Z

packages/core/test/render3/compiler_canonical_spec.ts

+        const e0_literal = ['Nancy', null];
+        // /NORMATIVE
+
+        expect(renderComp(MyApp)).toEqual(`<my-comp><p>Nancy</p><p>Bess</p></my-comp>`);


assert that e0_literal has not been changed. (I believe that your code changes it which is incorrect.)

mary-poppins · 2018-02-01T23:34:43Z

You can preview b4793f2 at https://pr21973-b4793f2.ngbuilds.io/.

mary-poppins · 2018-02-01T23:56:44Z

You can preview a16c5db at https://pr21973-a16c5db.ngbuilds.io/.

mhevery

I think the same exact code with few minor changes should work for object literals as well as for arrays.

mhevery · 2018-02-02T00:08:03Z

packages/core/src/render3/object_literal.ts

+ * @param exp Expression to set at index
+ * @returns Whether or not there has been a change
+ */
+function updateArrBinding(arr: any[], index: number, exp: any): boolean {


I think this can be updateBinding since Array and Object will have the same exact code.

mhevery · 2018-02-02T00:09:21Z

packages/core/src/render3/object_literal.ts

+
+
+function getObjectCopy(index: number, arr: any[]): any[] {
+  const tView = getTView();


can you add assert that index comes in the right order? so that we don't create a spares array.

mhevery · 2018-02-02T00:10:49Z

packages/core/src/render3/object_literal.ts

+  const objectLiterals = tView.objectLiterals;
+  return objectLiterals && index < objectLiterals.length ?
+      objectLiterals[index] :
+      (objectLiterals || (tView.objectLiterals = []))[index] = arr.slice();


If you do something like Array.isArray(arr) ? arr.slice() : Object.create(Object, arr). it should work for both array and object. You may want to rename arr => obj

mhevery · 2018-02-02T00:13:06Z

packages/core/src/render3/object_literal.ts

+}
+
+
+function getObjectCopy(index: number, arr: any[]): any[] {


Can we call it getMutableBlueprint?

mary-poppins · 2018-02-05T19:45:11Z

You can preview b1ce536 at https://pr21973-b1ce536.ngbuilds.io/.

kara · 2018-02-06T01:35:16Z

presubmit

vicb · 2018-02-06T20:00:36Z

packages/core/src/render3/object_literal.ts

+}
+
+/** Updates two expressions in an object literal if they have changed. */
+function updateBinding2(obj: any, key1: number, exp1: any, key2: number, exp2: any): boolean {


string | number (multiples)

Good catch. Can include in my follow-up here #22045

vicb · 2018-02-06T20:01:29Z

packages/core/src/render3/object_literal.ts

+
+
+/**
+ * Updates an expression in an object literal if the expression has changed.


object literal should be object literal or array

vicb · 2018-02-06T20:03:22Z

packages/core/src/render3/object_literal.ts

+ * @param exp Expression to set at key
+ * @returns A copy of the object or NO_CHANGE
+ */
+export function objectLiteral1(objIndex: number, obj: any, key: string | number, exp: any): any {


objectLiteral is confusing wrt to array ?

Since the instruction encompasses both, @mhevery and I discussed using "object" as the more general of the two.

vicb · 2018-02-06T20:03:47Z

packages/core/src/render3/object_literal.ts

+ * @param key1
+ * @param exp1
+ * @param key2
+ * @param exp2


remove undocumented params ?

PR Close angular#21973

angular-automatic-lock-bot · 2019-09-14T04:22:43Z

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

feat(ivy): memoize array literals in render3

02b6bd8

googlebot added the cla: yes label Feb 1, 2018

mhevery suggested changes Feb 1, 2018

View reviewed changes

fixup! feat(ivy): memoize array literals in render3

b4793f2

kara added action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy labels Feb 1, 2018

kara added the target: major This PR is targeted for the next major release label Feb 1, 2018

fixup! feat(ivy): memoize array literals in render3

a16c5db

kara changed the title ~~feat(ivy): memoize array literals in render3~~ feat(ivy): support array literals in render3 Feb 1, 2018

mhevery suggested changes Feb 2, 2018

View reviewed changes

fixup! feat(ivy): memoize array literals in render3

b1ce536

kara changed the title ~~feat(ivy): support array literals in render3~~ feat(ivy): support object and array literals in render3 Feb 5, 2018

mhevery approved these changes Feb 5, 2018

View reviewed changes

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 5, 2018

kara added the action: merge The PR is ready for merge by the caretaker label Feb 6, 2018

alxhub closed this in 4d62be6 Feb 6, 2018

vicb reviewed Feb 6, 2018

View reviewed changes

packages/core/src/render3/object_literal.ts

* @param key1

* @param exp1

* @param key2

* @param exp2

Copy link

Contributor

vicb Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove undocumented params ?

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

feat(ivy): memoize array literals in render3 (angular#21973)

829d87d

PR Close angular#21973

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

feat(ivy): memoize array literals in render3 (angular#21973)

4404198

PR Close angular#21973

kara deleted the memoize branch October 13, 2018 01:09

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ivy): support object and array literals in render3 #21973

feat(ivy): support object and array literals in render3 #21973

kara commented Feb 1, 2018 •

edited

mary-poppins commented Feb 1, 2018

mhevery Feb 1, 2018

mhevery Feb 1, 2018

mhevery Feb 1, 2018

kara Feb 1, 2018 •

edited

mhevery Feb 1, 2018

mary-poppins commented Feb 1, 2018

mary-poppins commented Feb 1, 2018

mhevery left a comment

mhevery Feb 2, 2018

mhevery Feb 2, 2018

mhevery Feb 2, 2018

mhevery Feb 2, 2018

mary-poppins commented Feb 5, 2018

kara commented Feb 6, 2018

vicb Feb 6, 2018 •

edited

kara Feb 6, 2018

vicb Feb 6, 2018

vicb Feb 6, 2018

kara Feb 6, 2018

vicb Feb 6, 2018

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



		function getObjectCopy(index: number, arr: any[]): any[] {
		const tView = getTView();



		/**
		* Updates an expression in an object literal if the expression has changed.

feat(ivy): support object and array literals in render3 #21973

feat(ivy): support object and array literals in render3 #21973

Conversation

kara commented Feb 1, 2018 • edited

mary-poppins commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 1, 2018

mary-poppins commented Feb 1, 2018

mhevery 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

mary-poppins commented Feb 5, 2018

kara commented Feb 6, 2018

vicb Feb 6, 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

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

kara commented Feb 1, 2018 •

edited

kara Feb 1, 2018 •

edited

vicb Feb 6, 2018 •

edited