std: Add `io` module again #21835

alexcrichton · Feb 1, 2015

This commit is an implementation of RFC 576 which adds back the std::io
module to the standard library. No functionality in std::old_io has been
deprecated just yet, and the new std::io module is behind the same io
feature gate.

A good bit of functionality was copied over from std::old_io, but many tweaks
were required for the new method signatures. Behavior such as precisely when
buffered objects call to the underlying object may have been tweaked slightly in
the transition. All implementations were audited to use composition wherever
possible. For example the custom pos and cap cursors in BufReader were
removed in favor of just using Cursor<Vec<u8>>.

A few liberties were taken during this implementation which were not explicitly
spelled out in the RFC:

The old LineBufferedWriter is now named LineWriter
The internal representation of Error now favors OS error codes (a
0-allocation path) and contains a Box for extra semantic data.
The io prelude currently reexports Seek as NewSeek to prevent conflicts
with the real prelude reexport of old_io::Seek
The chars method was moved from BufReadExt to ReadExt.
The chars iterator returns a custom error with a variant that explains that
the data was not valid UTF-8.

rust-highfive · Feb 1, 2015

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · Feb 1, 2015

Note that rust-lang/rfcs#576 has not yet been merged, so this should hold off on merging until that happens.

alexcrichton · Feb 1, 2015

alexcrichton reviewed Feb 1, 2015

View changes

alexcrichton · Feb 1, 2015

src/libstd/io/impls.rs

+        unsafe {
+            let other = ptr::read(self);
+            *self = &mut other[amt..];
+        }


I was unable to figure out a safe way of doing this, but I have a feeling one may be lurking somewhere, certainly open to suggestions!

This works (see in playpen):

fn cut<T>(xs: &mut &mut [T], amt: usize) { let original = mem::replace(xs, &mut []); mem::replace(xs, &mut original[amt..]); }

Aha, thanks!

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/mod.rs

+    ///
+    /// The returned type implements `Iterator` where the `Item` is `Result<u8,
+    /// R::Err>`.  The yielded item is `Ok` if a byte was successfully read and
+    /// `Err` otherwise for I/O errors. EOF is mapped to returning `None` for


from this iterator

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/mod.rs

+    ///
+    /// The returned instance of `Read` will yield all this object's bytes
+    /// until EOF is reached. Afterwards the bytes of `next` will be yielded
+    /// infinitely.


Infinitely as even regardless of EOFs? Where will it get those infinite bytes from?

The returned Read instance will first read all bytes from this object until EOF is encountered. Afterwards the instance(’s output) is equivalent to the next(’s output).

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/mod.rs

+/// throughout the I/O and related libraries take and provide types which
+/// implement the `Write` trait.
+pub trait Write {
+    /// Write a buffer into this object, returning how many bytes were read.


s/were read/were written/

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/mod.rs

+            match self.write(buf) {
+                Ok(0) => return Err(Error::new(ErrorKind::EndOfFile,
+                                               "failed to write whole buffer: \
+                                                eof reached", None)),


This is Write, not Read. Ok(0) does not signify EOFs, does it? I’d use ErrorKind::Other or something here.

Currently this is used to signify that the write could not complete because the "destination was full", which in some sense is a bit of an EOF condition. For example the main user of this is the Write impl for &mut [u8] which returns EOF when self is the empty slice.

That being said though, I can definitely see where having an EndOfFile variant of ErrorKind is somewhat confusing as it's not intended to be returned for functions like read.

Yes, we definitely need to change this error variant, or we will have mass confusion.

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/mod.rs

+
+/// Enumeration of possible methods to seek within an I/O object.
+#[derive(Copy, PartialEq, Eq, Clone, Debug)]
+pub enum SeekFrom {


This doesn’t match the current version of RFC. Did somebody forgot to update it?

Indeed! I adopted the proposed renaming ahead of time as I think we'll take it. I'll update this to whatever ends up being settled upon, however.

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/mod.rs

+
+    /// Read all bytes until the byte 0xA ('\n') is reached.
+    ///
+    /// This function will ;consume all bytes in the underlying stream until a


This is not good because

It will likely read more bytes from the underlying stream

Not all of those bytes will be appended

nagisa · Feb 1, 2015

nagisa reviewed Feb 1, 2015

View changes

nagisa · Feb 1, 2015

src/libstd/io/cursor.rs

+/// Cursors are not currently generic over the type contained within, but may
+/// become so.
+pub struct Cursor<T> {
+    pos: u64,


~~Why not usize?~~

comment removed

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/buffered.rs

+///
+/// This writer will be flushed when it is dropped.
+pub struct BufWriter<W> {
+    inner: Option<W>,


This can be implemented without Option.

It can indeed. That is currently being tracked in #21729. I would like to investigate the performance impact of having Option<W> vs W and didn't want to go too much into the benchmarking details on this PR. This is an implementation detail which shouldn't affect the API and we can always update this in the future.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/buffered.rs

+    fn flush_buf(&mut self) -> io::Result<usize> {
+        let len = self.buf.len();
+        let n = try!(self.inner.as_mut().unwrap().write(&self.buf[]));
+        // FIXME would be better expressed as .remove(0..n) if it existed


There is an RFC open for exactly this.

Indeed! That is what inspired me to write this comment.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/buffered.rs

+        }
+    }
+
+    fn flush(&mut self) -> io::Result<()> {


/// Flush this output stream, ensuring that all intermediately buffered contents reach their destination.

This implementation is wrong because flush_buf does not ensure this.

flush should probably return an error if flush_buf doesn't write all bytes.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/buffered.rs

+    fn drop(&mut self) {
+        if self.inner.is_some() {
+            // dtors should not panic, so we ignore a panicked flush
+            let _ = self.flush_buf();


/// This writer will be flushed when it is dropped.

Same as above.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/buffered.rs

+
+impl<W: Write> Write for LineWriter<W> {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        match buf.rposition_elem(&b'\n') {


This should use memrchr which is much faster.

Do we have an issue open for adding memchr intrinsics to LLVM? Has it been discussed with LLVM developers? cc @dotdash @Aatch @luqmana @pcwalton

memrchr, not memchr.

I used plural because I meant mem*chr, sorry for not being more clear.

I agree that this is definitely an area where we can have future optimizations. For now I think we may want to track this in an issue because we're not exposing memchr in the standard library (or memrchr). I tried to move instances of .iter().position(..) to position_elem or rposition_elem so they're easy to flag and migrate once we have them implemented.

One possibility would be to expand the slice::bytes module with some functions like this as a place to house the optimized versions.

memrchr seems to be a GNU extension, and isn't necessarily available cross-platform.

As with memmem it should be available everywhere but windows.

We could at least have the assembly implementations bundled with Rust to not depend on system ones - however, that's subpar compared with LLVM handling them - but it could be cumbersome for LLVM to keep track of all these intrinsics, maybe what it needs is more general loop idioms that optimize down to instruction similar to the manual versions.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/buffered.rs

+                let n = try!(self.inner.write(&buf[..i + 1]));
+                if n != i + 1 { return Ok(n) }
+                try!(self.inner.flush());
+                self.inner.write(&buf[i + 1..])


This does not return the correct value.

Oops, good catch!

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/cursor.rs

+            // currently are
+            let difference = pos as i64 - len as i64;
+            if difference > 0 {
+                self.get_mut().extend(repeat(0).take(difference as usize));


This should use memset which is much faster. (33x)

I definitely agree that memset would likely do the trick here, but for now I would prefer to avoid unnecessary unsafe code and this is a well known problem which needs an idiomatic solution no matter what.

For now I'd prefer to postpone optimizing this aspect in particular until we have a more general method of allowing the optimization to happen on Vec natively.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/cursor.rs

+    }
+}
+impl Write for Cursor<Vec<u8>> {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {


This looks like there is a much shorter implementation.

It would be more helpful to me at least for this to be a little more constructive. Do you have some suggestions about how the method could be trimmed down?

Something like this:

fn write(&mut self, buf: &[u8]) -> io::Result<usize> { let pos = self.pos as usize; // Insert skipped zeros. let tail = pos.saturating_sub(self.inner.len()); self.inner.extend(repeat(0).take(tail)); // Make room for buf. let overhead = (pos + buf.len()).saturating_sub(self.inner.len()); self.inner.reserve(overhead); unsafe { let len = self.inner.len(); self.inner.set_len(len + overhead); } // Copy buf. let dst = &mut self.inner[pos..]; slice::bytes::copy_memory(dst, buf); self.pos += buf.len() as u64; Ok(buf.len()) }

Cool! I think that the size of this method originally stemmed from trying to micro-optimize it because Write for Vec<u8> didn't exist (and it was the only write-into-memory writer), but I agree that lots of the branches can be removed.

I've adapted what you've posted to use memcpy twice instead of once to avoid the use of unsafe, but if it becomes a bottleneck we can certainly optimize it a bit more.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/error.rs

+    }
+
+    /// Creates a new instance of an `Error` from a particular OS error code.
+    pub fn from_os_error(code: i32) -> Error {


to_os_error?

Our conventions dictate that to_foo is a function which returns Foo of some form. In this case the Error isn't necessarily an OsError, so I wouldn't expect the os_error suffix of the to variant.

Our conventions also say that from_foo means that the function takes a Foo and returns a Self, which is largely what this method is doing. That being said this is saying that an "os error" is simply an instance of i32, which we may want to change in the future (with an explicit OsError), so it may not be the best name for this method.

I actually meant that there should be an inverse to_os_error function that returns the underlying error code, if any, for C interop.

I think for now I'm going to try to take a relatively conservative approach with io::Error and we can try to expand it later on.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/error.rs

+    }
+
+    /// Returns a detailed error message for this error (if one is available)
+    pub fn detail(&self) -> Option<String> {


mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+        let n = try!(f({
+            let base = v.as_mut_ptr().offset(v.len() as isize);
+            black_box(slice::from_raw_mut_buf(mem::copy_lifetime(v, &base),
+                                              v.capacity() - v.len()))


Why so complicated?

Never mind.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+    unsafe fn black_box<T>(dummy: T) -> T {
+        asm!("" : : "r"(&dummy));
+        dummy
+    }


This doesn't look like production quality. See e.g.

http://blog.fefe.de/?ts=afc03b62 (German)

http://llvm.org/bugs/show_bug.cgi?id=15495

This is passing a shared reference to an (otherwise immutable) stack slot. The only reason it works is that LLVM doesn't tell the difference - it's like using const in C when you clearly want to pretend mutation is happening.
Aside from telling LLVM about its immutability, an immediate T with no unsafe interior is safe to promote to a SSA value when debuginfo isn't required, and I have implemented this optimization in all of of my trans refactoring branches.
I highly suggest passing &mut dummy to asm!, in addition to the "memory" clobber suggested by @mahkoh.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+    ///
+    /// If this function encounters any form of I/O or other error, an error
+    /// variant will be returned. If an error is returned then it is guaranteed
+    /// that no bytes were read successfully.


Could they have been read unsuccessfully?

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+    ///
+    /// If the return value of this method is `Ok(n)`, then it must be
+    /// guaranteed that `0 <= n <= buf.len()`. If `n` is `0` then it indicates
+    /// that this reader as reached "end of file" and will likely no longer be


Unless buf.len is 0.

There are well known examples where 0 does not mean that further reads will also return 0. Maybe this should not talk about what's "likely" or "unlikely".

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+    /// is reached, at which point this function will immediately return.
+    ///
+    /// This function will only successfully return if an invocation of `Ok(0)`
+    /// succeeds at which point all bytes have been read into `buf`.


an invocation of Ok(0)

Maybe my English is failing me but this doesn't make much sense.

at which point all bytes have been read into buf

All bytes of what?

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+    ///
+    /// If any other read error is encountered then this function immediately
+    /// returns. Any bytes which have already been read will be present in
+    /// `buf` and the length will be adjusted appropriately.


and the length will be adjusted appropriately.

Sounds like an implementation detail.

mahkoh · Feb 1, 2015

mahkoh reviewed Feb 1, 2015

View changes

mahkoh · Feb 1, 2015

src/libstd/io/mod.rs

+        read_to_end(self, buf)
+    }
+
+    /// Read all remaining bytes in this source, placing them into `buf`.


Again: "all remaining bytes" is not necessarily true.

Can you clarify why you think this is not true?

It reads until EOF which does not imply that further reads also return EOF.

alexcrichton · Feb 3, 2015

@bors: r=aturon 5cf9905 p=1

bors · Feb 3, 2015

⌛️ Testing commit 5cf9905 with merge 9830b16...

bors · Feb 3, 2015

💔 Test failed - auto-linux-64-x-android-t

bors · Feb 3, 2015

💔 Test failed - auto-win-32-opt

bors · Feb 3, 2015

💔 Test failed - auto-linux-32-opt

alexcrichton · Feb 3, 2015

@bors: retry

bors · Feb 4, 2015

⌛️ Testing commit 5cf9905 with merge bc16888...

bors · Feb 4, 2015

💔 Test failed - auto-linux-64-x-android-t

alexcrichton · Feb 4, 2015

@bors: retry

bors · Feb 4, 2015

⌛️ Testing commit 5cf9905 with merge 661a65e...

bors · Feb 4, 2015

💔 Test failed - auto-linux-64-x-android-t

alexcrichton · Feb 4, 2015

@bors: retry

bors · Feb 4, 2015

⌛️ Testing commit 5cf9905 with merge b74eef5...

bors · Feb 4, 2015

💔 Test failed - auto-linux-64-x-android-t

alexcrichton · Feb 4, 2015

@bors: retry

alexcrichton · Feb 4, 2015

@bors: retry

bors · Feb 4, 2015

⌛️ Testing commit 5cf9905 with merge 6a02f98...

bors · Feb 4, 2015

💔 Test failed - auto-linux-64-x-android-t

sfackler · Feb 4, 2015

@bors: retry

reem · Feb 24, 2015

reem reviewed Feb 24, 2015

View changes

reem · Feb 24, 2015

src/libstd/io/mod.rs

+
+    // Semi-hack used to prevent LLVM from retaining any assumptions about
+    // `dummy` over this function call
+    unsafe fn black_box<T>(mut dummy: T) -> T {


Perhaps we should move this out of test into std::mem or something

It is not related to memory, though. I find test be a better location for this.

rust-highfive assigned aturon Feb 1, 2015

alexcrichton referenced this pull request Feb 1, 2015
Merged
Amend RFC 517: Revisions to reader/writer, core::io and std::io #576

alexcrichton force-pushed the alexcrichton:iov2 branch from 0ae1533 to 7aaa7de Feb 1, 2015

alexcrichton force-pushed the alexcrichton:iov2 branch from 8346a0d to 5cf9905 Feb 3, 2015

bors merged commit 5cf9905 into rust-lang:master Feb 4, 2015
1 of 2 checks passed

1 of 2 checks passed

homu Test failed
Details

continuous-integration/travis-ci The Travis CI build passed
Details

alexcrichton deleted the alexcrichton:iov2 branch Feb 4, 2015

aturon referenced this pull request Feb 18, 2015
Closed
Stabilization for 1.0-beta #22500
75 of 91 tasks complete

rust-lang/rust

Join GitHub today

std: Add `io` module again #21835

Conversation

alexcrichton commented Feb 1, 2015

rust-highfive assigned aturon Feb 1, 2015

This comment has been minimized.

rust-highfive commented Feb 1, 2015

This comment has been minimized.

alexcrichton commented Feb 1, 2015

alexcrichton referenced this pull request Feb 1, 2015

alexcrichton force-pushed the alexcrichton:iov2 branch from 0ae1533 to 7aaa7de Feb 1, 2015

alexcrichton reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nagisa reviewed Feb 1, 2015 View changes

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

This comment has been minimized.

mahkoh reviewed Feb 1, 2015 View changes

This comment has been minimized.

alexcrichton force-pushed the alexcrichton:iov2 branch from `0ae1533` to `7aaa7de` Feb 1, 2015

alexcrichton reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

nagisa reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

mahkoh reviewed Feb 1, 2015

View changes

alexcrichton force-pushed the alexcrichton:iov2 branch from `8346a0d` to `5cf9905` Feb 3, 2015