-
Notifications
You must be signed in to change notification settings - Fork 268
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
Optimize per-request work dispatching to wasm #3063
Optimize per-request work dispatching to wasm #3063
Conversation
This commit implements an optimization available from Wasmtime to improve the per-request dispatching to a wasm handler for an HTTP request. Previously the typed view of a request handler was loaded on each request but this involves somewhat expensive work such as: * A number of string lookups to find the right export. * A type-check to ensure that the component export has the right type. This can all be front-loaded to component-load time with `*Indices` structures that are generated by Wasmtime's `bindgen!` macro. This commit uses these generated structures to replace handler detection and then plumbs the final `*Indices` structure to dispatching the request so the dispatch doesn't need to perform extra work per-request. Signed-off-by: Alex Crichton <alex@alexcrichton.com>
.env("CARGO_TARGET_DIR", &out_dir) | ||
// If RUSTFLAGS was set it'll be passed to this build script through | ||
// this variable but since we're cross-compiling to wasm we almost | ||
// surely don't want this to get picked up, so remove this from the | ||
// destination flags. | ||
.env_remove("CARGO_ENCODED_RUSTFLAGS"); |
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'll note that this is technically unrelated to this change, but I've got [target.$my_host] rustflags = '...'
on my local machine which was causing the build of tests to fail-by-default without this.
_ => anyhow::bail!( | ||
"component exports multiple different handlers but \ | ||
it's expected to export only one" | ||
), |
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 wonder if we really want to do this. Thinking to the future, if someone wants to build a component that works with wasi:http 0.2 or 0.3 and exports both, shouldn't we just pick 0.3?
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 think that'd be reasonable yeah, AFAIK this logic is a historical holdover at this point but having a tiered "if you export this we call it first" seems reasonable to me
(I'm also not able to merge this if someone wouldn't mind merging for me) |
This commit implements an optimization available from Wasmtime to improve the per-request dispatching to a wasm handler for an HTTP request. Previously the typed view of a request handler was loaded on each request but this involves somewhat expensive work such as:
This can all be front-loaded to component-load time with
*Indices
structures that are generated by Wasmtime'sbindgen!
macro. This commit uses these generated structures to replace handler detection and then plumbs the final*Indices
structure to dispatching the request so the dispatch doesn't need to perform extra work per-request.