feat(bytesbuf_io): add ConcurrentRead and ConcurrentWrite traits#292
feat(bytesbuf_io): add ConcurrentRead and ConcurrentWrite traits#292sandersaares wants to merge 3 commits intomainfrom
Conversation
Add ConcurrentRead and ConcurrentWrite traits to support concurrent I/O operations on types that implement Read/Write traits. Unlike Read/Write which take &mut self (limiting to one operation at a time), these traits take &self and return independent Handle types, enabling callers to drive multiple reads or writes in parallel.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 144 144
Lines 8917 8917
=======================================
Hits 8917 8917 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// enabling the caller to drive any number of reads in parallel. | ||
| /// | ||
| /// [`concurrently()`]: ConcurrentRead::concurrently | ||
| pub trait ConcurrentRead: HasMemory + Memory + Debug { |
There was a problem hiding this comment.
if the problem is that Read uses mut reference, all we need is an extension for Read trait that allows to get another copy of this instance
it could look like this
pub trait ConcurentRead : Read {
fn duplicate(&self) -> Self;
}the benefit is that you don't need Handle type and the trait itself acts like an extension for Read types
There was a problem hiding this comment.
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.
Handle type adds some level of complexity, probably it will look worse in method's signatures
fn open_concurrent_read<P, R>() -> P
where
P: ConcurrentRead<Handle: R>
R: Read + Send + 'static,
{
//...
}alternative
fn open_concurrent_read<R>() -> R
where
R: ConcurrentRead
{
//...
}and also the name is ConcurrentRead, but it doesn't really have any read methods, so it's more like ConcurrentReadProvider or something
There was a problem hiding this comment.
Wouldn't just making the Read implemnet Clone essentially making it concurrent?
This way, you don't even need to introduce new trait anymore.
There was a problem hiding this comment.
Wouldn't just making the Read implemnet Clone essentially making it concurrent?
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.
and also the name is ConcurrentRead, but it doesn't really have any read methods, so it's more like ConcurrentReadProvider or something
Agreed but that "provider" style felt too verbose. Do you have shorter alternative name suggestions? Concurrent is a long word :)
There was a problem hiding this comment.
This also has the limitation that it forces each read to go through a Self. This seems like an unnecessary limitation.
Why is this a problem?
edit: If i read the API properly, you use concurrent method to retrieve the handle. You could do the same thing by cloning the type that implements Read trait and continue using it on other scope.
To me, the flow is almost identical.
There was a problem hiding this comment.
You could do the same thing by cloning the type that implements Read trait and continue using it on other scope.
There is no guarantee that the type that implements Read can 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 Self feels 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.
what about ReadDemux/ReadDemultiplexer name?
There was a problem hiding this comment.
Demux/Demultiplexer implies 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.
| /// enabling the caller to drive any number of reads in parallel. | ||
| /// | ||
| /// [`concurrently()`]: ConcurrentRead::concurrently | ||
| pub trait ConcurrentRead: HasMemory + Memory + Debug { |
There was a problem hiding this comment.
Why is Debug a requirement here? That seems like an anti-pattern to me.
There was a problem hiding this comment.
Generic code without Debug requirements gets annoying because you cannot debug-print stuff even though 99% of things implement Debug. I feel we can expect all but the most private types to always implement Debug just to make the dev UX easier.
| /// enabling the caller to drive any number of reads in parallel. | ||
| /// | ||
| /// [`concurrently()`]: ConcurrentRead::concurrently | ||
| pub trait ConcurrentRead: HasMemory + Memory + Debug { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. ReadDefer and ReadConcurrent maybe?
|
Going on vacation, so marking as draft - will continue processing the topic when back. |
Summary
Add
ConcurrentReadandConcurrentWritetraits tobytesbuf_ioto support concurrent I/O operations.Motivation
The existing
ReadandWritetraits take&mut self, limiting callers to one operation at a time. Some I/O endpoints (e.g. those backed by completion-based I/O models) can support many operations in flight simultaneously for higher throughput.Changes
ConcurrentReadtrait - takes&selfand returns an independentReadhandle viaconcurrently(), enabling multiple concurrent readsConcurrentWritetrait - takes&selfand returns an independentWritehandle viaconcurrently(), enabling multiple concurrent writesRead/Write(HasMemory + Memory + Debug)Implementorsto the spelling dictionary