Computed: memory leak

[basvanmeurs]

Version

3.0.0

Reproduction link

https://pastebin.pl/view/raw/ac6ade7c

Steps to reproduce

1. Click on clear button
2. In chrome, take a memory heap snapshot.

What is expected?

MyClass instances are not in the snapshot.

What is actually happening?

MyClass instances are in the snapshot.

---

The instances are retained by the 'computedField' computeds, which are retained in someRefThatExistsLongerThanA's depMaps:
https://github.com/vuejs/vue-next/blob/d52d139b854238708195f5d332b7d0ec3a4883a5/packages/reactivity/src/effect.ts#L143

This is because dependencies from someRefThatExistsLongerThanA to computedFields are never cleared, except immediately before re-evaluating them. Computed does not have an exported way to 'cleanup' them, like watch and watchEffect have (WatchStopHandle).

We ran into this when we found a memory leak in one of our applications.

Solutions:

1. ComputedRef.cleanup
   ComputedRef could offer a cleanup method which simply runs the effects' cleanup (https://github.com/vuejs/vue-next/blob/d52d139b854238708195f5d332b7d0ec3a4883a5/packages/reactivity/src/effect.ts#L111), as well as triggering itself. The latter is necessary to recursively trigger computed dependees, so that they become dirty and don't return stale results when called later - if ever.
   Downsides are that this should be done manually by the developer, and that calling computedRef.cleanup will also invoke watchers if they are still active - so they should be unregistered before cleaning up the computeds that they depend on. In practice it will be quite cumbersome, and another chore for developers.

2. Alternatively, we could clear the gathered Sets as part of add (https://github.com/vuejs/vue-next/blob/d52d139b854238708195f5d332b7d0ec3a4883a5/packages/reactivity/src/effect.ts#L180). There's no point in maintaining them anyway from this point, as all dependees are triggered and will re-evaluate.
   The benefit is that if all dependencies have been updated for some computed, that it will be totally dereferenced. The developer doesn't have to do anything except for - possibly - being careful with refs that are never or rarely updated. When such a case is encountered, triggerRef could be used to make sure that dependees are dereferenced once in a while. In practice this is not very likely to happen though.

There is another positive side effect, in that dependees that were marked dirty have been removed already, and do not have to be traversed upon every update of the dependency. In our own use case this is greatly beneficial, as we're having up to 20'000 rows that have a computed that depends on one global computed, while only a fraction of rows is actually invoked.

[edison1105]

see vuejs/rfcs#212 (comment)

[posva]

Thank you for the boiled down and detailed report. This is a part of the problem related to the fact that reactive properties are only automatically cleaned and must be attached to a component instance to be cleared (called inside of setup). As pointed out, vuejs/rfcs#212 is meant to tackle the larger problem.

As a workaround, you can create something like this https://github.com/antfu/vueuse/blob/039aea9809c45f74a86cdef9248d821a1d24f8f0/packages/core/createGlobalState/index.ts#L3-L16 to be able to unmount the app manually and remove all dependencies.

[basvanmeurs]

Thanks @posva
The workaround is not really useful for our situation, in which we have a class-based model using reactivity. We want parts to be cleaned up, not the full app.

The propasal seems good for more advanced use cases, but it seems a bit overcomplicated for simple use cases. I don't want to have to create and cleanup a scope everywhere we're using a computed. The management of creating and cleaning them up is cumbersome, hacky and prevents syntax like:

class MyClass {
  public myComputed = computed(() => ...)
}

I can't wrap that computed in a scope at that place. I'd have to move their definitions to the class constructor and then create a manual destructor method which should be called from somewhere else. The management of calling it is an unnecessary complexity. Feels like boiler plate code as well.

All I'm suggesting: when dependees are triggered, just remove them from the depsMap. This is a tiny change, and will automatically cleanup stale computeds from the deps instead of holding on to them forever. Finally allowing the GC to clean them up. And it will save some performance as well when there is a large quantitiy of dependees that are not really used, reducing the deps set. This just works, scope or no scope.

[LinusBorg]

Yeah, The second proposal about clearing the Set might still be worth looking into, it seems to me.

Not necessarily as a bugfix for this rather than an optimization.

This is a tiny change, and will automatically cleanup stale computeds from the deps instead of holding on to them forever.

Tiny change but needs intense review to not miss other edge cases.

[basvanmeurs]

Added a PR. Tests are all passing as expected.

I verified with the original example that, after clicking the 'updateRef' button, the computeds and instances are indeed dereferenced.

[posva]

You are right, it's worth cleaning up the held reference if possible. Thanks for the PR!

[yyx990803]

Hmm, technically this isn't a leak, because a computed property creates an effect which needs to be stopped to release itself from all depMaps.

When you create a computed inside a component's setup(), its effect is automatically stopped when the component unmounts so the user don't need to think about it.

In your case, you need to make sure your class instances, when "cleared", properly stop all the effects it created (including raw effects, watchers, and the computed properties).

#2263 doesn't actually fix the problem since it only deletes the effect when the effect is triggered by the outer scope ref - i.e. you need to click "updateRef" after clear for it to work. If the ref is never updated, the class instances are still going to be kept in memory. Also, this only works for computed because its getter is run lazily, it wouldn't work for raw effects or watchers.

I would say what's proposed in vuejs/rfcs#212 should be the right solution here. In terms of ergonomics, I imagine something like this would help:

function autostop(factory) {
  let instance
  const scope = effectScope(() => {
    instance = factory()
  })
  instance.teardown = () => stop(scope)
}

// when creating class instances, wrap inside autostop
let aInstances = Array.from({ length: 10}, (x, i) => autostop(() => new MyClass(i)));

// when clearing instances, call stop()
aInstances.forEach(i => i.teardown())
aInstances = [];

Either way, I think we need to provide a way to explicitly stop computed property effects.

[basvanmeurs]

Hi Evan, thanks for your response. I can see the advantage of using scopes, and I like that PR. However, it would make sense to be able to stop the computeds individually, as we can do with watchers.