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

Add buffered I/O #8953

Closed
brson opened this issue Sep 3, 2013 · 6 comments
Closed

Add buffered I/O #8953

brson opened this issue Sep 3, 2013 · 6 comments

Comments

@brson
Copy link
Contributor

brson commented Sep 3, 2013

libuv provides unbuffered I/O. We also need buffered decorators for readers and writers.

@chris-morgan
Copy link
Member

I certainly felt the need of this with rust-http, where code like writing an HTTP header to a socket with write() calls for name, ": ", value, and "\r\n" would take four TCP packets rather than probably a small fraction of one. Hence (in part) my current buffering arrangement in rust-http.

I think the difficulty here is handling reading and writing. As it happens, I'd rather like to be able to split a socket up into a reader and a writer, but that's out of scope here. I think, though, that we'd need three decorators; one for Reader, one for Writer and one for Stream—the last being basically the first two glued together.

I switched from my own BufferedReader and BufferedWriter to having a unified BufferedStream (the former two using borrowing on account of needing to read and write) when that approach became infeasible. Here, a BufferedReader and BufferedWriter would, I presume, own the Reader or Writer.

My BufferedStream code is available at https://github.com/chris-morgan/rust-http/blob/f78236284b73ba5e980962b1ffbd71ea9a03fbbc/src/libhttp/buffer.rs

@bluss
Copy link
Member

bluss commented Sep 5, 2013

CM, do you need to implement BufferedStream separately, you can just let the Writer trait pass through the reader buffer?

pub struct ReadBuffer<R> {
    priv reader: R,
    priv buffer: ~[u8],
    priv lo: uint,
    priv hi: uint,
}

impl<R> Decorator<R> for ReadBuffer<R> {
    fn inner(self) -> R { self.reader }
    fn inner_ref<'a>(&'a self) -> &'a R { &self.reader }
    fn inner_mut_ref<'a>(&'a mut self) -> &'a mut R { &mut self.reader }
}

impl<W: Writer> Writer for ReadBuffer<W> {
    fn write(&mut self, buf: &[u8]) {
        self.inner_mut_ref().write(buf)
    }
    fn flush(&mut self) {
        self.inner_mut_ref().flush()
    }
}

@chris-morgan
Copy link
Member

@blake2-ppc BufferedStream must buffer both reading and writing. Still, what you suggest sounds a good way of achieving buffering of just reading or just writing.

@bluss
Copy link
Member

bluss commented Sep 5, 2013

I mean that you can stack the BufferedWriter on top of the BufferedReader this way, without having to implement BufferedStream separately.

@olsonjeffery
Copy link
Contributor

@chris-morgan +1 I think it's great that you already have this code set aside and I think it's worth getting into the rust tree as-is (to relieve you of the burden of carrying it in rust-http, if anything).

With that being said, while looking this over something occurred to me: is it worth considering that we break Reader & and Writer into buffered/unbuffered equivalents and that this buffered implementation, as Chris presents it, would be pushed into uvio and exposed by an Rtio trait?

If we look forward to platform-backed implementations for everything in rt::io (that is, bsd sockets or win32 instead of libuv for everything in rt::io::net & the equiv. per-platform APIs for file, timer, etc), we could imagine (in the world of files, at least) a BufferedStream being implemented using a FILE* handle + fread(3) et al, while an UnbufferedStream uses a file descriptor + pread(2) and friends.

The drawback to this approach, of course, is that the buffering code is duplicated in multiple places in uvio or anywhere that doesn't have a platform-level buffering API. In this world, we would have BufferedTcpStream, BufferedFileStream, etc as user-facing constructs, with backing Rtio traits. Chris's BufferedStream would be re-used internally within uvio (really, it's generally applicable for any underlying implementation that doesn't have a platform-level buffered IO api for the thing we're mapping).

This may be worthy of breaking out into a separate ticket, and there's certainly an argument against what I'm proposing (as Chris has presented this solution, a BufferedStream can wrap anything that impls Reader+Writer or Stream).

And based on my current read of rt::io (someone correct me if I'm mistaken), we would lose the ability to expose both fread and pread in a platform-backed implementation of rt::io::file. Not sure how important that loss is, but what I suggest above is a legitimate way to address it.

@chris-morgan
Copy link
Member

@blake2-ppc yep, after this and discussion in IRC I get what's being meant. Yes, that would work, with type BufferedStream<T> = BufferedReader<BufferedWriter<T>>. The exception is if we need any BufferedStream::*; then that can't be done, to the best of my knowledge.

bors added a commit that referenced this issue Sep 11, 2013
The default buffer size is the same as the one in Java's BufferedWriter.

We may want BufferedWriter to have a Drop impl that flushes, but that
isn't possible right now due to #4252/#4430. This would be a bit
awkward due to the possibility of the inner flush failing. For what it's
worth, Java's BufferedReader doesn't have a flushing finalizer, but that
may just be because Java's finalizer support is awful.

The current implementation of BufferedStream is weird in my opinion, but
it's what the discussion in #8953 settled on.

I wrote a custom copy function since vec::copy_from doesn't optimize as
well as I would like.

Closes #8953
@bors bors closed this as completed in 71f0305 Sep 11, 2013
flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 30, 2022
…, r=Manishearth

add vec.capacity() to [`slow_vec_initialization`] detection

fix rust-lang#8800

for example
```rust
let mut vec1 = Vec::with_capacity(len);
vec1.resize(vec1.capacity(), 0);

let mut vec2 = Vec::with_capacity(len);
vec2.extend(repeat(0).take(vec2.capacity()));
```
will trigger the lint

---

changelog: add `vec.capacity()` to [`slow_vec_initialization`] detection
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 a pull request may close this issue.

4 participants