-
Notifications
You must be signed in to change notification settings - Fork 12
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
internal/wasmtools: run wasm-tools.wasm in embedded Wazero runtime #252
Conversation
Makefile
Outdated
@@ -5,8 +5,8 @@ wit_files = $(sort $(shell find testdata -name '*.wit' ! -name '*.golden.*')) | |||
json: $(wit_files) | |||
|
|||
.PHONY: $(wit_files) | |||
$(wit_files): | |||
wasm-tools component wit -j --all-features $@ > [email protected] | |||
$(wit_files): wit/wasm-tools.wasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put the Wazero and wasm-tools dependency into internal/ or x/.
As it stands, this creates a new problem where consumers of package cm will have an implicit dependency on Wazero. We need to figure out a way around this.
internal/wasmtools/wasmtools.go
Outdated
|
||
// Executor is an interface for running Wasm modules. | ||
type Executor interface { | ||
Run(ctx context.Context, args []string, stdin io.Reader, fsMap map[string]string) (stdout *bytes.Buffer, stderr *bytes.Buffer, err error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be an fs.FS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to the fsMap? yes it can be defined as map[fs.FS]string
} | ||
config = config.WithFSConfig(fsConfig) | ||
|
||
_, err = w.runtime.InstantiateModule(ctx, w.module, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this run in a goroutine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. I've added a 10s timeout to it so that it won't run forever.
WithSysWalltime(). | ||
WithArgs(append([]string{"wasm-tools.wasm"}, args...)...) | ||
if name != nil { | ||
config = config.WithName(*name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the name
arg necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the name? Does it matter to the caller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to run wasm-tools.wasm multiple times, the Run()
would fail without the name parameter because the wazero module configuration defaults the "name" to what was decoded from the name section and duplicate names are not allowed in a single Runtime.
internal/wasmtools/wasmtools.go
Outdated
return w.runtime.Close(ctx) | ||
} | ||
|
||
func (w *WasmTools) Run(ctx context.Context, args []string, stdin io.Reader, fsMap map[string]string, name *string) (stdout *bytes.Buffer, stderr *bytes.Buffer, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of fsMap map[string]string
, how about a varadic dirs ...string
at the end of the argument list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not want to distinguish host and guest paths?
If we use dirs ...string
we will have to set the guest path as the same as the host path. i.e. fsConfig.WithDirMount(dir, dir)
I think we’ll need to add build tags to disable this functionality when run under TinyGo (e.g. have |
13978f5
to
c41c661
Compare
@Mossaka can you rebase and squash some commits? |
dc61352
to
aba1c33
Compare
* add wasm-tools as git submodule * update Makefile to compile wit/wasm-tools.wasm * use wazero to execute wasm-tools.wasm * Makefile: add a build command * Commit the wasm-tools.wasm file * Makefile: replace path with a variable for the name of the target * wit, Makefile: move wasmtools.go to internal/wasmtools * Remove wasmtools submodule Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Adding lto, opt-level=z and other options to the cargo.toml reduces the wasm-tools.wasm binary size from 7.8 MBi to 3.5 MBi Signed-off-by: Jiaxiao Zhou (Mossaka) <[email protected]>
Signed-off-by: Jiaxiao Zhou <[email protected]>
…map[fs.FS]string Signed-off-by: Jiaxiao Zhou <[email protected]>
Signed-off-by: Jiaxiao Zhou <[email protected]>
…ve naming * Rename to WasmTools to Instance * Add build tags to disable wasmtools when run under TinyGo * wasmtools_tinygo: New returns a new error * Rename Executor to Runner Signed-off-by: Jiaxiao Zhou <[email protected]>
… in TinyGo Signed-off-by: Jiaxiao Zhou <[email protected]>
aba1c33
to
ff76f42
Compare
…nce API Signed-off-by: Jiaxiao Zhou <[email protected]>
Signed-off-by: Jiaxiao Zhou <[email protected]>
Added more docs and updated CHANGELOG |
…m and update Makefile This adds gzip compression for the wasm-tools.wasm to further reduce the size to 1.2M from 3.5M. Added the wasm file to the .gitignore and thus the remote repo will only have the gzip file. Updated Makefile to zip and unzip the Wasm module. Signed-off-by: Jiaxiao Zhou <[email protected]>
Signed-off-by: Jiaxiao Zhou <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the propose of the gzip code? Groundwork for gzipping the binary output?
@@ -32,6 +32,9 @@ jobs: | |||
with: | |||
go-version-file: go.mod | |||
|
|||
- name: Set up wams-tools.wasm | |||
run: make internal/wasmtools/wasm-tools.wasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to do this?
The artifact should already be committed, and this adds a Rust dependency / build step to the release workflow.
) | ||
|
||
//go:embed wasm-tools.wasm | ||
var wasmTools []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be the gz file?
@@ -147,6 +155,9 @@ jobs: | |||
with: | |||
version: ${{ env.wasm-tools-version }} | |||
|
|||
- name: Set up wams-tools.wasm | |||
run: make internal/wasmtools/wasm-tools.wasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
Should we delete the step above?
@@ -61,6 +66,9 @@ jobs: | |||
with: | |||
version: ${{ env.wasm-tools-version }} | |||
|
|||
- name: Set up wams-tools.wasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary? Delete this and the step above?
Had an idea: |
Implemented in #272 |
Close for #272 |
what I did to optmize the wasm-tools Wasm binary size
The current wasm-tools.wasm size is 3.5M
"component"
featureLet me know if you know more ways to cut down the size of the binary!