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

Optimize copy. #50

Merged
merged 3 commits into from
Jan 7, 2025
Merged

Conversation

sunfishcode
Copy link
Contributor

Optimize wstd::io::copy using the wasi-io splice function. To do this, add a way to query AsyncRead and AsyncWrite implementations for an underlying AsyncInputStream and AsyncOutputStream.

This is similar to optimizations done in std::io::copy, however those optimizations make use of specialization, which isn't available on stable Rust. That said, using explicit interfaces instead of specialization does have the advantage that user-defined types can opt into optimization if they're able to.

This also eliminates the RefCells in TcpStream.

Optimize `wstd::io::copy` using the wasi-io `splice` function. To do
this, add a way to query `AsyncRead` and `AsyncWrite` implementations
for an underlying `AsyncInputStream` and `AsyncOutputStream`.

This is similar to optimizations done in `std::io::copy`, however
those optimizations make use of specialization, which isn't available
on stable Rust. That said, using explicit interfaces instead of
specialization does have the advantage that user-defined types can
opt into optimization if they're able to.

This also eliminates the `RefCell`s in `TcpStream`.
@sunfishcode sunfishcode force-pushed the sunfishcode/optimize-copy branch from 843f966 to b7b5207 Compare January 6, 2025 17:59
@sunfishcode
Copy link
Contributor Author

sunfishcode commented Jan 6, 2025

The tcp_echo_server test is hanging on what looks like a bug in the tcp_echo_server program, which is currently depending on what appears to be a Wasmtime bug.

The current unoptimized copy code calls AsyncInputStream::read which does ready() followed by read(), and in the second iteration the ready() and read() returns 0 bytes, which appears to violate the "The pollable given by subscribe will be ready when more bytes are available." guarantee in the spec.

If that Wasmtime bug is fixed, then the tcp_echo_server test will start hanging. That looks like it should be fixed by calling shutdown on the write half after doing the write_all, which will then cause the server's copy to complete and the client's subsequent read_to_end to complete. That change would also fix the failure here.

@sunfishcode
Copy link
Contributor Author

This can be reproduced on trunk by adding this assert, checking the comment just above:

diff --git a/src/io/streams.rs b/src/io/streams.rs
index 63c084d..7173398 100644
--- a/src/io/streams.rs
+++ b/src/io/streams.rs
@@ -44,7 +44,10 @@ impl AsyncRead for AsyncInputStream {
             // WASI's `read` doesn't mean end-of-stream as it does in Rust,
             // however since we called `self.ready()`, we'll always get at
             // least one byte.
-            Ok(r) => r,
+            Ok(r) => {
+                assert!(!r.is_empty());
+                r
+            }
             // 0 bytes from Rust's `read` means end-of-stream.
             Err(StreamError::Closed) => return Ok(0),
             Err(StreamError::LastOperationFailed(err)) => {

The assert fails, even though the test still passes.

Shut down the writing end of the socket after writing, to allow the
server to end the stream, allowing a `read_to_end` on the reading end
to reach the end of the stream.
@sunfishcode sunfishcode force-pushed the sunfishcode/optimize-copy branch from 12c1e68 to fe74c8f Compare January 6, 2025 22:52
@sunfishcode
Copy link
Contributor Author

I've now reported the Wasmtime bug upstream in bytecodealliance/wasmtime#9938, and fixed the test to avoid depending on the Wasmtime bug.

@sunfishcode
Copy link
Contributor Author

With the test fixed, the CI here now passes.

@pchickey pchickey merged commit c276fdb into yoshuawuyts:main Jan 7, 2025
4 checks passed
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