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

Adds wazero implementation to make this CGO-free #19

Closed
wants to merge 3 commits into from

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Oct 29, 2022

This adds a wazero implementation in order to make the project CGO-free.
For example, there's no need to load native libraries (as currently
exists with wasmer). To achieve this, I added a go.mod file for wasmer.

Fixes #15

@codefromthecrypt
Copy link
Contributor Author

I'll rebase this after #18 is merged, most of the change here is in that PR, just it hasn't merged yet

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Oct 29, 2022

Note: I had an odd compile error until I inlined the example header into the same file:

./main.go:69:40: undefined: myHeaderMap

After that (last commit) the example works fine using wazero:

$ cd example
$ go run main.go
receive request /
print header from server host, User-Agent -> [curl/7.84.0]
print header from server host, Accept -> [*/*]
receive request /order-fries?size=super
print header from server host, Accept -> [*/*]
print header from server host, User-Agent -> [curl/7.84.0]
^Csignal: interrupt

* limitations under the License.
*/

package wazero
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's where the actual code begins

w.data = data
}

func (w *Instance) Acquire() bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept all the locking as-is except there's no need to cache function or memory references in wazero.


// Call implements common.WasmFunction
func (f *wasmFunction) Call(args ...interface{}) (interface{}, error) {
realArgs := make([]uint64, 0, len(args))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the second direction of reflection. Not only are the function imports defined reflectively, they are also accessed the other way in reflectively. I think the intent was for this to be a generalized swappable runtime, but only one runtime lived here until now. This is the point I'd suggest being direct instead. make it a single runtime and then you don't need to do things reflectively. doing so would couple to wazero, but also make things a lot faster and also get back go context (as well drop all the system dependencies). worth a thought!

@codefromthecrypt codefromthecrypt changed the title Adds wazero Adds wazero implementation to make this CGO-free Oct 29, 2022
@codefromthecrypt
Copy link
Contributor Author

ok this is ready, except #18 needs to be merged first

This adds a wazero implementation in order to make the project CGO-free.
For example, there's no need to load native libraries (as currently
exists with wasmer). To achieve this, I added a go.mod file for wasmer.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
example/main.go Show resolved Hide resolved
wazero/instance.go Outdated Show resolved Hide resolved
Adrian Cole added 2 commits October 31, 2022 15:38
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

This change can't go in unless mosn can commit to a floor version of 1.18. definitely wazero will stop compiling with 1.17 next release.

@antJack
Copy link
Contributor

antJack commented Oct 31, 2022

for those <1.18ers, they could still build with wasmer

@codefromthecrypt
Copy link
Contributor Author

Please commit to a version policy of some kind. when you get to the point of 1.18 let me know and I can start helping again. There's no point recommending people to use wasm through wazero if the project can't commit to working with 1.18.

@codefromthecrypt
Copy link
Contributor Author

the only way to let some unknown audience of <1.18 users remain working is if mosn tests that version. Otherwise, it is manual guessing and auditing each library dependency or worse, never updating any of them. I thought that CI was how things would decide what is supported, but what I understand here is that you are saying 1.18 can also work, and versions below it are also supposed to work, but not tested. The whole thing will be less subjective if you can as a project commit to a version range and test that. Ex we test 1.18 and 1.19 and no way can we promise other versions will keep working.

@codefromthecrypt
Copy link
Contributor Author

I don't think it makes sense to merge this with a build flag on go version, especially when we can't know what version mosn supports. Let's wait until the project can make a decision on go version. Otherwise integration is a mess, like the wasm engine and everything it touches needs to have a go118 flag? Before making this decision, I'd recommend doing a go get -u ./... on mosn as you may already have deps that need at least 1.16 or 1.17 already, just don't know yet because the ones in use are so old.

@codefromthecrypt
Copy link
Contributor Author

I understand mosn will allow libraries to depend on features in a go version one behind what go supports, so 1.17. I checked and the critical code here still works in 1.17. once mosn/api#51 is approved, I'll start again. If it isn't I'll close this PR.

@codefromthecrypt
Copy link
Contributor Author

This will take more work as I noticed after looking more carefully. #21

I'll put it as a draft for now.

@codefromthecrypt codefromthecrypt marked this pull request as draft November 2, 2022 08:00
@codefromthecrypt
Copy link
Contributor Author

ok status update. wazero works fine with the v2 ABI. I think the v1 ABI is not worth the effort to port, as not only is it unusable in current SDKs, it is slower also. I've confirmed locally that wazero is faster than wasmer in context lifecycle by a large order, and avoiding reflection makes the gap much larger as well.

If you'd like to proceed with making this performant, I'd encourage moving forward and #22 . We can keep the slow reflection if that's desirable, but I think it is a poor choice to do that. In fact I would recommend deleting this whole module as mosn shouldn't be slow for no good reason. cc @taoyuanyuan

@codefromthecrypt
Copy link
Contributor Author

I'm closing this as I think rehabilitating this module wastes contributor effort vs doing it again. If this project becomes healthy and current, please ping me!

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

Successfully merging this pull request may close these issues.

Can you add the implementation and example of wazero?
2 participants