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
perf(core): avoid storing LView in __ngContext__ #41908
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/** | ||
* @license | ||
* Copyright Google LLC All Rights Reserved. | ||
* | ||
* Use of this source code is governed by an MIT-style license that can be | ||
* found in the LICENSE file at https://angular.io/license | ||
*/ | ||
|
||
import {assertNumber} from '../../util/assert'; | ||
import {ID, LView} from './view'; | ||
|
||
// Keeps track of the currently-active LViews. | ||
const TRACKED_LVIEWS = new Map<number, LView>(); | ||
|
||
// Used for generating unique IDs for LViews. | ||
let uniqueIdCounter = 0; | ||
|
||
/** Starts tracking an LView and returns a unique ID that can be used for future lookups. */ | ||
export function registerLView(lView: LView): number { | ||
const id = uniqueIdCounter++; | ||
TRACKED_LVIEWS.set(id, lView); | ||
return id; | ||
} | ||
|
||
/** Gets an LView by its unique ID. */ | ||
export function getLViewById(id: number): LView|null { | ||
ngDevMode && assertNumber(id, 'ID used for LView lookup must be a number'); | ||
return TRACKED_LVIEWS.get(id) || null; | ||
} | ||
|
||
/** Stops tracking an LView. */ | ||
export function unregisterLView(lView: LView): void { | ||
ngDevMode && assertNumber(lView[ID], 'Cannot stop tracking an LView that does not have an ID'); | ||
TRACKED_LVIEWS.delete(lView[ID]); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 atrc.1
version):and let us know if the memory leak that you've observed is resolved.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sod thanks for checking! 👍