-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
fix(reactivity): fix dep memory leak #7827
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
Conversation
export type Dep = Set<ReactiveEffect> & | ||
TrackedMarkers & { | ||
target?: unknown | ||
key?: unknown | ||
} |
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.
I wasn't sure if there was a reason why the properties w
and n
use single-letter names. I've used target
and key
here, but they can easily be changed to t
and k
if required.
export function removeEffectFromDep(dep: Dep, effect: ReactiveEffect) { | ||
dep.delete(effect) | ||
if (dep.target && dep.size === 0) { | ||
const depsMap = targetMap.get(dep.target) | ||
if (depsMap) { | ||
depsMap.delete(dep.key) | ||
} | ||
dep.target = dep.key = null | ||
} | ||
} | ||
|
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.
I considered using a setTimeout()
here to defer the purging of the empty dependencies. Removing them immediately like this isn't strictly necessary, and could lead to some extra churn if empty dependencies are removed and then immediately re-added.
However, in the most common cases, the trackOpBit
stuff avoids this extra churn, so I decided it wasn't worth the extra complexity.
triggerEffects(new Set(effects), eventInfo) | ||
} else { | ||
triggerEffects(createDep(effects)) | ||
triggerEffects(new Set(effects)) | ||
} | ||
} | ||
} | ||
|
||
export function triggerEffects( | ||
dep: Dep | ReactiveEffect[], | ||
dep: Set<ReactiveEffect>, | ||
debuggerEventExtraInfo?: DebuggerEventExtraInfo | ||
) { | ||
// spread into array for stabilization | ||
const effects = isArray(dep) ? dep : [...dep] | ||
const effects = [...dep] |
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.
These changes were inspired by #6185.
The createDep(effects)
call isn't really creating a Dep
, it's just using a Set
to remove duplicates. This is the only place that passes an argument to createDep()
, and I wanted to change the signature of createDep()
to be able to pass the target
and key
instead. The other changes here follow from that.
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.
I think you can keep the signature of triggerEffects, then create the Set inside the triggerEffects if needed.
d2a84eb
to
c3d54d6
Compare
Co-Authored-By: skirtle <65301168+skirtles-code@users.noreply.github.com> vuejs#7827
Co-Authored-By: skirtle <65301168+skirtles-code@users.noreply.github.com> vuejs#7827
c3d54d6
to
c795a2e
Compare
Size ReportBundles
Usages
|
I believe this problem has been fixed by the overhaul of the reactivity system in 3.4. |
Here's a Playground where you can observe the leak:
Playground
Click the button a few times and then take a Heap Snapshot. Filtering on
VClass
, you should see something like this:The instance with distance 7 is supposed to be there. That is a current dependency of the
computed
property. But the other instances, with distance 10, shouldn't be there. They are no longer in use and should have been garbage collected.The
targetMap
shown in the Retainers is fromeffect.ts
. That's aWeakMap
. The keys in that map are reactive objects, so in my example it would be the thing I've calledset
. If theset
object is GCed then the leak will be cleaned up, but in cases where that object is retained long term, e.g. within a global store, theWeakMap
won't save us.The value inside the
WeakMap
is aMap
. The key for thisMap
is the property being tracked. The corresponding value is aSet
of effects that are tracking that property, referred to asDep
in the types.When a dependency is removed from an effect, it is removed from that
Set
of effects. However, if thatSet
is now empty, it isn't removed. That is a small leak, but arguably not worth the effort to tidy up.The potential for a much bigger leak occurs when the reactive object is a
Set
or aMap
. In that case, the 'property' being tracked can also be an object. So while the emptySet
of dependencies may only be a small leak, the leak of the key can be an arbitrary, and potentially large, object. In my example it is the relatively smallVClass
object, but in principle that object could be much bigger.To put it another way:
The leaking objects don't need to have ever been in the reactive
Set
orMap
. It's enough just to callhas()
, as that establishes a dependency on them, even if they are never in the collection.Here is the same example but using the code in this PR:
Playground - leak fixed