async local io #1115

rklaehn · 2023-06-20T12:02:07Z

Async local io

this does not really make too much sense right now, but will become part of the workspace once the iroh crate split is done.

rklaehn · 2023-06-20T14:04:08Z

@Arqu any idea about these failures?

error: failed to run custom build command for openssl-sys v0.9.88

No idea why it is trying to build openssl-sys in the first place. openssl-sys is not a dependency. I guess these have to be installed on the runner?

Arqu · 2023-06-20T14:19:25Z

Cargo.lock

+ "lazy_static",
+ "libc",
+ "log",
+ "openssl",


This is pulling it in

Arqu · 2023-06-20T14:20:24Z

Cargo.lock

@@ -2799,10 +2956,12 @@ dependencies = [
 "http-body",
 "hyper",
 "hyper-rustls",
+ "hyper-tls",


I'd check we don't accidentally pull in hyper-tls (think it's on be default and needs to be excluded or something like that)

rklaehn · 2023-06-21T07:57:43Z

This is already published. Here are the docs: https://docs.rs/iroh-io/0.1.0/iroh_io/

dignifiedquire · 2023-06-21T09:17:57Z

iroh-io/Cargo.toml

+proptest = "1"
+tempfile = "3"
+tokio = { version = "1", features = ["rt", "test-util", "macros"] }
+warp = { version = "0.3.5", default-features = false }


Would be great to switch this to axum, given that is already in our dependency tree for the derper. (as a follow up)

yes, on my todo list, especially given that warp does http range requests wrong.

iroh-io/README.md

dignifiedquire

Looks good overall. I haven't reviewed the details of the impl, just general structure.

dignifiedquire · 2023-06-21T09:23:26Z

iroh-io/src/lib.rs

+        &mut self,
+        offset: u64,
+        data: [u8; N],
+    ) -> Self::WriteArrayAtFuture<'_>;


Could be nice to provide a default impl that just fallsback to use write_at

😢 default impls would require defaults for associated types, which are unstable

dignifiedquire · 2023-06-21T09:25:03Z

iroh-io/src/lib.rs

+    //     let buf = resource.read_at(1000000, 100).await?;
+    //     println!("buf: {:?}", buf);
+    //     Ok(())
+    // }


what's up with this test?

It's just a thing to explore the behaviour of one particular http server that supports range requests. Since it is on IPFS I don't want to keep it enabled. I guess I could just disable it instead of commenting it out.

dignifiedquire · 2023-06-21T09:26:05Z

iroh-io/src/mem.rs

+use futures::future;
+use std::io;
+
+impl AsyncSliceReader for bytes::Bytes {


Do you plan on adding impls for Vec<u8> and std::io::Cursor potentially? might be quite nice

Yeah, for Vec it would basically be the same. Cursor I am not sure about since you don't have the notion of a "current position".

it would also be good to test that this interface works with https://docs.rs/mmap-rs (and potentially adding a cfged impl to this code) as this would likely be a nice speedup in a bunch of cases.

just a thought though

Oh boy, that is a can of worms.

So - it is definitely possible to do an impl of this that uses memory mapping. There is a big but though: since there is no custom constructor for Bytes you would have to copy from the memory mapped region into the Bytes, even if you could ensure that the Bytes will keep the memory mapped buffer alive.

I have opened an issue on bytes::Bytes for this, and so have other people. But so far no success. For other projects I have therefore decided to write my own Bytes equivalent type. But for this crate I decided to go with Bytes since it is ubiquitous and the Bytes/BytesMut combo is quite helpful when doing zero alloc IO (splitting off Bytes from a BytesMut etc) and I did not feel like reimplementing all of this.

This was my issue:
tokio-rs/bytes#571

The other issues:
tokio-rs/bytes#437
tokio-rs/bytes#526

My version of Bytes:
https://github.com/cloudpeers/radixdb/blob/master/src/store/blob_store.rs#L37

By the way: what is the current crate for memory mapped IO?

There is memmap, memmap2, and now this mmap-rs

I thought memmap2 was the current one... 🤷‍♂️

This is memmap2 download stats. It certainly seems to be popular...

dignifiedquire · 2023-06-21T09:27:49Z

iroh-io/src/tokio_io.rs

+                        **h = Some(state);
+                        r
+                    }
+                    Err(_) => Err(io::Error::new(


would probably be helpful to at least include the error in the message?

rklaehn · 2023-06-21T09:35:38Z

Looks good overall. I haven't reviewed the details of the impl, just general structure.

The impls are as simple as possible. There is an entire big can of worms called buffering that I will need to add. But not in this PR

also make sure the inner error is propagated when unwrapping a JoinHandle

* feat: add iroh io * docs: add badges and license information to iroh-io also make sure the inner error is propagated when unwrapping a JoinHandle

rklaehn force-pushed the iroh-io branch from 49b5c3c to 5a50a21 Compare June 20, 2023 12:03

rklaehn changed the base branch from main to refactor-crates June 20, 2023 12:17

rklaehn force-pushed the iroh-io branch 2 times, most recently from 35d8913 to 3f52a5b Compare June 20, 2023 13:08

rklaehn marked this pull request as ready for review June 20, 2023 13:09

Arqu reviewed Jun 20, 2023

View reviewed changes

Cargo.lock Outdated

"lazy_static",

"libc",

"log",

"openssl",

Copy link

Collaborator

Arqu Jun 20, 2023

Choose a reason for hiding this comment

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

This is pulling it in

Arqu reviewed Jun 20, 2023

View reviewed changes

rklaehn force-pushed the iroh-io branch 3 times, most recently from e0ee56f to 5edef76 Compare June 20, 2023 14:48

dignifiedquire force-pushed the refactor-crates branch from 9450247 to 9cfe650 Compare June 21, 2023 08:47

rklaehn force-pushed the iroh-io branch from 236b024 to 7886305 Compare June 21, 2023 09:01

dignifiedquire reviewed Jun 21, 2023

View reviewed changes

iroh-io/README.md Outdated Show resolved Hide resolved

feat: add iroh io

15081d7

rklaehn force-pushed the iroh-io branch from 7886305 to 15081d7 Compare June 21, 2023 09:20

dignifiedquire approved these changes Jun 21, 2023

View reviewed changes

docs: add badges and license information to iroh-io

8980dfa

also make sure the inner error is propagated when unwrapping a JoinHandle

rklaehn force-pushed the iroh-io branch from 6c933f3 to 8980dfa Compare June 21, 2023 09:49

rklaehn merged commit 23ea777 into refactor-crates Jun 21, 2023

rklaehn deleted the iroh-io branch June 21, 2023 10:17

matheus23 pushed a commit that referenced this pull request Nov 14, 2024

async local io (#1115)

b1cac48

* feat: add iroh io * docs: add badges and license information to iroh-io also make sure the inner error is propagated when unwrapping a JoinHandle

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async local io #1115

async local io #1115

rklaehn commented Jun 20, 2023

rklaehn commented Jun 20, 2023

Arqu Jun 20, 2023

Arqu Jun 20, 2023

rklaehn commented Jun 21, 2023

dignifiedquire Jun 21, 2023

rklaehn Jun 21, 2023

dignifiedquire left a comment

dignifiedquire Jun 21, 2023

rklaehn Jun 21, 2023

dignifiedquire Jun 21, 2023

dignifiedquire Jun 21, 2023

rklaehn Jun 21, 2023

dignifiedquire Jun 21, 2023

rklaehn Jun 21, 2023

dignifiedquire Jun 21, 2023

dignifiedquire Jun 21, 2023

rklaehn Jun 21, 2023

rklaehn Jun 21, 2023

rklaehn Jun 21, 2023

dignifiedquire Jun 21, 2023

rklaehn commented Jun 21, 2023

async local io #1115

async local io #1115

Conversation

rklaehn commented Jun 20, 2023

rklaehn commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rklaehn commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rklaehn commented Jun 21, 2023