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

feat: abstract away Wazero's api.ValueType and api.EncodeXX/api.DecodeXX #44

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

mhmd-azeez
Copy link
Collaborator

Currently we are exposing these wazero APIs:

The idea of this PR is hide away wazero from normal usage. If the user doesn't need any advanced configuration or functionality, then they shouldn't have to know about Wazero. That's why we are abstracting away api.EncodeXX/api.DecodeXX and api.ValueType since they will be used whenever you write a host function.

RuntimeConfig and ModuleConfig are about giving the user fine grain control of the underyling configurations. We expose some of the functionality as part of the manifest as well, see this for a more detailed breakdown.

And api.Memory is for write-through access of the underyling memory.

The only thing I am a bit worried about is RuntimeConfig.WithCloseOnContextDone because by default wazero sets it to false, which might be very confusing. We do set it to true if the user configures a timeout though.

@nilslice
Copy link
Member

nilslice commented Dec 5, 2023

The only thing I am a bit worried about is RuntimeConfig.WithCloseOnContextDone because by default wazero sets it to false, which might be very confusing. We do set it to true if the user configures a timeout though.

Do you think the optimal default behavior is to set this to true on behalf of the SDK user? If so, we could add some method to disable it and override that default.

Otherwise, I think generally this is a good update! As we discussed, it limits the amount of packages imported by the typical user (just Extism Go SDK), and thus is less likely to introduce version conflicts between Extism Go SDK and whichever Wazero module is imported by the end user.

@mhmd-azeez
Copy link
Collaborator Author

@nilslice

Do you think the optimal default behavior is to set this to true on behalf of the SDK user? If so, we could add some method to disable it and override that default.

For DX, it's better to be set to true by default, but it hurts performance quite a bit:

func BenchmarkCountVowels(b *testing.B) {
	ctx := context.Background()

	b.ResetTimer()
	b.Run("with_contextClose", func(b *testing.B) {
		b.ReportAllocs()
		manifest := Manifest{Wasm: []Wasm{WasmFile{Path: "wasm/count_vowels.wasm"}}}

		config := PluginConfig{
			EnableWasi:    true,
			ModuleConfig:  wazero.NewModuleConfig(),
			RuntimeConfig: wazero.NewRuntimeConfig().WithCloseOnContextDone(true),
		}

		plugin, err := NewPlugin(ctx, manifest, config, []HostFunction{})
		if err != nil {
			panic(err)
		}

		for i := 0; i < b.N; i++ {

			_, _, err = plugin.Call("count_vowels", []byte("Hello World!"))
			if err != nil {
				panic(err)
			}
		}
	})

	b.Run("normal", func(b *testing.B) {
		b.ReportAllocs()
		manifest := Manifest{Wasm: []Wasm{WasmFile{Path: "wasm/count_vowels.wasm"}}}

		config := PluginConfig{
			EnableWasi:    true,
			ModuleConfig:  wazero.NewModuleConfig(),
			RuntimeConfig: wazero.NewRuntimeConfig(),
		}

		plugin, err := NewPlugin(ctx, manifest, config, []HostFunction{})
		if err != nil {
			panic(err)
		}

		for i := 0; i < b.N; i++ {

			_, _, err = plugin.Call("count_vowels", []byte("Hello World!"))
			if err != nil {
				panic(err)
			}
		}
	})
}
go test -benchmem -run=^$ -bench ^BenchmarkCountVowels$
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkCountVowels/with_contextClose-12                  15078             77101 ns/op           58196 B/op        233 allocs/op
BenchmarkCountVowels/normal-12                             48499             24015 ns/op           48772 B/op         67 allocs/op
PASS
ok      github.com/extism/go-sdk        3.540s

It's 3.2X slower when counting vowels!

@nilslice
Copy link
Member

nilslice commented Dec 5, 2023

woah!

ok, @zshipko mentioned elsewhere that this could be possibly driven by the timeout set in the manifest. Is that the way this works now?

Sorry I'm out of the loop a bit on this.

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

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

LGTM!

@mhmd-azeez mhmd-azeez merged commit bef8f5f into main Dec 5, 2023
@mhmd-azeez mhmd-azeez deleted the abstract-wazero branch December 5, 2023 18:56
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.

2 participants