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

bugfix: Tighten up memory model. #417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miretskiy
Copy link

@miretskiy miretskiy commented Jan 14, 2025

Fixes #415

As documented in #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.

@miretskiy
Copy link
Author

Sorry about the various changes to the comments; those were done by go fmt.

wasmer/valuetype.go Outdated Show resolved Hide resolved
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.
@miretskiy
Copy link
Author

@syrusakbary do you think this approach makes sense?

@miretskiy
Copy link
Author

@ayys @syrusakbary : not sure who can review this; can you recommend somebody if you can't?
wasmer-go doesn't have any people, so it's hard to see who should review the code.

@baalimago
Copy link
Collaborator

baalimago commented Feb 1, 2025

Hi! I work for Wasmer as SRE and the primary focus of Wasmer is elsewhere (getting the Go SDK up and running is not a prio right now). But! I'm personally a gopher, and decided to take on wasmer-go as a pet project (hobby). I more on the cloudy side, don't know that much about compilers, C etc, but do know CI/CD and golang at large.

So, I just merged this pr which cleans up the project quite a bit and adds code a QA pipeline with gofumpt, staticcheck and other goodies. Could you please rebase on master? The conflicts are most likely the formatted files, and I also fixed a few staticcheck issues. Once rebased, I'll enable workflow runs for you. Then I'll review this PR more properly and we can take it from there

Thanks for your contribution, it's much appreciated!

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

Successfully merging this pull request may close these issues.

Memory management extremely error prone
2 participants