perf(ivy): move attributes array into component def #32798

crisbeto · 2019-09-21T08:18:56Z

Currently Ivy stores the element attributes into an array above the component def and passes it into the relevant instructions, however the problem is that upon minification the array will get a unique name which won't compress very well. These changes move the attributes array into the component def and pass in the index into the instructions instead.

Before:

const _c0 = ['foo', 'bar'];

SomeComp.ngComponentDef = defineComponent({
  template: function() {
    element(0, 'div', _c0);
  }
});

After:

SomeComp.ngComponentDef = defineComponent({
  consts: [['foo', 'bar']],
  template: function() {
    element(0, 'div', 0);
  }
});

A couple of cases that this PR doesn't handle:

Template references are still in a separate array.
i18n attributes are still in a separate array.

packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts

packages/compiler/src/render3/view/template.ts

packages/core/src/render3/definition.ts

crisbeto · 2019-09-23T18:37:08Z

@mhevery I've reworked it based on the feedback. Can you take another look?

packages/core/src/render3/VIEW_DATA.md

packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts

packages/core/test/render3/perf/setup.ts

packages/core/test/render3/render_util.ts

kara · 2019-10-03T00:07:03Z

packages/compiler/src/render3/view/template.ts

+      const literal = o.literalArr(constExprs);
+
+      // Try to reuse a literal that's already in the array, if possible.
+      for (let i = 0; i < this._constants.length; i++) {


Going through the constants list every time we see an attribute seems like it might be slower. Could we re-use some of the existing constant pool logic here to de-dupe?

kara · 2019-10-03T02:40:47Z

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

  const lView = getLView();
  const tView = lView[TVIEW];
+  const tViewConsts = tView.consts;
+  const consts = tViewConsts === null || constsIndex == null ? null : tViewConsts[constsIndex];


Have we checked yet if there's any perf impact of the extra read using the micro-benchmarks?

Also, wouldn't constIndex be null if tViewConsts is null? Could we remove the tViewConsts === null check?

Suggested change

const consts = tViewConsts === null || constsIndex == null ? null : tViewConsts[constsIndex];

const consts = constsIndex == null ? null : tViewConsts[constsIndex];

I haven't run any if the benchmarks yet. Also the constsIndex could be null if we need to skip over it to pass in the localRefs param. I suppose we can get rid of it once the local refs are in the consts array.

kara · 2019-10-03T03:01:54Z

Compression diff

Before (left), this PR (right)

Red squares -> yellow, showing some improvement

petebacondarwin · 2019-10-07T13:55:44Z

Would it be possible to make consts a function that is called once before the template function is first called? This would allow us to avoid running initialization code before it is needed.

It would be particularly useful for the $localize calls. Currently $localize is being called as soon as the component is imported into the application. This makes it problematic to find a suitable moment to load translations. Currently all these I18N_... constants get initialized so early it is really hard to ensure that runtime translations are loaded early enough.

crisbeto · 2019-10-07T15:19:39Z

Makes sense to me @petebacondarwin. The question is whether we want to get these changes in and do a follow-up that turns it into a function and moves the i18n and references arrays into it.

mhevery · 2019-10-07T21:58:48Z

Makes sense to me @petebacondarwin. The question is whether we want to get these changes in and do a follow-up that turns it into a function and moves the i18n and references arrays into it.

I vote to land this and follow up later.

kara

LGTM, but this is still blocked on running the microbenchmarks

Currently Ivy stores the element attributes into an array above the component def and passes it into the relevant instructions, however the problem is that upon minification the array will get a unique name which won't compress very well. These changes move the attributes array into the component def and pass in the index into the instructions instead. Before: ``` const _c0 = ['foo', 'bar']; SomeComp.ngComponentDef = defineComponent({ template: function() { element(0, 'div', _c0); } }); ``` After: ``` SomeComp.ngComponentDef = defineComponent({ consts: [['foo', 'bar']], template: function() { element(0, 'div', 0); } }); ``` A couple of cases that this PR doesn't handle: * Template references are still in a separate array. * i18n attributes are still in a separate array.

kara · 2019-10-08T18:18:46Z

@crisbeto Thanks for doing that! Looks like this PR gives us a slight perf edge, so we're definitely good to merge.

alxhub · 2019-10-08T20:06:40Z

Presubmit
Ivy Presubmit

mhevery

LGTM

Can we do a follow up PR:

embedded template functions should also be moved to consts
Also include localization strings in the consts.
Change the ComponentDef.const to have a function which returns the arrays. The reason for this is that @ocombe and @petebacondarwin need a way to delay the execution of the $localize until after locale strings are loaded. By putting it into the closure this can be achieved. When TView is created and we copy the consts from ComponentDef to the TView we can execute the closure which will in turn execute the $localize and we would cache the resulting values in TView

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

ngbot · 2019-10-09T18:04:16Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "google3" is failing
1 pending code review

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.

kara · 2019-10-09T18:05:37Z

merge-assistance: global approval

Follow-up from angular#32798. Moves the local references array into the component def's `consts` in order to make it compress better. Before: ``` const _c0 = ['foo', '']; SomeComp.ngComponentDef = defineComponent({ template: function() { element(0, 'div', null, _c0); } }); ``` After: ``` SomeComp.ngComponentDef = defineComponent({ consts: [['foo', '']], template: function() { element(0, 'div', null, 0); } }); ```

Currently Ivy stores the element attributes into an array above the component def and passes it into the relevant instructions, however the problem is that upon minification the array will get a unique name which won't compress very well. These changes move the attributes array into the component def and pass in the index into the instructions instead. Before: ``` const _c0 = ['foo', 'bar']; SomeComp.ngComponentDef = defineComponent({ template: function() { element(0, 'div', _c0); } }); ``` After: ``` SomeComp.ngComponentDef = defineComponent({ consts: [['foo', 'bar']], template: function() { element(0, 'div', 0); } }); ``` A couple of cases that this PR doesn't handle: * Template references are still in a separate array. * i18n attributes are still in a separate array. PR Close angular#32798

Follow-up from angular#32798. Moves the local references array into the component def's `consts` in order to make it compress better. Before: ``` const _c0 = ['foo', '']; SomeComp.ngComponentDef = defineComponent({ template: function() { element(0, 'div', null, _c0); } }); ``` After: ``` SomeComp.ngComponentDef = defineComponent({ consts: [['foo', '']], template: function() { element(0, 'div', null, 0); } }); ```

Follow-up from #32798. Moves the local references array into the component def's `consts` in order to make it compress better. Before: ``` const _c0 = ['foo', '']; SomeComp.ngComponentDef = defineComponent({ template: function() { element(0, 'div', null, _c0); } }); ``` After: ``` SomeComp.ngComponentDef = defineComponent({ consts: [['foo', '']], template: function() { element(0, 'div', null, 0); } }); ``` PR Close #33129

Follow-up from angular#32798. Moves the local references array into the component def's `consts` in order to make it compress better. Before: ``` const _c0 = ['foo', '']; SomeComp.ngComponentDef = defineComponent({ template: function() { element(0, 'div', null, _c0); } }); ``` After: ``` SomeComp.ngComponentDef = defineComponent({ consts: [['foo', '']], template: function() { element(0, 'div', null, 0); } }); ``` PR Close angular#33129

angular-automatic-lock-bot · 2019-11-09T16:07:52Z

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 Sep 21, 2019

crisbeto force-pushed the FW-1545/attrs-on-def branch 2 times, most recently from b419ac2 to b90be9e Compare September 21, 2019 08:38

crisbeto added 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 Sep 21, 2019

ngbot bot modified the milestone: needsTriage Sep 21, 2019

crisbeto marked this pull request as ready for review September 21, 2019 08:56

crisbeto requested review from a team as code owners September 21, 2019 08:56

mhevery suggested changes Sep 23, 2019

View reviewed changes

packages/compiler-cli/test/compliance/r3_view_compiler_listener_spec.ts Outdated Show resolved Hide resolved

packages/compiler/src/render3/view/template.ts Outdated Show resolved Hide resolved

packages/core/src/render3/definition.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the FW-1545/attrs-on-def branch from b90be9e to 025ba88 Compare September 23, 2019 18:27

crisbeto requested review from IgorMinar and a team as code owners September 23, 2019 18:27

crisbeto force-pushed the FW-1545/attrs-on-def branch from 025ba88 to d110496 Compare September 23, 2019 18:29

kara requested changes Oct 3, 2019

View reviewed changes

crisbeto force-pushed the FW-1545/attrs-on-def branch from d110496 to 9e3d17b Compare October 3, 2019 20:31

kara approved these changes Oct 7, 2019

View reviewed changes

kara added the state: blocked label Oct 7, 2019

kara assigned crisbeto Oct 8, 2019

crisbeto added 3 commits October 8, 2019 19:31

fixup! perf(ivy): move attributes array into component def

a4510a9

fixup! perf(ivy): move attributes array into component def

73d111b

crisbeto removed the state: blocked label Oct 8, 2019

kara assigned mhevery Oct 8, 2019

mhevery approved these changes Oct 9, 2019

View reviewed changes

packages/core/src/render3/interfaces/definition.ts Show resolved Hide resolved

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 9, 2019

kara unassigned mhevery Oct 9, 2019

kara added the action: merge The PR is ready for merge by the caretaker label Oct 9, 2019

kara added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 9, 2019

alxhub closed this in d5b87d3 Oct 9, 2019

crisbeto mentioned this pull request Oct 13, 2019

perf(ivy): move local references into consts array #33129

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ivy): move attributes array into component def #32798

perf(ivy): move attributes array into component def #32798

crisbeto commented Sep 21, 2019 •

edited

crisbeto commented Sep 23, 2019

kara Oct 3, 2019

kara Oct 3, 2019

crisbeto Oct 3, 2019

kara commented Oct 3, 2019 •

edited

petebacondarwin commented Oct 7, 2019

crisbeto commented Oct 7, 2019

mhevery commented Oct 7, 2019

kara left a comment

kara commented Oct 8, 2019

alxhub commented Oct 8, 2019

mhevery left a comment •

edited

ngbot bot commented Oct 9, 2019

kara commented Oct 9, 2019

angular-automatic-lock-bot bot commented Nov 9, 2019

	const consts = tViewConsts === null \|\| constsIndex == null ? null : tViewConsts[constsIndex];
	const consts = constsIndex == null ? null : tViewConsts[constsIndex];

perf(ivy): move attributes array into component def #32798

perf(ivy): move attributes array into component def #32798

Conversation

crisbeto commented Sep 21, 2019 • edited

crisbeto commented Sep 23, 2019

kara Oct 3, 2019

Choose a reason for hiding this comment

kara Oct 3, 2019

Choose a reason for hiding this comment

crisbeto Oct 3, 2019

Choose a reason for hiding this comment

kara commented Oct 3, 2019 • edited

petebacondarwin commented Oct 7, 2019

crisbeto commented Oct 7, 2019

mhevery commented Oct 7, 2019

kara left a comment

Choose a reason for hiding this comment

kara commented Oct 8, 2019

alxhub commented Oct 8, 2019

mhevery left a comment • edited

Choose a reason for hiding this comment

ngbot bot commented Oct 9, 2019

kara commented Oct 9, 2019

angular-automatic-lock-bot bot commented Nov 9, 2019

crisbeto commented Sep 21, 2019 •

edited

kara commented Oct 3, 2019 •

edited

mhevery left a comment •

edited