perf(common): code size reduction of `ngFor` directive #44315

JoostK · 2021-11-30T20:07:51Z

This commit makes several changes to the implementation of NgForOf to
reduce its code size in production builds:

The tailor-made message for an unsupported differ is fully
tree-shaken in production builds, in favor of the exception from the
differ factory itself.
The private _perViewChange method was changed into a free-standing
function, to allow its name to be minimized.
The need for an intermediate RecordViewTuple was avoided by
applying the operation in-place, instead of collection all insertions
into a buffer first. This is safe as the _perViewChange operation
that used to be done on each RecordViewTuple is entirely local to
the tuple itself. Hence, it is invariant to execution ordering which
means that the _perViewChange can be executed directly during the
forEachOperation loop.

JoostK · 2021-11-30T21:00:29Z

Note: the total size savings for the todo example app is 395 bytes. The removal of the insertion tuples buffer also reduces runtime memory pressure.

AndrewKushnir

This PR is in the draft mode (possibly in the "work in progress" state), but I wanted to leave a quick comment with an additional idea anyways :)

AndrewKushnir · 2021-12-01T01:45:41Z

packages/common/src/directives/ng_for_of.ts

@@ -234,42 +237,32 @@ export class NgForOf<T, U extends NgIterable<T> = NgIterable<T>> implements DoCh
            const view = this._viewContainer.createEmbeddedView(
                this._template, new NgForOfContext<T, U>(null!, this._ngForOf!, -1, -1),
                currentIndex === null ? undefined : currentIndex);
-            const tuple = new RecordViewTuple<T, U>(item, view);
-            insertTuples.push(tuple);
+            applyViewChange(view, item);
          } else if (currentIndex == null) {
            this._viewContainer.remove(
                adjustedPreviousIndex === null ? undefined : adjustedPreviousIndex);
          } else if (adjustedPreviousIndex !== null) {
            const view = this._viewContainer.get(adjustedPreviousIndex)!;
            this._viewContainer.move(view, currentIndex);


The changes look great, thanks @JoostK 👍

One more idea that we can explore is to extract the this._viewContainer into a local const, so it's better minified:

const viewContainer = this._viewContainer; // ... viewContainer.move(view, currentIndex);

would be minified to something like this:

var v = this._viewContainer; // ... v.move(view, currentIndex);

and given the fact that we have 7 instances of this._viewContainer inside the function - we'd get more savings.

Also, that may be a tiny perf runtime improvement as well, since we'd avoid extra property reads.

Thanks Andrew, that's an excellent idea!

JoostK · 2021-12-01T12:49:49Z

Cool, we're now at 497 fewer bytes! I'll see if I can get the tests green (mostly golden updates) before marking this for review.

AndrewKushnir

@JoostK thanks for the perf improvement 👍

packages/common/src/directives/ng_for_of.ts

AndrewKushnir · 2021-12-01T19:59:02Z

Presubmit.

AndrewKushnir

Reviewed-for: size-tracking

AndrewKushnir · 2021-12-01T21:49:05Z

Merge assistance: presubmits for this change are successful.

jessicajaniuk

LGTM 🍪

reviewed-for: size-tracking

This commit makes several changes to the implementation of `NgForOf` to reduce its code size in production builds: 1. The tailor-made message for an unsupported differ is fully tree-shaken in production builds, in favor of the exception from the differ factory itself. 2. The private `_perViewChange` method was changed into a free-standing function, to allow its name to be minimized. 3. The need for an intermediate `RecordViewTuple` was avoided by applying the operation in-place, instead of collecting all insertions into a buffer first. This is safe as the `_perViewChange` operation that used to be done on each `RecordViewTuple` is entirely local to the tuple itself. Hence, it is invariant to execution ordering which means that the `_perViewChange` can be executed directly during the `forEachOperation` loop.

AndrewKushnir · 2021-12-02T16:49:54Z

FYI, there was a problem merging this PR, so I did a rebase. Will try to merge it again once CI is completed.

AndrewKushnir · 2021-12-02T17:25:48Z

This PR was merged into the repository by commit 1336297.

This commit makes several changes to the implementation of `NgForOf` to reduce its code size in production builds: 1. The tailor-made message for an unsupported differ is fully tree-shaken in production builds, in favor of the exception from the differ factory itself. 2. The private `_perViewChange` method was changed into a free-standing function, to allow its name to be minimized. 3. The need for an intermediate `RecordViewTuple` was avoided by applying the operation in-place, instead of collecting all insertions into a buffer first. This is safe as the `_perViewChange` operation that used to be done on each `RecordViewTuple` is entirely local to the tuple itself. Hence, it is invariant to execution ordering which means that the `_perViewChange` can be executed directly during the `forEachOperation` loop. PR Close #44315

This commit makes several changes to the implementation of `NgForOf` to reduce its code size in production builds: 1. The tailor-made message for an unsupported differ is fully tree-shaken in production builds, in favor of the exception from the differ factory itself. 2. The private `_perViewChange` method was changed into a free-standing function, to allow its name to be minimized. 3. The need for an intermediate `RecordViewTuple` was avoided by applying the operation in-place, instead of collecting all insertions into a buffer first. This is safe as the `_perViewChange` operation that used to be done on each `RecordViewTuple` is entirely local to the tuple itself. Hence, it is invariant to execution ordering which means that the `_perViewChange` can be executed directly during the `forEachOperation` loop. PR Close angular#44315

angular-automatic-lock-bot · 2022-01-02T16:05:54Z

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

JoostK added area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release labels Nov 30, 2021

ngbot bot added this to the Backlog milestone Nov 30, 2021

google-cla bot added the cla: yes label Nov 30, 2021

AndrewKushnir self-requested a review November 30, 2021 20:17

sonukapoor approved these changes Dec 1, 2021

View reviewed changes

AndrewKushnir reviewed Dec 1, 2021

View reviewed changes

JoostK marked this pull request as ready for review December 1, 2021 16:44

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 1, 2021

JoostK requested a review from AndrewKushnir December 1, 2021 16:44

AndrewKushnir approved these changes Dec 1, 2021

View reviewed changes

packages/common/src/directives/ng_for_of.ts Show resolved Hide resolved

AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 1, 2021

AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit cross-cutting: tree-shaking area: performance and removed cross-cutting: tree-shaking labels Dec 1, 2021

JoostK force-pushed the core/smaller-ngforof branch from ce68a0d to 5f697f6 Compare December 1, 2021 21:06

AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Dec 1, 2021

AndrewKushnir approved these changes Dec 1, 2021

View reviewed changes

pullapprove bot requested review from alxhub, dylhunn, jelbourn and jessicajaniuk December 1, 2021 21:48

jessicajaniuk approved these changes Dec 1, 2021

View reviewed changes

AndrewKushnir force-pushed the core/smaller-ngforof branch from 5f697f6 to a724d4b Compare December 2, 2021 16:49

AndrewKushnir removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Dec 2, 2021

AndrewKushnir removed request for jelbourn, alxhub and dylhunn December 2, 2021 17:24

AndrewKushnir closed this in 1336297 Dec 2, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(common): code size reduction of `ngFor` directive #44315

perf(common): code size reduction of `ngFor` directive #44315

JoostK commented Nov 30, 2021

JoostK commented Nov 30, 2021 •

edited

AndrewKushnir left a comment

AndrewKushnir Dec 1, 2021

JoostK Dec 1, 2021

JoostK commented Dec 1, 2021

AndrewKushnir left a comment

AndrewKushnir commented Dec 1, 2021

AndrewKushnir left a comment

AndrewKushnir commented Dec 1, 2021

jessicajaniuk left a comment

AndrewKushnir commented Dec 2, 2021

AndrewKushnir commented Dec 2, 2021

angular-automatic-lock-bot bot commented Jan 2, 2022

perf(common): code size reduction of ngFor directive #44315

perf(common): code size reduction of ngFor directive #44315

Conversation

JoostK commented Nov 30, 2021

JoostK commented Nov 30, 2021 • edited

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Dec 1, 2021

Choose a reason for hiding this comment

JoostK Dec 1, 2021

Choose a reason for hiding this comment

JoostK commented Dec 1, 2021

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Dec 1, 2021

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Dec 1, 2021

jessicajaniuk left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Dec 2, 2021

AndrewKushnir commented Dec 2, 2021

angular-automatic-lock-bot bot commented Jan 2, 2022

perf(common): code size reduction of `ngFor` directive #44315

perf(common): code size reduction of `ngFor` directive #44315

JoostK commented Nov 30, 2021 •

edited