Skip to content
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

Memory leak involving instances and WrapFunc (due to cycle?) #57

Closed
tsandall opened this issue Feb 10, 2021 · 6 comments
Closed

Memory leak involving instances and WrapFunc (due to cycle?) #57

tsandall opened this issue Feb 10, 2021 · 6 comments

Comments

@tsandall
Copy link

Hello!

I'm running into a memory leak in v0.22.0 after picking up the fix for #42. Here's a simple program that reproduces it:

package main

import (
	"runtime"

	"github.com/bytecodealliance/wasmtime-go"
)

type X struct {
	instance *wasmtime.Instance
}

func (x *X) F() {

}

func main() {

	bs, err := wasmtime.Wat2Wasm(`
	(module
	  (import "" "hello" (func $hello))
	  (func (export "run")
		(call $hello))
	)
  `)
	check(err)

	for {

		x := &X{}

		store := wasmtime.NewStore(wasmtime.NewEngine())
		imports := []*wasmtime.Extern{wasmtime.WrapFunc(store, x.F).AsExtern()}
		module, err := wasmtime.NewModule(store.Engine, bs)
		check(err)

		instance, err := wasmtime.NewInstance(store, module, imports)
		check(err)

		x.instance = instance

		runtime.GC()
	}
}

func check(e error) {
	if e != nil {
		panic(e)
	}
}

This is a extension of the example from #42. The important difference is that the function passed to WrapFunc closes over the Instance because it's a member function of x. I think this creates a cycle that ultimately prevents the GC from calling the Instance finalizer.

I added some print statements into instance.go and func.go and confirmed that:

  • The instance finalizer is never called
  • The goFinalizeWrap function is never called and gWrapMap grows indefinitely
@alexcrichton
Copy link
Member

Thanks for the report! Unfortunately this is a difficult problem with no easy solution. As you've noticed there's a cycle between Rust's memory and Go's memory, concretetly:

  • Rust has an instance in memory
  • This instance has imported functions
  • One imported function is a Go-defined function
  • This Go-defined function closes over the Go instance
  • The Go instance points to the Rust instance in memory

My recommendation would be to not close over Store-related objects and instead use Caller if you can to access values. FWIW a similar problem exists in Rust if you create cycles manually as well.

@tsandall
Copy link
Author

tsandall commented Feb 11, 2021

Right now the code that instantiates everything provides the memory as an import alongside those Go-defined functions. If the Go-defined functions close over that memory, I'm assuming that'll also create a cycle and we'll still see the leak.

It looks like the Go-defined functions can get access to exported functions from the module via the Caller. Is it possible to also get access to the memory via the Caller?

UPDATE: It looks like the Caller only provides access to exports. Would it be possible to extend the Caller to provide access to imported memory?

@alexcrichton
Copy link
Member

Yes unfortunately closing over a Memory would also create a leak. As you've seen though the intended use case to fix this is to use Caller to acquire the exported memory (or anything else if necessary). If an imported memory is reexported then that can be accessed, otherwise if it's only imported then you'll be able to close over it in the Go host function I think?

@tsandall
Copy link
Author

@alexcrichton thanks for the quick reply! I'll try out the re-export approach.

otherwise if it's only imported then you'll be able to close over it in the Go host function I think?

Can you explain this a bit more? I don't see how the Go host function can close over the memory without creating a leak.

@alexcrichton
Copy link
Member

Hm no you're right, I was mistaken. I'm thinking of a historical design of Wasmtime, but yeah nowadays if you close over anything connected to a Store you'll cause a leak.

@alexcrichton
Copy link
Member

Ok this was fixed in the API redesign of #81, so I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants