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

Wasmer 1.0.2 function memory leak #201

Closed
Polygens opened this issue Feb 12, 2021 · 11 comments
Closed

Wasmer 1.0.2 function memory leak #201

Polygens opened this issue Feb 12, 2021 · 11 comments
Labels
🐞 bug Something isn't working

Comments

@Polygens
Copy link

Describe the bug

Memory usage slowly builds up when calling wasm module functions causing our containers to go OOM.

Steps to reproduce

I've updated the demo repository at https://github.com/Polygens/wasmer-leak to reproduce the problem with wasmer 1.0.
More info in the readme.

Expected behavior

After every iteration of the for loop in the demo repo I would expect memory usage to remain consistent. If we run the demo with a single iteration it uses around 28MB.
Or at least garbage collected before going OOM.

Actual behavior

Instead of memory being released after every iteration it builds up and gets OOM killed.
image

Additional context

We just upgraded to 1.0 to make use of the new traps ❤️ but now we have an issue with the wasm functions taking too much memory. Not sure if this was present before 1.0, we had a similar issue with wasm instantiation but that seems unrelated.

@Polygens Polygens added the 🐞 bug Something isn't working label Feb 12, 2021
@syrusakbary
Copy link
Member

Hi @Polygens, thanks for opening the issue!

Could it be possible that the leak is in the wasm program itself?
I've checked the repo but I couldn't find the source code that generates the wasm.wasm file (so we can invalidate the wasm-leak thesis)

@Polygens
Copy link
Author

Hey @syrusakbary, of course! I updated the demo repo with a folder containing the rust source code and build command that was used.

@Hywan
Copy link
Contributor

Hywan commented Feb 16, 2021

I'm able to reproduce the leak. Now I've to look to know where it comes from :-).

@Hywan
Copy link
Contributor

Hywan commented Feb 16, 2021

There is no leak inside the Wasm module. I've updated transform to a no-op and the leak is still present.

@Hywan
Copy link
Contributor

Hywan commented Feb 16, 2021

Here is my script to reproduce:

func TestFoo(t *testing.T) {
	engine := NewEngine()
	store := NewStore(engine)
	module, err := NewModule(
		store,
		[]byte(`
			(module
			  (type $sum_t (func (param i32 i32) (result i32)))
			  (func $sum_f (type $sum_t) (param $x i32) (param $y i32) (result i32)
			    local.get $x
			    local.get $y
			    i32.add)
			  (export "sum" (func $sum_f)))
		`),
	)
	assert.NoError(t, err)

	instance, err := NewInstance(module, NewImportObject())
	assert.NoError(t, err)

	sum, err := instance.Exports.GetRawFunction("sum")
	assert.NoError(t, err)

	for {
		result, err := sum.Call(1, 2)
		assert.NoError(t, err)
		assert.Equal(t, result, int32(3))
	}
}

@Hywan
Copy link
Contributor

Hywan commented Feb 16, 2021

I've found a “leak”. Actually it's not a leak. It's a design issue. The fix is explained in #207.

However, I've found a much smaller leak. I'm still digging. We lost 1Kb every 10sec, rather than 1Gb every 10sec :-p.

@Hywan
Copy link
Contributor

Hywan commented Feb 16, 2021

It seems that it's just Go playing with the GC. The memory doesn't increase with the time. It runs since 3mn, and it's still 12.3Mb. It seems the problem is solved.

@Polygens Can you test with the master branch please?

@Hywan Hywan closed this as completed Feb 16, 2021
@Hywan Hywan reopened this Feb 16, 2021
@Polygens
Copy link
Author

Hey @Hywan, your fix does seem to work!

We're no longer able to replicate the memory issue for function calls when using master. Thank you for fixing it so quickly ❤️

While trying your fix I did notice that we still have some memory increase in our actual services that use wasmer-go, but this seems related to instance.Exports.GetMemory("memory") demonstrated in at https://github.com/Polygens/wasmer-leak/blob/9cd1c01c47cfab5bf9511ab2228bc81ac251870b/main.go#L63 with some dummy code. Maybe this is just bad practice to call getMemory in a hot loop like this?

Should I create a new issue for this?

@Hywan
Copy link
Contributor

Hywan commented Feb 18, 2021

I've analyzed this particular case, and it's actually quite similar to what instance.Exports.GetFunction does.

When you're calling Exports.GetMemory, a new Memory object is created. It's “owned by” Exports (which is “owned by” Instance). Consequently, the garbage collector won't collect Memory in that case because Instance is still used/alive/outlasts Memory. (Actually, it collects Memory but it frees the pointer to wasm_memory_t without calling the destructor, so nothing is actually being freed).

I advice you to do something like this instead:

	memory, _ := instance.Exports.GetMemory("memory")

	for {
		// Calls the exported function with the start and length of the inputJson in memory
		// The return value is an integer containing the start and length of the transformed json in memory
		_, err := transform(0, len(input))
		if err != nil {
			logrus.Fatal("failed to transform: %w", err)
		}

		logrus.Debug(memory.Data())
	}

@Polygens
Copy link
Author

@Hywan Makes perfect sense, we already refactored our code to do it this way.
Thanks for looking into this, closing the issue!

@Hywan
Copy link
Contributor

Hywan commented Feb 18, 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

No branches or pull requests

3 participants