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.

Sign up for GitHub

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

Jump to bottom

fix(core): use WeakRef to prevent object retention in WeakMap #55476

Closed
wants to merge 2 commits into from
Closed

fix(core): use WeakRef to prevent object retention in WeakMap #55476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27ad5f5
fix(core): use `WeakRef` to prevent object retention in `WeakMap`
alan-agius4 Apr 23, 2024
3aa0b95
refactor(core): set up framework injector profile exclusively when `W…
alan-agius4 Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 20 additions & 20 deletions packages/core/src/render3/debug/framework_injector_profiler.ts
View file
Open in desktop
Expand Up @@ -6,11 +6,12 @@
* found in the LICENSE file at https://angular.io/license
*/

import {InjectionToken} from '../../di';
import {Injector} from '../../di/injector';
import {EnvironmentInjector} from '../../di/r3_injector';
import {Type} from '../../interface/type';
import {assertDefined, throwError} from '../../util/assert';
import {assertTNode, assertTNodeForLView} from '../assert';
import {assertTNodeForLView} from '../assert';
import {getComponentDef} from '../definition';
import {getNodeInjectorLView, getNodeInjectorTNode, NodeInjector} from '../di';
import {TNode} from '../interfaces/node';
Expand Down Expand Up @@ -54,16 +55,15 @@ import {InjectedService, InjectorCreatedInstance, InjectorProfilerContext, Injec
*
*/
class DIDebugData {
resolverToTokenToDependencies =
new WeakMap<Injector|LView, WeakMap<Type<unknown>, InjectedService[]>>();
resolverToProviders = new WeakMap<Injector|TNode, ProviderRecord[]>();
resolverToTokenToDependencies = new WeakMap<
Injector|LView, WeakMap<Type<unknown>|InjectionToken<unknown>, WeakRef<InjectedService[]>>>();
resolverToProviders = new WeakMap<Injector|TNode, WeakRef<ProviderRecord[]>>();
Comment on lines +58 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this work, given that the WeakRef holds an array that is uniquely referenced in the WeakMap? Wouldn't each individual array item need to be a WeakRef—or alternatively replace the array with a WeakSet (that probably won't work as I assume iteration is necessary)—to avoid collecting the array entirely since the WeakMap is the sole owner of the array otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a WeakRef for each individual item would also work, however it seems redundant.

From my understanding is that the leak was being because whilst the WeakMap doesn’t hold keys as strong reference, the value are, setting the containing array in a WeakRef seems to solve this, in fact in the profiler the leak was no longer observed.

Are you thinking that the value WeakRef might be collected before the WeakMap key?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning is that the array that is stored in the WeakRef is the only place it's stored, so the only retainer is the WeakRef itself which is a weak retainer... so I'd presume that the array is always eligible to be GC'd. That's my mental model though, my understanding might certainly be inaccurate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I though at first too but it does not appear to be the case.

const wm = new WeakMap();
const key = { a: 1 };
wm.set(key, new WeakRef([2]));

globalThis.gc();

console.log(wm.get(key)?.deref()); // [ 2 ]

standaloneInjectorToComponent = new WeakMap<Injector, Type<unknown>>();

reset() {
this.resolverToTokenToDependencies =
new WeakMap<Injector|LView, WeakMap<Type<unknown>, InjectedService[]>>();
this.resolverToProviders = new WeakMap<Injector|TNode, ProviderRecord[]>();
this.standaloneInjectorToComponent = new WeakMap<Injector, Type<unknown>>();
this.resolverToTokenToDependencies = new WeakMap();
this.resolverToProviders = new WeakMap();
this.standaloneInjectorToComponent = new WeakMap();
}
}

Expand Down Expand Up @@ -121,7 +121,7 @@ function handleInjectEvent(context: InjectorProfilerContext, data: InjectedServi
const diResolverToInstantiatedToken = frameworkDIDebugData.resolverToTokenToDependencies;

if (!diResolverToInstantiatedToken.has(diResolver)) {
diResolverToInstantiatedToken.set(diResolver, new WeakMap<Type<unknown>, InjectedService[]>());
diResolverToInstantiatedToken.set(diResolver, new WeakMap());
}

// if token is a primitive type, ignore this event. We do this because we cannot keep track of
Expand All @@ -131,17 +131,15 @@ function handleInjectEvent(context: InjectorProfilerContext, data: InjectedServi
}

const instantiatedTokenToDependencies = diResolverToInstantiatedToken.get(diResolver)!;
if (!instantiatedTokenToDependencies.has(context.token!)) {
instantiatedTokenToDependencies.set(context.token!, []);
}

const {token, value, flags} = data;

assertDefined(context.token, 'Injector profiler context token is undefined.');

const dependencies = instantiatedTokenToDependencies.get(context.token);
assertDefined(dependencies, 'Could not resolve dependencies for token.');
let dependencies = instantiatedTokenToDependencies.get(context.token)?.deref();
if (!dependencies) {
dependencies = [];
instantiatedTokenToDependencies.set(context.token, new WeakRef(dependencies));
}

const {token, value, flags} = data;
if (context.injector instanceof NodeInjector) {
dependencies.push({token, value, flags, injectedIn: getNodeInjectorContext(context.injector)});
} else {
Expand Down Expand Up @@ -251,11 +249,13 @@ function handleProviderConfiguredEvent(
throwError('A ProviderConfigured event must be run within an injection context.');
}

if (!resolverToProviders.has(diResolver)) {
resolverToProviders.set(diResolver, []);
let resolverData = resolverToProviders.get(diResolver)?.deref();
if (!resolverData) {
resolverData = [];
resolverToProviders.set(diResolver, new WeakRef(resolverData));
}

resolverToProviders.get(diResolver)!.push(data);
resolverData.push(data);
}

function getDIResolver(injector: Injector|undefined): Injector|LView|null {
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/render3/util/global_utils.ts
View file
Open in desktop
Expand Up @@ -71,8 +71,9 @@ export function publishDefaultGlobalUtils() {
if (!_published) {
_published = true;

if (typeof window !== 'undefined') {
// Only configure the injector profiler when running in the browser.
// Only configure the injector profiler when running in the browser and WeakRef is defined.
// In some G3 tests WeakRef is undefined as they user older (unsupported) browsers to test.
if (typeof window !== 'undefined' && typeof WeakRef !== 'undefined') {
setupFrameworkInjectorProfiler();
}

Expand Down
9 changes: 5 additions & 4 deletions packages/core/src/render3/util/injector_discovery_utils.ts
View file
Open in desktop
Expand Up @@ -132,12 +132,12 @@ function getDependenciesForTokenInInjector<T>(
const {resolverToTokenToDependencies} = getFrameworkDIDebugData();

if (!(injector instanceof NodeInjector)) {
return resolverToTokenToDependencies.get(injector)?.get?.(token as Type<T>) ?? [];
return resolverToTokenToDependencies.get(injector)?.get?.(token)?.deref() ?? [];
}

const lView = getNodeInjectorLView(injector);
const tokenDependencyMap = resolverToTokenToDependencies.get(lView);
const dependencies = tokenDependencyMap?.get(token as Type<T>) ?? [];
const dependencies = tokenDependencyMap?.get(token)?.deref() ?? [];

// In the NodeInjector case, all injections for every node are stored in the same lView.
// We use the injectedIn field of the dependency to filter out the dependencies that
Expand Down Expand Up @@ -207,7 +207,8 @@ function getProviderImportsContainer(injector: Injector): Type<unknown>|null {
function getNodeInjectorProviders(injector: NodeInjector): ProviderRecord[] {
const diResolver = getNodeInjectorTNode(injector);
const {resolverToProviders} = getFrameworkDIDebugData();
return resolverToProviders.get(diResolver as TNode) ?? [];

return resolverToProviders.get(diResolver as TNode)?.deref() ?? [];
}

/**
Expand Down Expand Up @@ -395,7 +396,7 @@ function walkProviderTreeToDiscoverImportPaths(
*/
function getEnvironmentInjectorProviders(injector: EnvironmentInjector): ProviderRecord[] {
const providerRecordsWithoutImportPaths =
getFrameworkDIDebugData().resolverToProviders.get(injector) ?? [];
getFrameworkDIDebugData().resolverToProviders.get(injector)?.deref() ?? [];

// platform injector has no provider imports container so can we skip trying to
// find import paths
Expand Down
2 changes: 1 addition & 1 deletion packages/tsconfig-build.json
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@
"target": "es2022",
// Keep the below in sync with ng_module.bzl
"useDefineForClassFields": false,
"lib": ["es2020", "dom", "dom.iterable"],
"lib": ["es2020", "dom", "dom.iterable", "ES2021.WeakRef"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that you run into a #54051 situation :)

https://caniuse.com/mdn-javascript_builtins_array_at
https://caniuse.com/mdn-javascript_builtins_weakref

Of course it's totally in the realm of https://angular.dev/tools/cli/build#configuring-browser-compatibility; but a mention in the changelog or migration guide would be very appreciated for folks that like to include a polyfill for a while to not have that harsh of a cutoff (and not be surprised by error reporting).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only used if the browser/environment supports it. You can read the rest of the PR, but basically this:

    // Only configure the injector profiler when running in the browser and WeakRef is defined.
    // In some G3 tests WeakRef is undefined as they user older (unsupported) browsers to test.
    if (typeof window !== 'undefined' && typeof WeakRef !== 'undefined') {

"skipLibCheck": true,
// don't auto-discover @types/node, it results in a ///<reference in the .d.ts output
"types": [],
Expand Down
2 changes: 1 addition & 1 deletion packages/tsconfig-tsec-base.json
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@
"extends": "./tsconfig-build.json",
"compilerOptions": {
"noEmit": true,
"lib": ["es2020", "dom", "dom.iterable"],
"lib": ["es2020", "dom", "dom.iterable", "ES2021.WeakRef"],
"plugins": [{"name": "tsec", "exemptionConfig": "./tsec-exemption.json"}]
}
}
2 changes: 1 addition & 1 deletion packages/tsconfig.json
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@
},
"rootDir": ".",
"inlineSourceMap": true,
"lib": ["es2020", "dom", "dom.iterable"],
"lib": ["es2020", "dom", "dom.iterable", "ES2021.WeakRef"],
"skipDefaultLibCheck": true,
"skipLibCheck": true,
"types": ["angular"]
Expand Down