-
Notifications
You must be signed in to change notification settings - Fork 17
feat(bytesbuf_io): add ConcurrentRead and ConcurrentWrite traits #292
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,6 +132,7 @@ HRESULT | |
| https | ||
| i.e. | ||
| impl | ||
| Implementors | ||
| impls | ||
| infeasible | ||
| inlined | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| use std::fmt::Debug; | ||
|
|
||
| use bytesbuf::mem::{HasMemory, Memory}; | ||
|
|
||
| use crate::Read; | ||
|
|
||
| /// Allows a [`Read`] source to support multiple concurrent read operations. | ||
| /// | ||
| /// The [`Read`] trait takes `&mut self`, limiting callers to one read at a time. | ||
| /// Some I/O endpoints can support many reads in flight simultaneously for higher throughput. | ||
| /// | ||
| /// Implementors return a new independent [`Read`] handle from each call to [`concurrently()`], | ||
| /// enabling the caller to drive any number of reads in parallel. | ||
| /// | ||
| /// [`concurrently()`]: ConcurrentRead::concurrently | ||
| pub trait ConcurrentRead: HasMemory + Memory + Debug { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is Debug a requirement here? That seems like an anti-pattern to me.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generic code without
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming feels strange to be. "Read" is a trait that has read methods on it, whereas this ConcurrentRead doesn't. I was expecting to see "concurrent read methods" (whatever that would be). This feels more like a clone-like thing. Like ReadClone and WriteClone?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cloning is intentionally not part of the picture - the thing being returned is not a clone, it is a new object that can be used to issue reads concurrently. Open to better names. In the same family of traits we might have "Deferral" capabilities. |
||
| /// The type of [`Read`] handle returned by [`concurrently()`](ConcurrentRead::concurrently). | ||
| type Handle: Read + Send + 'static; | ||
|
|
||
| /// Returns a new independent [`Read`] handle that can execute one concurrent read. | ||
| /// | ||
| /// Each handle operates independently - multiple handles may have reads in flight at the | ||
| /// same time without interfering with each other. | ||
| fn concurrently(&self) -> Self::Handle; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| use std::fmt::Debug; | ||
|
|
||
| use bytesbuf::mem::{HasMemory, Memory}; | ||
|
|
||
| use crate::Write; | ||
|
|
||
| /// Allows a [`Write`] sink to support multiple concurrent write operations. | ||
| /// | ||
| /// The [`Write`] trait takes `&mut self`, limiting callers to one write at a time. | ||
| /// Some I/O endpoints can support many writes in flight simultaneously for higher throughput. | ||
| /// | ||
| /// Implementors return a new independent [`Write`] handle from each call to [`concurrently()`], | ||
| /// enabling the caller to drive any number of writes in parallel. | ||
| /// | ||
| /// [`concurrently()`]: ConcurrentWrite::concurrently | ||
| pub trait ConcurrentWrite: HasMemory + Memory + Debug { | ||
| /// The type of [`Write`] handle returned by [`concurrently()`](ConcurrentWrite::concurrently). | ||
| type Handle: Write + Send + 'static; | ||
|
|
||
| /// Returns a new independent [`Write`] handle that can execute one concurrent write. | ||
| /// | ||
| /// Each handle operates independently - multiple handles may have writes in flight at the | ||
| /// same time without interfering with each other. | ||
| fn concurrently(&self) -> Self::Handle; | ||
| } |
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.
if the problem is that
Readusesmutreference, all we need is an extension for Read trait that allows to get another copy of this instanceit could look like this
the benefit is that you don't need
Handletype and the trait itself acts like an extension forReadtypesThere 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.
This alternative formulation feels more limiting. Why should we restrict implementations to only returning
Self? The current form feels more flexible at no real cost. Or am I missing some hidden cost/complexity?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.
Handletype adds some level of complexity, probably it will look worse in method's signaturesalternative
and also the name is
ConcurrentRead, but it doesn't really have any read methods, so it's more likeConcurrentReadProvideror somethingThere 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.
Wouldn't just making the
ReadimplemnetCloneessentially making it concurrent?This way, you don't even need to introduce new trait anymore.
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.
This also has the limitation that it forces each read to go through a
Self. This seems like an unnecessary limitation.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.
Agreed but that "provider" style felt too verbose. Do you have shorter alternative name suggestions? Concurrent is a long word :)
Uh oh!
There was an error while loading. Please reload this page.
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.
Why is this a problem?
edit: If i read the API properly, you use
concurrentmethod to retrieve the handle. You could do the same thing by cloning the type that implementsReadtrait and continue using it on other scope.To me, the flow is almost identical.
Uh oh!
There was an error while loading. Please reload this page.
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.
There is no guarantee that the type that implements
Readcan be cloned. The concept of "clone" is bigger than "concurrent reads" - we should not impose big-picture requirements like "must support cloning" just to enable concurrent reads.I agree the flow is the same - but this is not a question of what the code to use the type looks like, this is a question of what requirements we impose to support concurrent access. Being able to clone
Selffeels like a suspiciously large requirement because it affects more than just the "reading" API surface.That is not to say that I am completely against it but for a general-purpose API it does not feel like a conservative choice that favors stable long-term APIs.
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.
what about
ReadDemux/ReadDemultiplexername?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.
Demux/Demultiplexerimplies something completely different in my view (e.g. that the input is multiplexed, which it is not), does not feel appropriate as a name for this abstraction.