wasi-http: Remove extra task when reading bodies #7475
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit is in the same spirit as bytecodealliance#7461 and is aimed at removing an extra task when interoperating between WebAssembly and Hyper. The intention here is to ensure that when data is ready immediately a context switch between Tokio tasks isn't necessary. I'm still not sure why this context switch would tank performance so much. The goal of this commit is to remove the Tokio task associated with reading bodies. Currently a task is spawned with two channels coming out: one for frames and one for trailers. Instead the body is now read directly when data frames are read and trailers are managed similarly. The interactions here are somewhat nontrivial due to the management of resources and how the body needs to jump from the body stream object to the trailers object, but it's overall not the worst thing in the world. Locally on Linux this takes a "hello world" component which starts out by reading its HTTP body and then responds with a string from 6.7k rps locally via `wasmtime serve` to 16.8k rps. This restores the performance of wasi-http to be in-line with the historical SDK that Fermyon offered, for example, that's based on a custom HTTP interface. Note that these performance numbers use bytecodealliance#7461 as a baseline. This commit has a downside, however, that if the incoming body is not being subscribed to actively by the wasm then no activity will happen on the host. This is a function of `poll` isn't being actively watched unless `subscribe`'s result is being actively watched via a wasm call to `wasi:io/poll`. Manual investigation of Hyper shows that this is probably ok for now since bodies are primarily channel-based which seems like the tasks doing the actual I/O are managed elsewhere. In that sense this shouldn't cause any immediate impact, but Hyper's situation in the future could change or my analysis could additionally be wrong. Given the performance benefits here though I'd be tempted to push in favor of this commit and handle "poll in the background" as necessary in the future if needed.
alexcrichton
commented
Nov 3, 2023
| Ok(()) => {} | ||
| // TODO: same as above, shouldn't throw this error away. | ||
| Err(e) => tracing::warn!("dropping error {e}"), | ||
| } |
There was a problem hiding this comment.
I'll note that this, and the above, are somewhat unrelated but also tangential changes since I was touching these types. The above task previously dropped the error and this task tried to forward it along. In both cases though the result of this task was never actually looked at as the handle was exclusively used to keep the task alive and that's it.
I've updated this so both tasks consistently drop the error. I don't know how to communicate this error through the WASI APIs myself.
pchickey
approved these changes
Nov 3, 2023
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Nov 6, 2023
This commit is in the same spirit as bytecodealliance#7461 and is aimed at removing an extra task when interoperating between WebAssembly and Hyper. The intention here is to ensure that when data is ready immediately a context switch between Tokio tasks isn't necessary. I'm still not sure why this context switch would tank performance so much. The goal of this commit is to remove the Tokio task associated with reading bodies. Currently a task is spawned with two channels coming out: one for frames and one for trailers. Instead the body is now read directly when data frames are read and trailers are managed similarly. The interactions here are somewhat nontrivial due to the management of resources and how the body needs to jump from the body stream object to the trailers object, but it's overall not the worst thing in the world. Locally on Linux this takes a "hello world" component which starts out by reading its HTTP body and then responds with a string from 6.7k rps locally via `wasmtime serve` to 16.8k rps. This restores the performance of wasi-http to be in-line with the historical SDK that Fermyon offered, for example, that's based on a custom HTTP interface. Note that these performance numbers use bytecodealliance#7461 as a baseline. This commit has a downside, however, that if the incoming body is not being subscribed to actively by the wasm then no activity will happen on the host. This is a function of `poll` isn't being actively watched unless `subscribe`'s result is being actively watched via a wasm call to `wasi:io/poll`. Manual investigation of Hyper shows that this is probably ok for now since bodies are primarily channel-based which seems like the tasks doing the actual I/O are managed elsewhere. In that sense this shouldn't cause any immediate impact, but Hyper's situation in the future could change or my analysis could additionally be wrong. Given the performance benefits here though I'd be tempted to push in favor of this commit and handle "poll in the background" as necessary in the future if needed.
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Nov 6, 2023
This commit is in the same spirit as bytecodealliance#7461 and is aimed at removing an extra task when interoperating between WebAssembly and Hyper. The intention here is to ensure that when data is ready immediately a context switch between Tokio tasks isn't necessary. I'm still not sure why this context switch would tank performance so much. The goal of this commit is to remove the Tokio task associated with reading bodies. Currently a task is spawned with two channels coming out: one for frames and one for trailers. Instead the body is now read directly when data frames are read and trailers are managed similarly. The interactions here are somewhat nontrivial due to the management of resources and how the body needs to jump from the body stream object to the trailers object, but it's overall not the worst thing in the world. Locally on Linux this takes a "hello world" component which starts out by reading its HTTP body and then responds with a string from 6.7k rps locally via `wasmtime serve` to 16.8k rps. This restores the performance of wasi-http to be in-line with the historical SDK that Fermyon offered, for example, that's based on a custom HTTP interface. Note that these performance numbers use bytecodealliance#7461 as a baseline. This commit has a downside, however, that if the incoming body is not being subscribed to actively by the wasm then no activity will happen on the host. This is a function of `poll` isn't being actively watched unless `subscribe`'s result is being actively watched via a wasm call to `wasi:io/poll`. Manual investigation of Hyper shows that this is probably ok for now since bodies are primarily channel-based which seems like the tasks doing the actual I/O are managed elsewhere. In that sense this shouldn't cause any immediate impact, but Hyper's situation in the future could change or my analysis could additionally be wrong. Given the performance benefits here though I'd be tempted to push in favor of this commit and handle "poll in the background" as necessary in the future if needed. Signed-off-by: Alex Crichton <alex@alexcrichton.com>
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Nov 8, 2023
This commit is in the same spirit as bytecodealliance#7461 and is aimed at removing an extra task when interoperating between WebAssembly and Hyper. The intention here is to ensure that when data is ready immediately a context switch between Tokio tasks isn't necessary. I'm still not sure why this context switch would tank performance so much. The goal of this commit is to remove the Tokio task associated with reading bodies. Currently a task is spawned with two channels coming out: one for frames and one for trailers. Instead the body is now read directly when data frames are read and trailers are managed similarly. The interactions here are somewhat nontrivial due to the management of resources and how the body needs to jump from the body stream object to the trailers object, but it's overall not the worst thing in the world. Locally on Linux this takes a "hello world" component which starts out by reading its HTTP body and then responds with a string from 6.7k rps locally via `wasmtime serve` to 16.8k rps. This restores the performance of wasi-http to be in-line with the historical SDK that Fermyon offered, for example, that's based on a custom HTTP interface. Note that these performance numbers use bytecodealliance#7461 as a baseline. This commit has a downside, however, that if the incoming body is not being subscribed to actively by the wasm then no activity will happen on the host. This is a function of `poll` isn't being actively watched unless `subscribe`'s result is being actively watched via a wasm call to `wasi:io/poll`. Manual investigation of Hyper shows that this is probably ok for now since bodies are primarily channel-based which seems like the tasks doing the actual I/O are managed elsewhere. In that sense this shouldn't cause any immediate impact, but Hyper's situation in the future could change or my analysis could additionally be wrong. Given the performance benefits here though I'd be tempted to push in favor of this commit and handle "poll in the background" as necessary in the future if needed. Signed-off-by: Alex Crichton <alex@alexcrichton.com>
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit is in the same spirit as #7461 and is aimed at removing an extra task when interoperating between WebAssembly and Hyper. The intention here is to ensure that when data is ready immediately a context switch between Tokio tasks isn't necessary. I'm still not sure why this context switch would tank performance so much.
The goal of this commit is to remove the Tokio task associated with reading bodies. Currently a task is spawned with two channels coming out: one for frames and one for trailers. Instead the body is now read directly when data frames are read and trailers are managed similarly. The interactions here are somewhat nontrivial due to the management of resources and how the body needs to jump from the body stream object to the trailers object, but it's overall not the worst thing in the world.
Locally on Linux this takes a "hello world" component which starts out by reading its HTTP body and then responds with a string from 6.7k rps locally via
wasmtime serveto 16.8k rps. This restores the performance of wasi-http to be in-line with the historical SDK that Fermyon offered, for example, that's based on a custom HTTP interface. Note that these performance numbers use #7461 as a baseline.This commit has a downside, however, that if the incoming body is not being subscribed to actively by the wasm then no activity will happen on the host. This is a function of
pollisn't being actively watched unlesssubscribe's result is being actively watched via a wasm call towasi:io/poll. Manual investigation of Hyper shows that this is probably ok for now since bodies are primarily channel-based which seems like the tasks doing the actual I/O are managed elsewhere. In that sense this shouldn't cause any immediate impact, but Hyper's situation in the future could change or my analysis could additionally be wrong. Given the performance benefits here though I'd be tempted to push in favor of this commit and handle "poll in the background" as necessary in the future if needed.