Add metrics runtime for Wasmer and debug for Wasmtime #40
Conversation
merged upstream
merge upstream + back to wapc naming
|
Interesting. There's an alternative to consider which is to allow wasmer and wasmtime to be configured independently, similar to wazero. In #34 via @Arsen6331, we added This also allows you to push down new functionality before abstracting it, which is handy as abstraction is hard to get right. |
|
@JanFalkin ps if you are game with doing the same thing as we did in wazero (make a library-specific hole to configure the other runtimes), I'll offer the labor to make a PR that does that including testing the things that are testable. Lemme know! |
There was a problem hiding this comment.
Food for thought. In wazero, the engine creation is deferrable, which allows you to decide whether to instantiate a new runtime per module or not. I'm not entirely sure how safe it is on the other runtimes to re-use their engine, especially in parallel.
Ex.
e := EngineWithRuntime(func(ctx context.Context) (wazero.Runtime, error) {
return r, nil
})
Based on the way we have it now, the engine instance is passed, so the same model could be tried. I am not certain at all that wasmer or wasmtime is safe to use in those instances. That would be for the user to protect by either getting new instances always or somehow safeguarding thread access and synchronize the threads such that only a single passes. |
|
I don't think a user should have to protect calls to the engine. This would be unnecessary for safe ones. Maybe a benchmark can be used, if any reason just to show if something crashes or not? Ex. I recall this because wazero tests ended up crashing on both wasmtime and wasmer. Basically I added a benchmark and then moved things to smaller scopes until they stopped crashing. If you do locking instead, then effectively you get no affect from pooling. https://github.com/tetratelabs/wazero/blob/main/internal/integration_test/vs/wasmtime/wasmtime.go#L47-L48 In any case the main advice is to change the structure but not the scope in this change. Ex change how it is initialized, but don't make it share an engine as the current code doesn't. If you want to make the change to have wasmer and wasmtime share an instance, maybe do that in another commit so it is easier to recognize and back-out (ex do it in a commit after this for example) |
There was a problem hiding this comment.
My feedback remains the same that the option for creating an engine should be a function of it (like in wasmtime) as that ensures the engine returned can close it. that matches the scope of the prior code, but still gives flexibility for the person using it to return a function of a constant engine.
PS whatever syntax here should also be applied to wazero as it would feel off otherwise. I can do that after the fact once things settle.
engines/wasmer/wasmer.go
Outdated
| } | ||
|
|
||
| func Engine(engs ...*wasmer.Engine) wapc.Engine { |
There was a problem hiding this comment.
I think you meant to use the func type
| func Engine(engs ...*wasmer.Engine) wapc.Engine { | |
| func Engine(options ...EngineOption) wapc.Engine { |
There was a problem hiding this comment.
I thought I had it as such and there was feedback from either @pkedy or @syrusakbary that had me switch to the specific engine*. I can put it back; I just want to make sure we don't go back and forth. Thanks
There was a problem hiding this comment.
yep. if the preference is to remove the option type I think it still exists in the file, so should also be removed from there.
There was a problem hiding this comment.
I see. Doing so now.
There was a problem hiding this comment.
Sorry ran out of time today. Will commit hopefully Sat night or Sunday.
There was a problem hiding this comment.
@codefromthecrypt Sorry about the delay; I was rushing on Friday night. I think the change was actually very small. Let me know if this is not what you were asking for.
engines/wasmtime/wasmtime.go
Outdated
|
|
||
| func Engine() wapc.Engine { | ||
| return &engineInstance | ||
| func Engine(engs ...*wasmtime.Engine) wapc.Engine { |
There was a problem hiding this comment.
I think you meant to define a func type:
| func Engine(engs ...*wasmtime.Engine) wapc.Engine { | |
| // EngineOption is a wasmtime-specific configuration, applied via Engine | |
| type Option func(*engine) | |
| func Engine(options ...EngineOption) wapc.Engine { |
engines/wasmer/wasmer.go
Outdated
| } | ||
|
|
||
| func Engine(engs ...*wasmer.Engine) wapc.Engine { |
There was a problem hiding this comment.
this feels a bit odd, is this common (using varargs to make one of the args optional)?
I think how wazero does it is a lot less confusing (having a second function vs being too clever with one)
func EngineWithRuntime(newRuntime NewRuntime) wapc.Engine {
There was a problem hiding this comment.
@codefromthecrypt I was looking at that too but, if I remember, there was a comment regarding removing the function and just passing the engine ptr. Without using the varargs syntax you would have to pass nil if you want a new engine. I thought that was hideous looking but I am certainly willing to make it a function as orig, I just don't want to reignite the comment of removing the function for the ptr. I can make it look like wazero if that is desirable?
There was a problem hiding this comment.
I feel somehow the design for supplying an engine/runtime for wazero became reviewed and merged, so we can use that precedent to make the others able to supply their engine/runtimes the same way. This may get you out of the muck of trying to merge conflicting feedback into the same design :D
There was a problem hiding this comment.
Works for me! I'll give it a whirl.
There was a problem hiding this comment.
@codefromthecrypt. Changes pushed
| } | ||
|
|
||
| // UnwrapStore allows access to wasmtime-specific features. | ||
| func (m *Module) UnwrapStore() *wasmtime.Store { |
There was a problem hiding this comment.
maybe add functions like this to wazero for parity, noting that the naming suffix should reflect the type returned and wazero doesn't call things store :D
There was a problem hiding this comment.
Makes sense, will do.
There was a problem hiding this comment.
Done. I think the names are consistent but different.
| ctx := context.Background() | ||
|
|
||
| // Configure waPC to use a specific wasmer feature. | ||
| e := EngineWithRuntime(func() (*wasmer.Engine, error) { |
There was a problem hiding this comment.
someone may feel like this should be EngineWithEngine to match the type, but I feel runtime is nicer than stuttering.
There was a problem hiding this comment.
Agreed. I was thinking along those lines as well, but the name is cumbersome to say the least.
There was a problem hiding this comment.
one note and then I think it is ready for @pkedy to do a final pass
engines/wazero/wazero.go
Outdated
| @@ -145,6 +144,16 @@ func (e *engine) New(ctx context.Context, host wapc.HostCallHandler, guest []byt | |||
| return | |||
| } | |||
|
|
|||
| // UnwrapCompiledModule allows access to wazero-specific compiled module. | |||
| func (m *Module) UnwrapCompiledModule() *wazero.CompiledModule { | |||
There was a problem hiding this comment.
this one is a bit niche. I'd remove it and instead add Instance.UnwrapModule
|
@JanFalkin I've been a bit busy but rest assured I'll get to this soon. |
BTW I am away starting this evening till tomorrow evening. In case you are looking for me. |
| @@ -0,0 +1,30 @@ | |||
| package wasmer | |||
There was a problem hiding this comment.
Please add this build tag. I have an M1 Mac apparently this feature is not supported.
//go:build amd64
// +build amd64|
|
||
| // Engine returns a new wapc.Engine which uses the DefaultRuntime. | ||
| func Engine() wapc.Engine { |
There was a problem hiding this comment.
Instead of having separate Engine and EngineWithRuntime functions, I wonder about using the Functional Options pattern here. It would be backward compatible. Is there any reason why we wouldn't use it?
https://golang.cafe/blog/golang-functional-options-pattern.html
There was a problem hiding this comment.
@pkedy I thought we had started with that pattern but moved to a wasm engine specific config/override. I am willing to modify as you all see fit, but I think we should now keep all engines consistent, and any change to wasmer should be reflected to wasmtime and wazero. This work specifically tried to normalize the creation/config behavior such that all engines look about the same.
There was a problem hiding this comment.
This can be consistent and backward compatible with functional options, but thinking about it more there might be an advantage to having the signature for Engine be the same (passing to higher order functions). Let's keep this as is.
There was a problem hiding this comment.
LGTM!
@JanFalkin Thank you for your unwavering engagement on this!
@codefromthecrypt Thank you for all the feedback!
|
@JanFalkin do you have any feedback since this is working? Are you using the feature successfully and how do you establish the cost and limits you use, if so. curious as we think about this generically. |
|
guess it would be a follow-up to tetratelabs/wazero#422 (comment) about if things are working now or not, and if so any insights since putting it together. |
|
@codefromthecrypt Stuff is working as expected. I have 2 scenarios that require the feature. The wasmer feature unfortunately has not been tested to a great extent however we have been using the dwarf debugging capabilities of wasmtime a lot. The metering is being tested at a basic level but we have not fully enabled the wasm engine in our demo and prod environments yet. I am ahead of the curve a bit. |
First this pull is intentional; sorry about the last false alarm. I am not certain if this is the best way to introduce variants to the engine. This pull utilizes some new functionality on wasmer that exposes usage metrics from the wasm engine wasmerio/wasmer-go@586bbfe. Unfortunately this requires the use of unsafe as we must provide a C callback for the metrics calculation. In addition to the work for wasmer's metrics, I have also included the code for wasmtime to enable a debug session in the presence of a DWARF enable debugger. Both of these features are included in 2 new "new" functions. Another possibility would be to provide construct data such that "new" itself would have enough context to determine how the engine should be created. The pull will hopefully inspire some discussion and I can adapt the code anyway we decide.