perf(core): avoid storing LView in ngContext #41358

crisbeto · 2021-03-28T09:40:38Z

Currently we save a reference to an LView on most DOM nodes created by Angular either by saving the LView directly in the __ngContext__ or by saving the LContext which has a reference to the LView. This can be a problem if the DOM node is retained in memory, because the LView has references to all of the child nodes of the view, as well as other internal data structures.

Previously we tried to resolve the issue by clearing the __ngContext__ when a node is removed (see #36011), but we decided not to proceeed, because it can slow down destruction due to a megamorphic write.

These changes aim to address the issue while reducing the performance impact by assigning a unique ID when an LView is created and adding it to __ngContext__. All active views are tracked in a map where their unique ID is used as the key. We don't need to worry about leaks within that map, because LViews are an internal data structure and we have complete control over when they are created and destroyed.

Fixes #41047.

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

packages/core/test/bundling/forms_reactive/index.ts

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

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

packages/core/src/render3/context_discovery.ts

packages/core/test/acceptance/i18n_spec.ts

packages/core/test/bundling/forms_reactive/index.ts

crisbeto · 2021-03-29T19:06:31Z

All of the feedback should be addressed now.

AndrewKushnir

Thanks for the changes @crisbeto 👍 Just left a few comments.

One more nit: should we change the fix(core) -> refactor(core) or perf(core)? It looks like this is more of an improvement vs a bug fix.

packages/core/src/debug/debug_node.ts

packages/core/src/render3/context_discovery.ts

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

packages/core/src/render3/util/discovery_utils.ts

packages/core/test/acceptance/debug_spec.ts

mgechev

The change will break DevTools. Exporting getLViewById will be helpful to let our metadata collection mechanism fallback to the new approach.

AndrewKushnir

LGTM, thanks for addressing the feedback @crisbeto 👍

FYI, I'm adding the "blocked" label for now to make sure we unblock Minko (based on his comment) before merging.

packages/core/src/debug/debug_node.ts

packages/core/src/render3/context_discovery.ts

crisbeto · 2021-04-22T19:51:32Z

I've pushed another change to account for the recent switch from loadLContext to getLContext.

AndrewKushnir

@crisbeto thanks for applying fixes and rebasing the PR! 👍
I've left a couple comments (mostly around the areas that might potentially have null where we extract lView from the context).

packages/core/src/render3/util/discovery_utils.ts

crisbeto

I've addressed the latest set of feedback.

packages/core/src/render3/util/discovery_utils.ts

AndrewKushnir · 2021-04-23T18:12:15Z

Presubmit.

AndrewKushnir · 2021-04-23T22:36:06Z

FYI presubmits went well, I'll run a global one during the weekend (to make sure there are no corner-cases that we've not reviewed/discussed) and I will share results on Monday.

AndrewKushnir · 2021-04-26T04:11:15Z

Global Presubmit (Ivy version).

AndrewKushnir · 2021-04-26T16:16:24Z

FYI, global presubmit went well too, removing the "presubmit" label.

Currently we save a reference to an `LView` on most DOM nodes created by Angular either by saving the `LView` directly in the `__ngContext__` or by saving the `LContext` which has a reference to the `LView`. This can be a problem if the DOM node is retained in memory, because the `LView` has references to all of the child nodes of the view, as well as other internal data structures. Previously we tried to resolve the issue by clearing the `__ngContext__` when a node is removed (see #36011), but we decided not to proceeed, because it can slow down destruction due to a megamorphic write. These changes aim to address the issue while reducing the performance impact by assigning a unique ID when an `LView` is created and adding it to `__ngContext__`. All active views are tracked in a map where their unique ID is used as the key. We don't need to worry about leaks within that map, because `LView`s are an internal data structure and we have complete control over when they are created and destroyed. Fixes #41047. PR Close #41358

This is a follow-up to angular#41358 where my assumption was that `destroyLView` would be called for all views down the tree, but it turns out that it only gets called for the root and we actually call `cleanUpView` for the descendants. The result is that some views might not be removed from the registry after they're destroyed. These changes move the `unregisterLView` call into `cleanUpView` instead.

…)" This reverts commit 18b33e7.

angular/angular#41358

…41901) This reverts commit 18b33e7. PR Close #41901

These changes combine angular#41358 and angular#41894. Currently we save a reference to an `LView` on most DOM nodes created by Angular either by saving the `LView` directly in the `__ngContext__` or by saving the `LContext` which has a reference to the `LView`. This can be a problem if the DOM node is retained in memory, because the `LView` has references to all of the child nodes of the view, as well as other internal data structures. Previously we tried to resolve the issue by clearing the `__ngContext__` when a node is removed (see angular#36011), but we decided not to proceeed, because it can slow down destruction due to a megamorphic write. These changes aim to address the issue while reducing the performance impact by assigning a unique ID when an `LView` is created and adding it to `__ngContext__`. All active views are tracked in a map where their unique ID is used as the key. We don't need to worry about leaks within that map, because `LView`s are an internal data structure and we have complete control over when they are created and destroyed. Fixes angular#41047.

angular-automatic-lock-bot · 2021-05-27T16:21:10Z

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

google-cla bot added the cla: yes label Mar 28, 2021

crisbeto mentioned this pull request Mar 28, 2021

refactor(core): explore approach to avoid storing LView in __ngContext__ #41293

Closed

crisbeto force-pushed the ng-context branch 2 times, most recently from 2d315ad to 32733ed Compare March 28, 2021 10:42

crisbeto commented Mar 28, 2021

View reviewed changes

packages/core/src/render3/instructions/lview_tracking.ts Outdated Show resolved Hide resolved

packages/core/src/render3/instructions/lview_tracking.ts Outdated Show resolved Hide resolved

packages/core/test/bundling/forms_reactive/index.ts Show resolved Hide resolved

crisbeto requested a review from mhevery March 28, 2021 11:16

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Mar 28, 2021

ngbot bot modified the milestone: Backlog Mar 28, 2021

crisbeto marked this pull request as ready for review March 28, 2021 11:16

crisbeto force-pushed the ng-context branch 2 times, most recently from 7038e66 to 59a47d5 Compare March 29, 2021 18:21

mhevery suggested changes Mar 29, 2021

View reviewed changes

crisbeto force-pushed the ng-context branch 2 times, most recently from 816eeef to c2c32e2 Compare March 29, 2021 18:47

AndrewKushnir reviewed Mar 29, 2021

View reviewed changes

mgechev reviewed Mar 29, 2021

View reviewed changes

packages/core/test/acceptance/debug_spec.ts Outdated Show resolved Hide resolved

mgechev requested changes Mar 29, 2021

View reviewed changes

crisbeto force-pushed the ng-context branch from c2c32e2 to 1db8959 Compare March 30, 2021 09:27

crisbeto changed the title ~~fix(core): avoid storing LView in __ngContext__~~ perf(core): avoid storing LView in __ngContext__ Mar 30, 2021

crisbeto force-pushed the ng-context branch 5 times, most recently from 83bf66c to 186f79d Compare March 30, 2021 11:05

AndrewKushnir approved these changes Mar 30, 2021

View reviewed changes

packages/core/src/debug/debug_node.ts Outdated Show resolved Hide resolved

packages/core/src/debug/debug_node.ts Outdated Show resolved Hide resolved

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

crisbeto force-pushed the ng-context branch 3 times, most recently from c8d56c2 to 68478a4 Compare April 22, 2021 19:22

fixup! perf(core): avoid storing LView in __ngContext__

39abf88

crisbeto force-pushed the ng-context branch from 68478a4 to 39abf88 Compare April 22, 2021 20:34

AndrewKushnir reviewed Apr 22, 2021

View reviewed changes

fixup! perf(core): avoid storing LView in __ngContext__

d906995

crisbeto commented Apr 23, 2021

View reviewed changes

packages/core/src/render3/util/discovery_utils.ts Outdated Show resolved Hide resolved

packages/core/src/render3/util/discovery_utils.ts Outdated Show resolved Hide resolved

packages/core/src/render3/util/discovery_utils.ts Outdated Show resolved Hide resolved

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Apr 26, 2021

crisbeto added the action: merge The PR is ready for merge by the caretaker label Apr 26, 2021

jessicajaniuk closed this in 18b33e7 Apr 26, 2021

JoostK mentioned this pull request Apr 26, 2021

Ivy Memory Leak: Select & Option Elements #37671

Closed

crisbeto mentioned this pull request Apr 29, 2021

test(core): make tests more resilient to header offset changes #41883

Closed

crisbeto mentioned this pull request Apr 30, 2021

fix(core): remove nested views from registry when root is destroyed #41894

Closed

crisbeto added a commit to crisbeto/angular that referenced this pull request Apr 30, 2021

Revert "perf(core): avoid storing LView in __ngContext__ (angular#41358…

23223da

…)" This reverts commit 18b33e7.

crisbeto added a commit to crisbeto/angular that referenced this pull request Apr 30, 2021

Revert "perf(core): avoid storing LView in __ngContext__ (angular#41358…

efb5d99

…)" This reverts commit 18b33e7.

satanTime added a commit to help-me-mom/ng-mocks that referenced this pull request Apr 30, 2021

fix: supporting new structure of lView

8d3cadf

angular/angular#41358

mhevery pushed a commit that referenced this pull request Apr 30, 2021

Revert "perf(core): avoid storing LView in __ngContext__ (#41358)" (#…

2dd96e0

…41901) This reverts commit 18b33e7. PR Close #41901

mhevery pushed a commit that referenced this pull request Apr 30, 2021

Revert "perf(core): avoid storing LView in __ngContext__ (#41358)" (#…

1c0db8c

…41901) This reverts commit 18b33e7. PR Close #41901

crisbeto mentioned this pull request Apr 30, 2021

perf(core): avoid storing LView in __ngContext__ #41908

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(core): avoid storing LView in ngContext #41358

perf(core): avoid storing LView in ngContext #41358

crisbeto commented Mar 28, 2021 •

edited

crisbeto commented Mar 29, 2021 •

edited

AndrewKushnir left a comment

mgechev left a comment

AndrewKushnir left a comment

crisbeto commented Apr 22, 2021

AndrewKushnir left a comment

crisbeto left a comment

AndrewKushnir commented Apr 23, 2021

AndrewKushnir commented Apr 23, 2021

AndrewKushnir commented Apr 26, 2021

AndrewKushnir commented Apr 26, 2021

angular-automatic-lock-bot bot commented May 27, 2021

perf(core): avoid storing LView in __ngContext__ #41358

perf(core): avoid storing LView in __ngContext__ #41358

Conversation

crisbeto commented Mar 28, 2021 • edited

crisbeto commented Mar 29, 2021 • edited

AndrewKushnir left a comment

Choose a reason for hiding this comment

mgechev left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

crisbeto commented Apr 22, 2021

AndrewKushnir left a comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Apr 23, 2021

AndrewKushnir commented Apr 23, 2021

AndrewKushnir commented Apr 26, 2021

AndrewKushnir commented Apr 26, 2021

angular-automatic-lock-bot bot commented May 27, 2021

perf(core): avoid storing LView in ngContext #41358

perf(core): avoid storing LView in ngContext #41358

crisbeto commented Mar 28, 2021 •

edited

crisbeto commented Mar 29, 2021 •

edited