refactor(core): minor ComponentDef improvements by crisbeto · Pull Request #46093 · angular/angular

crisbeto · 2022-05-22T08:43:08Z

Makes the following improvements in the runtime:

Uses the unique ID of the component definition to keep track of its injector in the StandaloneFeature, instead of the definition itself. This reduces the amount of memory we can leak, if something doesn't get cleaned up.
Changes the naming and description of the ComponentDef.id to reflect what it is used for.

Makes the following improvements in the runtime: * Uses the unique ID of the component definition to keep track of its injector in the `StandaloneFeature`, instead of the definition itself. This reduces the amount of memory we can leak, if something doesn't get cleaned up. * Changes the naming and description of the `ComponentDef.id` to reflect what it is used for.

AndrewKushnir · 2022-05-23T18:08:57Z

packages/core/src/render3/features/standalone_feature.ts


-    if (!this.cachedInjectors.has(componentDef)) {
+    if (!this.cachedInjectors.has(componentDef.id)) {
      const providers = internalImportProvidersFrom(false, componentDef.type);


This reduces the amount of memory we can leak, if something doesn't get cleaned up.

@crisbeto I think this line and the underlying logic in the internalImportProvidersFrom (this one specifically) would prevent the componentDef from being released from the memory, i.e. there will always be a reference to the componentDef.type (which points back to componentDef) when the StandaloneService is around. If my understanding is correct, this change has no effect on the memory management.

Isn't that one going to get destroyed together with the LView or am I misunderstanding?

Either way, my thinking with this change is that it's functionally the same and in the worst case it'll work exactly the same as it does now. In the best case we might get a small memory improvement 🤷

@crisbeto looking at the code again, I think this change makes sense 👍

The code that I referred to in my previous message (this one) is actually invoked for NgModules only. For the nested standalone components we don't store their defs in an injector, we just iterate over its dependencies, so there should be no extra references. The change that you've made should allow to avoid a reference from the cached injector map. So technically if there are no other references to a particular componentDef (once a component is destroyed) - it should be released from the memory. Currently we would retain the componentDef even if it's unused (when there are no active instances of that component).

Also adding @pkozlowski-opensource as a reviewer, since he is the most familiar with this code.

So, I don't see harm in making those changes but also not sure if it moves a needle. Observations:

standalone injectors are used in the dynamic instantiation scenarios;

we are going through the ComponentFactory to dynamically instantiate components;

ComponentFactory already holds onto component def / type;

ApplicationRef is using the above info to have a collection of bootstrapped types - actually I'm not sure why we are doing this...

The net effect of the above is that we already hold references to all the dynamically bootstrapped types.

What I like about this PR is that it clarifies the component's id purpose.
What I dislike is that it is a bit harder now to understand the standalone future.

As I've said, no strong opinion here - don't mind either way.

My understanding is that lazy-loaded components (via router) would benefit from this change. For example, if we move from route A, which uses ComponentA to route B, which uses ComponentB (both components are standalone) - the ComponentA class (and everything that it refers to and depends on) would be retained in memory, since there is a reference to it (because it's used as a key in a map) in the Standalone injector service. With this change, this reference is no longer present, thus allowing the memory to be released (unless there are some other refs to the class retained somewhere). If my understanding is correct, this PR fixes a memory leak that may happen in certain scenarios (lazy loading).

I'm not familiar with the router internals to confirm that this is a problem. More specifically I don't know how component reuse works. Anyhow, I see your point: having less references === more safety.

I don't mind either way. Another option we could consider is the usage of a WeekMap

Andrew raised this over DM: we can't use a WeakMap, because we have to be able to iterate over the keys in ngOnDestroy.

Was going to suggest a WeakMap as well but any iteration rules it out unfortunately. I think it would otherwise be ideal here though. What if we used a combination of WeakMap for the lookup and a Set (or array) for the injector cleanup. Has a side benefit of making it more clear that there is both caching and cleanup tracking happening in the class.

Oh right, WeakMap is not an option then.

As I've said, no objections on merging this PR, it does provide an additional safety net.

We could use a WeakMap and a separate Set to keep track of the injector instances themselves (for iteration). Either way is fine with me.

pkozlowski-opensource · 2022-05-24T08:14:56Z

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

 export interface ComponentDef<T> extends DirectiveDef<T> {
  /**
-   * Runtime unique component ID.
+   * Unique ID for the component. Used in view encapsulation and


"for the component" => "for the component type / definition" - or anything else that makes it clear that we are talking about component classes and not instances.

pkozlowski-opensource

LGTM

alxhub

Note that this PR should not merge within the current feature freeze period and should wait for 14.0.1.

alxhub · 2022-05-31T20:00:45Z

packages/core/src/render3/features/standalone_feature.ts


-    if (!this.cachedInjectors.has(componentDef)) {
+    if (!this.cachedInjectors.has(componentDef.id)) {
      const providers = internalImportProvidersFrom(false, componentDef.type);


We could use a WeakMap and a separate Set to keep track of the injector instances themselves (for iteration). Either way is fine with me.

crisbeto · 2022-06-02T09:03:43Z

Blocked until after the commit freeze.

alxhub · 2022-06-02T20:41:29Z

This PR was merged into the repository by commit 0206c10.

Makes the following improvements in the runtime: * Uses the unique ID of the component definition to keep track of its injector in the `StandaloneFeature`, instead of the definition itself. This reduces the amount of memory we can leak, if something doesn't get cleaned up. * Changes the naming and description of the `ComponentDef.id` to reflect what it is used for. PR Close #46093

angular-automatic-lock-bot · 2022-07-03T16:06:51Z

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

crisbeto marked this pull request as ready for review May 22, 2022 09:08

pullapprove bot requested a review from alxhub May 22, 2022 09:08

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: rc This PR is targeted for the next release-candidate labels May 22, 2022

ngbot bot added this to the Backlog milestone May 22, 2022

AndrewKushnir reviewed May 23, 2022

View reviewed changes

AndrewKushnir requested a review from pkozlowski-opensource May 23, 2022 21:44

pkozlowski-opensource reviewed May 24, 2022

View reviewed changes

AndrewKushnir approved these changes May 25, 2022

View reviewed changes

pkozlowski-opensource approved these changes May 26, 2022

View reviewed changes

alxhub approved these changes May 31, 2022

View reviewed changes

crisbeto removed the state: blocked label Jun 2, 2022

alxhub closed this in 0206c10 Jun 2, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 3, 2022

Conversation

crisbeto commented May 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clydin May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Jun 2, 2022

Uh oh!

alxhub commented Jun 2, 2022

Uh oh!

angular-automatic-lock-bot bot commented Jul 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AndrewKushnir May 24, 2022 •

edited

Loading

clydin May 25, 2022 •

edited

Loading