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 management extremely error prone #415

Open
miretskiy opened this issue Jan 10, 2025 · 0 comments · May be fixed by #417
Open

Memory management extremely error prone #415

miretskiy opened this issue Jan 10, 2025 · 0 comments · May be fixed by #417
Labels
🐞 bug Something isn't working

Comments

@miretskiy
Copy link

miretskiy commented Jan 10, 2025

Recently, I've authored #414
to fix some of the memory issues; However, more issues remain.
Indeed, if you simply run go test -test.count 10 -- chances are the package test
will fail. It so happens that the failures will happen inside TestSumLoop test (oftentimes)
which was added by the above PR.

However, it's unlikely that that test is to blame -- rerunning that test 10k times didn't yield
a single failure. So, what gives? After adding more debug information, it became apparent that the
panic is experienced in the TestSumLoop test, but it is caused by another test that finished running, namely
TestImportTypeForTableType

The Finalizer set by

runtime.SetFinalizer(valueType, func(valueType *ValueType) {
causes the go test to panic
when running some other test.

The failure itself can be reproduced by running TestImportTypeForTableType by itself (with something like -test.count=50).
This leads me to think that the memory management model used in wasmer-go is somewhat unworkable in its current state.

While I appreciate the desire to have finalizers to free the memory, doing so is problematic for few reasons:

  1. SetFinalizer documentation indicates that:
// There is no guarantee that finalizers will run before a program exits,
// so typically they are useful only for releasing non-memory resources
// associated with an object during a long-running program.

So, it kinda says: don't rely on it for memory management.

  1. There are way too many objects that are created -- engine,, module, store, function, imports, exports, types, etc -- to know
    and understand which ones may be missing a KeepAlive call.

  2. Requiring callers to close resources is not that bad -- that's very much idiomatic Go.

Wasmtime also relies on finalizers; however they try a bit harder to make sure things work okay:
https://github.com/bytecodealliance/wasmtime-go/blob/66086d3cc5430a0026da19f98542946acd908006/ffi.go#L19
Perhaps something like this needs to be implement in wasmer-go as well.

@miretskiy miretskiy added the 🐞 bug Something isn't working label Jan 10, 2025
miretskiy pushed a commit to miretskiy/wasmer-go that referenced this issue Jan 13, 2025
Informs wasmerio#415

As documented in wasmerio#415,
the memory mode exposed by wasmer-go is error prone and hard to use.

Introduce a `CPtrBase[T]` helper struct to attempt to surface
errors often and early.  The `CPtrBase` is meant to store a CGo pointer.
Then, through conditional compilation, a debug implementation, enabled
via `-tags memcheck` tag, is used, which adds expensive pointer access
checks as well as forces calls to `runtime.GC()` whenever underlying
pointer accessed.  These access are very slow -- however, they cause
almost immediate crashes to surface in tests.

It's likely that not all issues have been identified and fixed;
These fixes that were identified were verified by running tests with
at least 5000 successful runs.
miretskiy pushed a commit to miretskiy/wasmer-go that referenced this issue Jan 14, 2025
Fixes wasmerio#415

As documented in wasmerio#415,
the memory mode exposed by wasmer-go is error prone and hard to use.

Introduce a `CPtrBase[T]` helper struct to attempt to surface
errors often and early.  The `CPtrBase` is meant to store a CGo pointer.
Then, through conditional compilation, a debug implementation, enabled
via `-tags memcheck` tag, is used, which adds expensive pointer access
checks as well as forces calls to `runtime.GC()` whenever underlying
pointer accessed.  These access are very slow -- however, they cause
almost immediate crashes to surface in tests.

It's possible that some issues still remain;
The fixes were applied in order to ensure that the tests pass 5000
times.  Beyond that, there could still be remaining issues.

Release note (<category, see below>): <what> <show> <why>
@miretskiy miretskiy linked a pull request Jan 14, 2025 that will close this issue
miretskiy pushed a commit to miretskiy/wasmer-go that referenced this issue Jan 14, 2025
Fixes wasmerio#415

As documented in wasmerio#415,
the memory mode exposed by wasmer-go is error prone and hard to use.

Introduce a `CPtrBase[T]` helper struct to attempt to surface
errors often and early.  The `CPtrBase` is meant to store a CGo pointer.
Then, through conditional compilation, a debug implementation, enabled
via `-tags memcheck` tag, is used, which adds expensive pointer access
checks as well as forces calls to `runtime.GC()` whenever underlying
pointer accessed.  These access are very slow -- however, they cause
almost immediate crashes to surface in tests.

It's possible that some issues still remain;
The fixes were applied in order to ensure that the tests pass 5000
times.  Beyond that, there could still be remaining issues.
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.

1 participant