WeakMap memory leak

[lujjjh]

package gojatest

import (
	"runtime"
	"testing"
	"time"

	"github.com/dop251/goja"
)

func TestWeakMap(t *testing.T) {
	for i := 0; i < 100; i++ {
		r := goja.New()
		r.RunScript("", "var x = {}; new WeakMap().set(x, {})")
		r = nil
	}
	runtime.GC()
	time.Sleep(100 * time.Millisecond)
	runtime.GC()
	time.Sleep(100 * time.Millisecond)
}

$ go test weakmap_test.go -memprofile=/tmp/heap

There are still tons of inuse_objects:

Still have no idea why (it seems to be related with runtime.SetFinalizer).

[lujjjh]

new WeakMap().set(x = {}, {})

There is a circular reference:

x.weakColls references to the runtime via the value {}, while the runtime references to x (in globalObject).

So the finalizer on x.weakColls will never be called since x.weakColls will never be garbage collected.

[dop251]

You are right. I tried to make the implementation as unobtrusive as possible because many people don't use weak collections. Unfortunately, I failed 😞

Your particular case could be fixed by introducing a wrapper around Runtime, so that the current runtime becomes unexported:

type Runtime struct {
    *runtime
}

then setting a finalizer on *Runtime which clears the globalObject.

However, this will not work if an object is used as its own value in a weak collection (i.e. if you do m.set(x, x)).

The only way I can see to solve this would be doing the same wrapper trick on *Object. This would mean, however, an extra pointer for each object (even if they are never part of any weak collection and even if weak collections are not used). Not sure this will help the GC and the overall performance and not sure I want to do this, seems a rather drastic change for a corner case.

What I could do instead is detect this and remove the finalizer, so that at least the object is garbage-collected when the weakmap itself becomes unreferenced.

Unless there is a better suggestion, I'll do it.

[dop251]

In fact, it won't work in a more generic case: if the value has a reference to the key, i.e. m.set(x.y, x))

[lujjjh]

Your particular case could be fixed by introducing a wrapper around Runtime [...] then setting a finalizer on *Runtime which clears the globalObject.

I actually encountered this problem in core-js, enforceInternalState(value) operates as m.set(value, {}) to an internal WeakMap while the key value is a function which overwrites Function.prototype.toString. So I should clear the runtime.global.FunctionPrototype as well. (Or simply set WeakMap to goja.Undefined() before running core-js to workaround.)

In fact, it won't work in a more generic case: if the value has a reference to the key, i.e. m.set(x.y, x))

Yes, I'm also thinking over if there is a better solution which can also cover this case.

[dop251]

Giving it more thought, I don't think adding a finalizer to Runtime is a good idea. Imagine there is a custom struct that refers to Runtime and this struct is put into the globalObject. Suddenly this makes the whole Runtime not garbage-collectable.

And even the trick with *Object won't solve all cases. In fact I believe there is no perfect solution within the current Go functionality. There is simply no way to guarantee there are no circular references (for example the key may be the map itself).

I know Go authors don't want to make improvements with finalizers because they believe they are "overused". Maybe it's worth making a case to them?

[lujjjh]

I know Go authors don't want to make improvements with finalizers because they believe they are "overused". Maybe it's worth making a case to them?

I agree that Go should improve finalizers, however, not likely in the short term.

Currently, maybe we could implement a "lazy gc" on WeakMap? That is, clean the keys only when accessing the map such as m.has, m.get and m.set (or periodly in background / every N instructions?).

In that case, a weakCollection would not hold a reference to the data of WeakMap (hence no circular references), but simply a struct that records the keys to be removed.

type weakMap struct {
	// need to synchronise access to the data map because it may be accessed
	// from the finalizer goroutine
	sync.Mutex
	data     map[uint64]Value
	removals *removals
}

// TODO: locks.
type removals struct{
	keys []uint64
}

func (r *removals) removeId(id uint64) {
	r.keys = append(r.keys, id)
}

func (vm *weakMap) gc() {
	for _, id := range vm.removals.keys {
		vm.removeId(id)
	}
	vm.removals.keys = nil
}

func (wm *weakMap) set(key *Object, value Value) {
	wm.gc()
	...
	refs.add(wm.removals)
}

[dop251]

I was thinking along the same line. This approach seems the most reasonable. Hopefully the change will fix the problem and will not create any unwanted side effects.

[ushio117]

good job!