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 #262

Closed
prep opened this issue May 2, 2021 · 5 comments · Fixed by #277
Closed

Memory leak #262

prep opened this issue May 2, 2021 · 5 comments · Fixed by #277
Assignees
Labels
🐞 bug Something isn't working

Comments

@prep
Copy link
Contributor

prep commented May 2, 2021

Apologies for creating another issue.

Describe the bug

I think there might be a memory leak when creating instances.

A clear and concise description of what the bug is

When I create an instance with a small module in an endless loop, the memory as is shown by Activity Monitor on macOS is ever increasing at a higher rate than Go's memory stats show.

Steps to reproduce

The example in #148 seems to show the problem, but the version below shows Go's internal memory stats:

package main

import (
	"fmt"
	"log"
	"runtime"
	"time"

	"github.com/wasmerio/wasmer-go/wasmer"
)

func runInstance() error {
	wasmBytes := []byte(`
		(module
		  (type $add_one_t (func (param i32) (result i32)))
		  (func $add_one_f (type $add_one_t) (param $value i32) (result i32)
		    local.get $value
		    i32.const 1
		    i32.add)
		  (export "add_one" (func $add_one_f)))
	`)

	store := wasmer.NewStore(wasmer.NewEngine())

	module, err := wasmer.NewModule(store, wasmBytes)
	if err != nil {
		return fmt.Errorf("Unable to compile module: %w", err)
	}

	instance, err := wasmer.NewInstance(module, wasmer.NewImportObject())
	if err != nil {
		return fmt.Errorf("Unable to instantiate the module: %w", err)
	}

	addOne, err := instance.Exports.GetFunction("add_one")
	if err != nil {
		return fmt.Errorf("Unable to fetch the add_one function: %w", err)
	}

	if _, err = addOne(1); err != nil {
		return fmt.Errorf("Unable to call the add_one function: %w", err)
	}

	return nil
}

func mb(b uint64) uint64 {
	return b / 1024 / 1024
}

func main() {
	go func() {
		for {
			time.Sleep(time.Second)
			runtime.GC()

			var m runtime.MemStats
			runtime.ReadMemStats(&m)
			log.Printf("alloc: %d Mb total alloc: %d Mb sys: %d Mb", mb(m.Alloc), mb(m.TotalAlloc), mb(m.Sys))
		}
	}()

	for {
		if err := runInstance(); err != nil {
			log.Fatalf("Unable to run instance: %s", err)
		}
	}
}

Expected behavior

I expect the memory usage to grown a bit and then even out to a stable number.

Actual behavior

The memory usage keeps increasing.

Additional context

I've confirmed that the instance's runtime.SetFinalizer() hook is properly called, so the instance appears to be deleted by the embedded call to wasm_instance_delete() as expected.

From the above program:

2021/05/02 15:59:29 alloc: 49 Mb total alloc: 169 Mb sys: 87 Mb

From Activity Monitor on macOS:
activity monitor

@prep prep added the 🐞 bug Something isn't working label May 2, 2021
@Hywan Hywan self-assigned this May 3, 2021
@prep
Copy link
Contributor Author

prep commented May 8, 2021

Hey Ivan,

I did some more digging on this and from what I can tell it appears that Exports{} isn't GCed. Which is to say, I set up some simple log.Print() calls in the runtime.SetFinalizer() hooks for Instance{}, Exports{} and Memory{} and only the finalizer func for Instance{} appears to be called.

@prep
Copy link
Contributor Author

prep commented May 16, 2021

Although it didn't fix the problem of the memory leak, the GC not cleaning up Exports{} has to do with the fact that the exports map[string]*Extern field creates a circular reference between Exports{} and Extern{}. I was able to solve this by changing the runtime.SetFinalizer() hook in Instance{} to the following:

	runtime.SetFinalizer(self, func(self *Instance) {
		self.Exports.exports = nil
		C.wasm_instance_delete(self.inner())
	})

@Hywan
Copy link
Contributor

Hywan commented May 17, 2021

Congrats for the fix! And thanks for digging the problem! Can you open a PR please?

@Hywan
Copy link
Contributor

Hywan commented Jun 28, 2021

OK so finalizers aren't running because the document says:

it will run eventually

There is no guarantee it will run.

I'll implement the Closer interface for types, as we did before the 1.0 version.

@Hywan
Copy link
Contributor

Hywan commented Jun 28, 2021

@prep Can you try this patch #277 and tell me if it fixes your problem. I've found several problems that I'll describe in the PR a little bit later. I though Go GC was better at collecting orphan objects, but it appears no. I'm continuing my digging to find more potential issues but I believe most of the problems have been solved now for your usecases.

Hywan added a commit to Hywan/go-ext-wasm that referenced this issue Jun 28, 2021
First, this patch provides new `Close` methods on some “top types”
(`Store`, `Module`, `Instance` etc.). The `Close` methods are
redundant to the runtime finalizers, but they are useful to force to
destruct a specific type manually before the Go GC collect it.

Note that calling `Close` will remove the runtime finalizer attached
to the object.

Note that runtime finalizers call the `Close` method if defined on the
current object.

Second, this patch forces the destructors (now the `Close` methods) to
destruct the children attached to the current object. Let's see it in
details:

* `Module` now has two new fields `importTypes *importTypes` and
  `exportTypes *exportTypes`. The value is null, except if the
  `Imports` or `Exports` methods are called. They will store their
  respective results in their respective fields. The goal of this
  change is twofold:

  1. Avoiding computing the same import and export types multiple
     times each time one of the method is called,

  2. By holding a reference to the private `importTypes` and
     `exportTypes`, `Module` can free them. Before that, the objects
     were orphan and the garbage collector had difficulties to collect
     them.

* `Instance` now forces to free its `Exports`,

* `Exports` now force to free all the items of its `exports` map
  field. Before this patch, only the `_inner` C pointer was freed, but
  not the map. I have no idea why the map wasn't collected by the Go
  GC.

Running the script written by @prep in
wasmerio#262 shows that there is
no more memory leaks.
@Hywan Hywan closed this as completed in #277 Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants