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
fix(core): use WeakRef
to prevent object retention in WeakMap
#55476
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": [], | ||
|
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.
How would this work, given that the
WeakRef
holds an array that is uniquely referenced in theWeakMap
? Wouldn't each individual array item need to be aWeakRef
—or alternatively replace the array with aWeakSet
(that probably won't work as I assume iteration is necessary)—to avoid collecting the array entirely since theWeakMap
is the sole owner of the array otherwise?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.
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 aWeakRef
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 theWeakMap
key?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.
My reasoning is that the array that is stored in the
WeakRef
is the only place it's stored, so the only retainer is theWeakRef
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.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.
That is what I though at first too but it does not appear to be the case.