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

Replace AsyncRead impl of Object with Stream of Bytes #1205

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Feb 2, 2024

This replaces the AsyncRead implementation of Object with a Stream<Item = io::Result<Bytes>>.

The reason for doing this breaking change is that because the AsyncRead impl is a wrapper of a Consumer (which in turn uses a Subscriber internally) the data already came in the form of a Bytes Stream, so it was the wrong abstraction to force the user to go through the naive AsyncRead implementation, at times probably needing to reconvert that back to a Stream because of many async crates preferring them to read/write style APIs.

This removes 2 memcpys from the stack (copying into the VecDeque and copying out of the VecDeque).

Users who need the AsyncRead implementation can use tokio_util::io::StreamReader, which has an even more efficient implementation than the nats.rs one by doing only one memcpy.

As a side note, this also replaces what I thought was a messy implementation (and in some places unexpected, like throwing an error if a second read had been launched after exhausting the stream, or the use of unwrap in creating the Consumer) with a more structured and idiomatic one.

@paolobarbolini paolobarbolini force-pushed the object-stream-read branch 4 times, most recently from 7a2dc77 to ed1bc50 Compare February 6, 2024 16:42
@Jarema
Copy link
Member

Jarema commented Feb 8, 2024

I like how the new way simplifies the codebase, and cleans it up. I just wonder if we should introduce such a hefty breaking change that will affect anyone using Object Store.

EDIT: Did you have an opportunity to bench it against the current solution?

@paolobarbolini
Copy link
Contributor Author

I haven't. I was mainly triggered by the AsyncReader 😃. I'll look into it.

@thomastaylor312
Copy link
Contributor

Just chiming in here to say that I'd love to see this land. Having it be a stream feels much easier (and idiomatic) to work with

@Jarema
Copy link
Member

Jarema commented Sep 4, 2024

Happy to revisit this one, but requires performance check.

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.

3 participants