fix(reactivity): fix dep memory leak #7827
Conversation
| export type Dep = Set<ReactiveEffect> & | ||
| TrackedMarkers & { | ||
| target?: unknown | ||
| key?: unknown | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
I think you can keep the signature of triggerEffects, then create the Set inside the triggerEffects if needed.
skirtles-code
force-pushed
the
dep-memory-leak
branch
from
d2a84eb
to
c3d54d6
Compare
yyx990803
added
the
❗ p4-important
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
yyx990803
force-pushed
the
dep-memory-leak
branch
from
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
computedproperty. But the other instances, with distance 10, shouldn't be there. They are no longer in use and should have been garbage collected.The
targetMapshown 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 thesetobject 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, theWeakMapwon't save us.The value inside the
WeakMapis aMap. The key for thisMapis the property being tracked. The corresponding value is aSetof effects that are tracking that property, referred to asDepin the types.When a dependency is removed from an effect, it is removed from that
Setof effects. However, if thatSetis 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
Setor aMap. In that case, the 'property' being tracked can also be an object. So while the emptySetof 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 smallVClassobject, 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
SetorMap. 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