make it easy to consume a DmaStreamReader without copying
#474
Conversation
HippoBaro
force-pushed
the
better_stream_reader
branch
from
d53526b
to
cd12a8c
Compare
|
Reviews are welcome but let's not merge this right now: I want to give this a spin on some real workload first :) |
|
This code has been deployed for a little while now, and it seems to be working as expected. We have streams that go over every variable-sized item in a file. Their throughput has doubled, primarily because calls to All in all pretty happy with the result! |
glommio/src/io/dma_file_stream.rs
Outdated
| /// [`DmaStreamReaderBuilder`]) and act accordingly. | ||
| /// extra copy. | ||
| /// | ||
| /// This function returns a tuple of a [`ReadResult`] and a [`usize`]. The |
There was a problem hiding this comment.
I am a fan of the approach. Well done.
Not sure I am a fan of the unnamed tuple. Why do you think this is preferable to a specialized struct ?
There was a problem hiding this comment.
Right, I agree.
Come to think of it, this API should work like POSIX read, i.e., it should return what was read instead of what is left to read. What is left to read is actually very hard to know, given we could hit EOF at any time.
If I return what was read instead, then I don't need to return a tuple because, by definition, you can already get that with ReadResult::len(). That will simplify things even more. Nice!
HippoBaro
force-pushed
the
better_stream_reader
branch
from
cd12a8c
to
fb0cddd
Compare
The `get_buffer_aligned` API could be better. It relies on user code to be aware of buffer boundaries locations. This makes user code more complicated and brittle because the buffer size will generally be a configurable knob, subject to change. Fear not, however, for there is a better way! Instead of failing when reading too much in `get_buffer_aligned,` read whatever we can from the current buffer. This allows the following: * If the read fits inside a buffer, the user can consume the result directly without any copy; * If the read crosses buffer boundaries and the user can consume partial results, it can do so in a loop without any copies; * If the read crosses buffer boundaries and the user needs a complete result, `get_buffer_aligned` may be used trivially in a loop that concatenates the results in a reusable user-allocated buffer. The following commit will replace the `AsyncRead::poll_read` implementation with a simple loop calling `get_buffer_aligned.`
We need to make sure the stream can't return any bytes beyond the `max_pos` offset. Right now, `poll_read` respects this while `poll_get_buffer_aligned` doesn't. This commit makes sure we are consistent across the two.
Massive simplification; Oh Yeah!!
HippoBaro
force-pushed
the
better_stream_reader
branch
from
fb0cddd
to
93dfc17
Compare
|
An additional change I am considering making would be to change the name of the two functions We could even create two traits: |
|
We can add the traits, but the biggest advantage of having traits in this case is if there are other implementations. So probably best to wait until the need shows up |
The
get_buffer_alignedAPI could be better. It relies on user code to beaware of buffer boundaries locations. This makes user code more
complicated and brittle because the buffer size will generally be a
configurable knob, subject to change.
Fear not, however, for there is a better way!
Instead of failing when reading too much in
get_buffer_aligned,readwhatever we can from the current buffer and return the number of
remaining bytes. This allows the following:
directly without any copy;
results, it can do so in a loop without any copies;
result,
get_buffer_alignedmay be used trivially in a loop thatconcatenates the results in a reusable user-allocated buffer.
This PR also replaces the ad-hoc
poll_readimplementation for a proxycall to
get_buffer_aligned, resulting in a very nice reduction in complexity.