perf(core): avoid storing LView in __ngContext__ #41908
Conversation
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.
crisbeto
added
action: review
AndrewKushnir
added
the
action: presubmit
| import {ID, LView} from './view'; | ||
|
|
||
| // Keeps track of the currently-active LViews. | ||
| const TRACKED_LVIEWS = new Map<number, LView>(); |
There was a problem hiding this comment.
Is there a test for memory leaks in ssr context? We ran rc.1 which included #41358 and this Map filled up rather quickly with DOM nodes, crashing the server through out of memory in a matter of seconds.
That this is stil a single Map for multiple apps rendering on one global scope makes me rather nervous.
There was a problem hiding this comment.
We ended up reverting the original commit due to the memory leak and the fact that fixing the leak revealed some other issues. The reverted code will be in rc.2.
These changes include the original fix plus a fix for the memory leak so that we can debug the issues.
There was a problem hiding this comment.
Hi @sod, thanks for testing an RC version and sharing the feedback 👍
As Kristiyan mentioned, the previous version of this change was reverted and this PR has the necessary fixes to make sure the memory leak doesn't happen. If you get a chance, it's be great if you could try to use the following package (based on the changes in this PR) in your package.json file (while keeping other packages at rc.1 version):
...
"@angular/core": "https://978551-24195339-gh.circle-artifacts.com/0/angular/core-pr41908-dc11acb763.tgz",
...
and let us know if the memory leak that you've observed is resolved.
Thank you.
There was a problem hiding this comment.
Yeah, with https://978551-24195339-gh.circle-artifacts.com/0/angular/core-pr41908-dc11acb763.tgz memory consumption is stable. After 1000 requests the memory footprint is the same. And comparing memory heap dumps, I see no angular or dominos specific retention.
There was a problem hiding this comment.
@sod thanks for checking! 👍
AndrewKushnir
added
area: core
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
These changes combine #41358 and #41894.
Currently we save a reference to an
LViewon most DOM nodes created by Angular either by saving theLViewdirectly in the__ngContext__or by saving theLContextwhich has a reference to theLView. This can be a problem if the DOM node is retained in memory, because theLViewhas 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
LViewis 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, becauseLViews are an internal data structure and we have complete control over when they are created and destroyed.Fixes #41047.