Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ivy doesn’t deallocate memory of destroyed components correctly #35148

Closed
christianliebel opened this issue Feb 4, 2020 · 6 comments
Closed
Assignees
Labels
area: core Issues related to the framework runtime state: has PR type: bug/fix

Comments

@christianliebel
Copy link
Contributor

🐞 bug report

Affected Package

The issue is caused by package @angular/core@9.0.0-rc.14, earlier versions probably as well.

Is this a regression?

This issue does not occur in Angular 8 + View Engine.

Description

Ivy does not seem to correctly deallocate memory of destroyed components in all cases. We’ve noticed an issue that when navigating away from a route, at least some of the loaded components are getting destroyed but not deallocated and garbage collected. (The issue might not be restricted to routing.)

The problem is that unnecessary memory is not deallocated, which might lead to problems on mobile and other low-memory devices and might also have security implications (e.g. when sensitive state remains in memory, even after a user has logged out).

🔬 Minimal Reproduction

Here is a minimal repo of the problem. It’s a plain Angular CLI application with three routes and four components:

  • /#/route1 (Route1Component) includes the ContentComponent as a child that loads the contents of a large file retrieved via the network into a field of its class during ngOnInit, consuming some memory. This component logs to the console when it’s being initialized or destroyed.
  • /#/route2 (Route2Component) and route3 (Route3Component) don’t include this component.

Repro steps:

git clone https://github.com/thinktecture-labs/ivy-mem-leak.git
cd ivy-mem-leak
npm i
npm start

Stop ng serve and compare this with the behavior of Angular 8:

git checkout ng8
npm i
npm start

🔥 Exception or Error

The memory of route1’s components doesn’t seem to be correctly deallocated when leaving route1 for route2 on Angular 9/Ivy:

Angular 9/Ivy Angular 8/View Engine
image image

As per DevTools, (at least) the component instance of the ContentComponent seems to be retained by a contextLView array even after it has been destroyed:

image

Is this behavior intended or is this a bug?

Researched by @ManuelRauber, @thomashilzendegen and me.

🌍 Your Environment

Angular Version:


     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/


Angular CLI: 9.0.0-rc.12
Node: 12.14.1
OS: darwin x64

Angular: 9.0.0-rc.14
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-rc.12
@angular-devkit/build-angular     0.900.0-rc.12
@angular-devkit/build-optimizer   0.900.0-rc.12
@angular-devkit/build-webpack     0.900.0-rc.12
@angular-devkit/core              9.0.0-rc.12
@angular-devkit/schematics        9.0.0-rc.12
@angular/cli                      9.0.0-rc.12
@ngtools/webpack                  9.0.0-rc.12
@schematics/angular               9.0.0-rc.12
@schematics/update                0.900.0-rc.12
rxjs                              6.5.4
typescript                        3.7.5
webpack                           4.41.2

Anything else relevant?

@ngbot ngbot bot added this to the needsTriage milestone Feb 4, 2020
@AndrewKushnir AndrewKushnir added the area: core Issues related to the framework runtime label Feb 4, 2020
@mhevery
Copy link
Contributor

mhevery commented Feb 5, 2020

Kudos for such a well documented repro steps. I wish more people would provide examples such as these.

It looks to me that master is also on Ivy and when I try to reproduce the working example it also retains on master could you please fix that?

@mhevery
Copy link
Contributor

mhevery commented Feb 5, 2020

OK, I was able to find a root cause and will work on a fix.

Root cause is that for perf reasons we cache LFrame so that we don't have to allocate it all the time. To be extra fast we clear the LFrame on enter rather that on leave. However it does mean that the deepest LFrame will retain objects until the depth matches again.

The fix is to simply clear the LFrame on leave rather then on enter

@thomashilzendegen
Copy link

@mhevery Thanks for your support. I have to add something:

We used @angular-devkit/build-angular version 0.900.0-rc.12 (as seen in the CLI version output). When I switched to the recent version (0.900.0-rc.14) I got a slightly better result. When the depth matches between routes the cleanup works as intended, but when I navigate to the root route (e.g. by removing route1) the allocated components stay in memory.

mhevery added a commit to mhevery/angular that referenced this issue Feb 5, 2020
Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object.

The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()`

Fix angular#35148
@christianliebel
Copy link
Contributor Author

It looks to me that master is also on Ivy and when I try to reproduce the working example it also retains on master could you please fix that?

@mhevery In the repro, the master branch is Angular 9/Ivy and the ng8 branch is Angular 8/View Engine. As you’ve provided a potential fix, I guess we don't have to update the repro repo?

@mhevery
Copy link
Contributor

mhevery commented Feb 6, 2020

@mhevery In the repro, the master branch is Angular 9/Ivy and the ng8 branch is Angular 8/View Engine. As you’ve provided a potential fix, I guess we don't have to update the repro repo?

no need to do anything.

mhevery added a commit to mhevery/angular that referenced this issue Feb 11, 2020
Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object.

The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()`

Fix angular#35148
mhevery added a commit to mhevery/angular that referenced this issue Feb 11, 2020
Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object.

The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()`

Fix angular#35148
mhevery added a commit to mhevery/angular that referenced this issue Feb 14, 2020
Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object.

The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()`

Fix angular#35148
@alxhub alxhub closed this as completed in b9b512f Feb 14, 2020
alxhub pushed a commit that referenced this issue Feb 14, 2020
Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object.

The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()`

Fix #35148

PR Close #35156
sonukapoor pushed a commit to sonukapoor/angular that referenced this issue Feb 17, 2020
…35156)

Root cause is that for perf reasons we cache `LFrame` so that we don't have to allocate it all the time. To be extra fast we clear the `LFrame` on `enterView()` rather that on `leaveView()`. The implication of this strategy is that the deepest `LFrame` will retain objects until the `LFrame` allocation depth matches the deepest object.

The fix is to simply clear the `LFrame` on `leaveView()` rather then on `enterView()`

Fix angular#35148

PR Close angular#35156
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime state: has PR type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants