refactor(core): explore approach to avoid storing LView in __ngContext__ #41293
Conversation
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
crisbeto
force-pushed
the
ng-context-rework
branch
from
941bfae
to
d44a212
Compare
crisbeto
commented
Mar 21, 2021
| @@ -108,6 +108,33 @@ | |||
| "packages/compiler/src/render3/view/styling_builder.ts", | |||
| "packages/compiler/src/render3/view/template.ts" | |||
| ], | |||
| [ | |||
There was a problem hiding this comment.
Please ignore these circular dependencies. If we decide to go for this approach, I'll make sure that no new circular dependencies are introduced.
| @@ -123,6 +123,8 @@ function renderChildComponents(hostLView: LView, components: number[]): void { | |||
| } | |||
| } | |||
|
|
|||
| const ACTIVE_LVIEWS: (LView|null)[] = []; | |||
There was a problem hiding this comment.
This part is potentially leaky, but IMO it'll be fine, because this is the only place where LViews are created and destroyed and we have full control over it.
mhevery
reviewed
Mar 22, 2021
There was a problem hiding this comment.
This Looks promising, I think we should explore further. My thoughts:
- With this we are essentially getting into the business of memory management. In practice this is a ref counting scheme. The concern with ref-counting schemes is that one may forget to release memory.
- I think the key observation here is that when
LViewgets destroyed we already have to keep track of it because we need to callngDestroylife-cycle events. This means that once destroyed there is no way to resurrect the data. Piggybacking to clean upACTIVE_LVIEWSwould be a minor cost. - There is still a concern that someone may detach
ViewRefand than just drop it without properngDestroy. This would create a memory leak, but a good argument can be made that it is app error since theViewRefcould be re-added and dropping it creates an unreachable references.
NOTE:
- Did not review the code in depth only looked at the main algorithm for tracking ids
- Will need a way to deal with holes in the
ACTIVE_LVIEWSand also to ensure that we are not creating holey arrays.
crisbeto
force-pushed
the
ng-context-rework
branch
from
d44a212
to
48ec112
Compare
**Note:** this is mostly an exploratory PR to check whether the approach actually works and to start a discussion. There's more work to be done to make it mergeable (e.g. it introduces new circular dependencies, there are no unit tests etc.). Explores adding a unique ID to each `LView` which is stored in `__ngContext__`, rather than the LView itself. This will avoid worsening memory leaks where a detached element is retained along with its `LView`. Also reworks `LContext` to store the ID of its `LView`, rather than the `LView` itself, because the `LContext` can also be stored in `__ngContext__`.
crisbeto
force-pushed
the
ng-context-rework
branch
from
48ec112
to
9cc02cf
Compare
|
Closing in favor of #41358. |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Note: this is mostly an exploratory PR to check whether the approach actually works and to start a discussion. There's more work to be done to make it mergeable (e.g. it introduces new circular dependencies, there are no unit tests etc.).
Explores adding a unique ID to each
LViewwhich is stored in__ngContext__, rather than the LView itself. This will avoid worsening memory leaks where a detached element is retained along with itsLView.Furthermore, it reworks
LContextto store the ID of itsLView, rather than theLViewitself, because theLContextcan also be stored in__ngContext__.