File IO with read_to_end and read_to_string is slower than possible

[Mark-Simulacrum]

Rust's current implementation of read_to_end will read exponentially larger chunks of the buffer, but only up to 8192. This is slow when reading a greater than 16KB file if counting by syscalls.

NodeJS reads the entire file in one go, making 3 syscalls: open, fstat, read, and close.
Rust currently makes (for the same file) 58 syscalls not counting mmap/madvise which may be circumstantial (not related to IO). Specifically: open, ioctl, read * 37, mmap, madvise, read * 18, madvise * 14, close.

I would expect that functions such as read_to_end and read_to_string would do the minimal work necessary to read the entire file.

The below diff is what it takes to currently get Rust to do 4 syscalls (open, fstat, read, read, close). I think it's reasonable that similar code could be included (perhaps with specialization) in the filesystem reading code.

The difference between the two (with dropping caches):
fast version:

real    2m45.157s
user    0m11.484s
sys 0m1.516s

current, slower version:

real    3m9.654s
user    0m11.580s
sys 0m1.956s

-            let mut file = File::open(entry.path())?;
-            let mut file_contents = String::new();
+
+            let file = File::open(entry.path())?;
+            let capacity = match file.metadata() {
+                Ok(metadata) => metadata.len(),
+                Err(_) => 0,
+            };
+            let mut reader = BufReader::with_capacity(capacity as usize, file);
+            let mut file_contents = String::with_capacity(capacity as usize);
+            let len = match reader.fill_buf() {
+                Ok(buf) => {
+                    match ::std::str::from_utf8(buf) {
+                        Ok(s) => file_contents.push_str(s),
+                        Err(_) => {
+                            skipped += 1;
+                            continue;
+                        }
+                    };
+                    buf.len()
+                },
+                Err(_) => {
+                    skipped += 1;
+                    continue;
+                }
+            };
+            reader.consume(len);
+            reader.read_to_string(&mut file_contents)?;
+
             // Skip files whose size is 0.
-            if file.read_to_string(&mut file_contents)? == 0 {
+            if file_contents.is_empty() {

[Stebalien]

I completely agree but we can do better. We can just read directly to the string (given a bit of unsafe code).

[Mark-Simulacrum]

The standard library already reads into the underlying Vec<u8> buffer of strings and then does a pass to check that the data is valid. That's something that should continue to be done; my point with showing the code above was to demonstrate that it is possible currently, and that including something like what I suggest shouldn't require a massive overhaul of the code which reads to end/string.

[alexcrichton]

This has been proposed before as well, and I've written up some concerns about this solution.

It basically just boils down to the fact that if you expect read to only call read, this can cause problems. If you want your program to run fast, though, then it's not unexpected but rather desired.

[Mark-Simulacrum]

Would it be unreasonable to ask that if a buffer has been pre-allocated to the capacity, the buffer's capacity is used as a heuristic? This requires no extra syscalls. Also, it seems (though I may be wrong here) that this was the behavior before 240734c (PR #23847); where the write size no longer used the underlying buffers capacity as a baseline for how much to read.

I feel that while growing the underlying buffer's capacity without bounds isn't a good idea (at least by default); using the existing capacity still makes sense. Namely, if the buffer's capacity is greater than the default buffer size (8KB), then don't allocate more memory (this is already the case). However, if given a buffer with a greater capacity, attempt to read buf.capacity() - buf.len() as before, in the original code which introduced faster behavior (ccb4e84; PR #23820).

Some form of exponential (?) backing off could be implemented if the read returns less than expected.

[alexcrichton]

@Mark-Simulacrum hm it should be the case right now that if you have a pre-allocated buffer (via Vec::with_capacity or something like that) we should attempt to read into the entire buffer. I could imagine that the growing is not proportional after that, but it seems reasonable to change that!

[Mark-Simulacrum]

This is the line in the source code for the read_to_end (used by both Read::read_to_string and Read::read_to_end). I see no reference to the capacity, though I may be misreading something.

I can try to get the description I've listed above implemented, though I'm not sure I can run the tests locally with any success (in the past I've seen failures on master, so I suspect something in my environment may be wrong).

[retep998]

The current default read_to_end appears to simply start with a read of 16 always.

rust/src/libstd/io/mod.rs

Lines 345 to 374 in b30eff7

	fn read_to_end<R: Read + ?Sized>(r: &mut R, buf: &mut Vec<u8>) -> Result<usize> {
	let start_len = buf.len();
	let mut len = start_len;
	let mut new_write_size = 16;
	let ret;
	loop {
	if len == buf.len() {
	if new_write_size < DEFAULT_BUF_SIZE {
	new_write_size *= 2;
	}
	buf.resize(len + new_write_size, 0);
	}
	match r.read(&mut buf[len..]) {
	Ok(0) => {
	ret = Ok(len - start_len);
	break;
	}
	Ok(n) => len += n,
	Err(ref e) if e.kind() == ErrorKind::Interrupted => {}
	Err(e) => {
	ret = Err(e);
	break;
	}
	}
	}
	buf.truncate(len);
	ret
	}

Some types however opt into using read_to_end_uninitialized instead however, which reads into the space between .len() and .capacity() however big it is. https://github.com/rust-lang/rust/blob/e1195c24bb567019d7cdc65bf5a4c642e38475d1/src/libstd/sys/common/io.rs

[Mark-Simulacrum]

These are the only uses of read_to_end_unitialized that I could find aside from tests/benchmarks. FileDesc is used by File in src/libstr/sys/unix/fs.rs I think, but I don't understand where std::fs::File comes from yet.

Either way, it seems that the capacity-based reading either doesn't currently happen or doesn't happen on Windows.

• FileDesc - src/libstd/sys/unix/fd.rs:150
• RawHandle - src/libstd/sys/windows/handle.rs:198
• Socket - src/libstd/sys/windows/net.rs:250
• Stdin - src/libstd/sys/windows/stdio.rs:148

[alexcrichton]

Yes that's the generic implementation of read_to_end but that's not the one that's specialized for files. Both Windows and Unix bottom out in read_to_end_uninitialized which reads up to the vector's capacity.

[Mark-Simulacrum]

Hmm, I'm not sure I'm doing this right, but with the code below and debugging in gdb, I'm hitting the read_to_end helper function, not the read_to_end_unitialized. So.. not sure what's going on here. I'm on unix (Ubuntu 16.04)...

let mut file = File::open(entry.path())?;
let mut file_contents = String::new();
file.read_to_string(&mut file_contents).unwrap();

[alexcrichton]

Oh looks like the implementation of read_to_string calls the read_to_end helper rather than self.read_to_end. We can probably trivially fix that and have read_to_string benefit in the same way that read_to_end does!

[Mark-Simulacrum]

I don't believe there's much more to do here, so I'm going to close. Benchmarks seemed to indicate this wasn't actually a win.