computed on a shared reactive causes a memory leak

wcldyx commented 4 years ago

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Title</title>
    <script src="https://cdn.jsdelivr.net/npm/vue"></script>
    <script src="https://unpkg.com/@vue/composition-api/dist/vue-composition-api.umd.js"></script>
</head>
<body>
<div id="app"></div>
<script>
    Vue.use(vueCompositionApi.default);

    //1. Create a globally Shared reactive object
    const store = vueCompositionApi.reactive({
        list: new Array(100000).fill('').map((_, index) => {
            return {
                index
            }
        })
    })

    // 2.Use the global Shared reactive object in the "computed" function in the "Item" component
    const Example = {
        template: `<div>{{ls.length}}</div>`,
        setup() {
            return {
                ls: vueCompositionApi.computed(() => store.list)
            }
        }
    }

    // 3, On the "App" component, use the "toggle" button to continuously create and destroy the "Item" component
    const App = {
        components: {Example},
        template: `
            <div>
                <button @click="show = !show">toggle</button>
                <example v-if="show"></example>
            </div>`,
        setup() {
            return {
                show: vueCompositionApi.ref(false)
            }
        }
    }

    new Vue({
        el: '#app',
        render: h => h(App)
    })
</script>
</body>
</html>

Keep clicking the toggle button, you will find that the memory keeps increasing and will not be destroyed. 1583170840(1)

wcldyx commented 4 years ago

Can anyone see it, can you help me

wcldyx commented 4 years ago

@LinusBorg Can you help me with this problem

LinusBorg commented 4 years ago

Here we create a small vm to handle the computed property:

https://github.com/vuejs/composition-api/blob/8cea63aadba5ea75969389e1c438b6fa5e775821/src/apis/computed.ts#L28-L35

If the computed property references objects/state that persists your host component's lifecylce, it will lead to the memory leak.

So we should make sure to destroy this instance once the host component is destroyed:

vm.$on('hook:destroyed', () => computedHost.$destroy())

will test this later.

LinusBorg commented 4 years ago

So a Little update on this:

Scope of the issue.

This memory leak happens like this:

You have a reactive object that lives outside of a component and outlives the component lifecycle
You create computed() refs that have a dependency for this object in a component
When the component gets destroyed, the computed() ref itself is garbage collected, but a Vue instance, created internally by computed() stays in memory, causing a leak for each such computed() that's being created and then disposed.

Cause

To implement computed(), we create a small new Vue() instance and add the argument of computed() as a computed property:

https://github.com/vuejs/composition-api/blob/8cea63aadba5ea75969389e1c438b6fa5e775821/src/apis/computed.ts#L28-L35

This is necessary to actually achieve the typical computed behaviour.

However, the means that:

A Watcher is created for this computed property.

https://github.com/vuejs/vue/blob/a59e05c2ffe7d10dc55782baa41cb2c1cd605862/src/core/instance/state.js#L186-L192

This watcher is added as a subscriber to the Dep of this reactive state.

https://github.com/vuejs/vue/blob/a59e05c2ffe7d10dc55782baa41cb2c1cd605862/src/core/observer/dep.js#L23-25

That means that the Dep instance has a reference to this watcher, and the watcher has a reference to that Vue instance

As we can't track or be informed when a compute() ref is being gargabe collected, we have to way to clean up this watcher and therefore, this reference

Previous attempts at a fix

In #277, I thought that I could solve the problem by destroying the Vue instance when the component in which we created the computed() unmounts.

But that means that this computed reference, if not being garbage collected, will stop working, event tough both thje computed ref instance and the reactive state is depends on still exist.

So I abandoned it.

Other ideas

Currently investigating, will report once I have something to write down.

shaonialife commented 4 years ago

is this OK ?

LinusBorg commented 4 years ago

Thanks for helping out!

I think this will have a similar problem as my approach. Think of this scenerio:

import { reactive, computed } from '@vue/composition-api'
import store from '/.someReactiveObject'

export default defineComponent({
  setup() {
   const parentInterface =  inject('someKyey')
   const countPlusOne = computed(() => store.counter + 1)
   parentInterface.passComputed(countPlusOne)   
  }
})

This is of course nonsensical, but it should be easy to imagine that we want to pass our computed property to the parent, or some totally different module that we imported, for example. One of the advantages of having computed refs is that we an pas them around, after all!

In such a case, your approach, as well as mine, would break the internal computation of countPlusOne when the component is being destoryed, even though the computed ref is still referenced by the parent component somewhere, and likely expected to work as normal.

That's basically the reason that I dropped my initial idea.

commyfriend commented 4 years ago

any progress here? I meet the same problem.

LinusBorg commented 4 years ago

So I checked this out again and compared what we did in vue-next. I learned that we basically chose to do what I initially wanted to do here as well: destroy the watcher (or as its called in vue-next internally: thee effect) on unmount

See:

https://github.com/vuejs/vue-next/blob/5f8967479023068391bb467633894362f12c83bb/packages/runtime-core/src/apiComputed.ts#L18

https://github.com/vuejs/vue-next/blob/5f8967479023068391bb467633894362f12c83bb/packages/runtime-core/src/component.ts#L534-L538

https://github.com/vuejs/vue-next/blob/5f8967479023068391bb467633894362f12c83bb/packages/runtime-core/src/renderer.ts#L1818-L1821

So I think we can resurrect #277

wcldyx commented 4 years ago

Hello everyone, is this problem fixed, why not update it?

LinusBorg commented 4 years ago

We will. Lots of things to do right now, please be patient.

antfu commented 4 years ago

Hi @LinusBorg, I might have some idea

we want to pass our computed property to the parent

How can we pass a computed to the parent? passComputed seems not to be an API (sorry I am not that familiar with vue-next). Could you lead me to some code entry? I guess we can try to collect VMs that the computed attached to. And after all the VMs are destroyed, we could dispose the computed safely.

or some totally different module that we imported.

For persisted/global computed, we could suggest users define them outside of any instance. So the computed won't attach to any VM, then the problem never bothers.

    const store = vueCompositionApi.reactive({
        list: new Array(100000).fill('').map((_, index) => {
            return {
                index
            }
        })
    })

+   const ls = vueCompositionApi.computed(() => store.list)

    // 2.Use the global Shared reactive object in the "computed" function in the "Item" component
    const Example = {
        template: `<div>{{ls.length}}</div>`,
        setup() {
            return {
-                ls: vueCompositionApi.computed(() => store.list)
+                ls
            }
        }
    }

vuejs / composition-api