Add Read::size_hint and pre-allocate in read_to_end

[SimonSapin]

Many FromIterator implementations rely on Iterator::size_hint to pre-allocate memory. This is a prototype of what something equivalent for std::io::Read could look like. An alternative would be to not add any public API but use an private specialization trait, like ZipImpl in libcore.

This came up in #45837 in the case of reading from a file. I’ve measured File::read_to_end with a vector created with Vec::with_capacity(file.metadata().len() as usize), compared to Vec::new(). On my linux desktop with an SSD (though everything is probably in filesystem cache here), pre-allocating + reading take 3% more time to 43% less time depending on file size.

Running target/release/deps/read_to_end_bench-9122fdf004b0166c
running 24 tests
test a_read_128::vec_new ... bench:1,380 ns/iter(+/- 49) = 92MB/s
test a_read_128::vec_with_capacity ... bench:1,416 ns/iter(+/- 107) = 90MB/s
test b_read_512::vec_new ... bench:1,690 ns/iter(+/- 78) = 302MB/s
test b_read_512::vec_with_capacity ... bench:1,736 ns/iter(+/- 52) = 294MB/s
test c_read_2k::vec_new ... bench:2,085 ns/iter(+/- 113) = 982MB/s
test c_read_2k::vec_with_capacity ... bench:2,015 ns/iter(+/- 72) = 1016MB/s
test d_read_8k::vec_new ... bench:2,778 ns/iter(+/- 131) = 2948MB/s
test d_read_8k::vec_with_capacity ... bench:2,516 ns/iter(+/- 89) = 3255MB/s
test e_read_32k::vec_new ... bench:5,107 ns/iter(+/- 103) = 6416MB/s
test e_read_32k::vec_with_capacity ... bench:4,404 ns/iter(+/- 81) = 7440MB/s
test f_read_128k::vec_new ... bench:16,232 ns/iter(+/- 106) = 8074MB/s
test f_read_128k::vec_with_capacity ... bench:12,223 ns/iter(+/- 103) = 10723MB/s
test g_read_512k::vec_new ... bench:39,179 ns/iter(+/- 210) = 13381MB/s
test g_read_512k::vec_with_capacity ... bench:31,704 ns/iter(+/- 98) = 16536MB/s
test h_read_1m::vec_new ... bench:463,119 ns/iter(+/- 2,354) = 2264MB/s
test h_read_1m::vec_with_capacity ... bench:251,983 ns/iter(+/- 3,072) = 4161MB/s
test i_read_2m::vec_new ... bench:668,742 ns/iter(+/- 8,317) = 3135MB/s
test i_read_2m::vec_with_capacity ... bench:383,879 ns/iter(+/- 1,269) = 5463MB/s
test j_read_4m::vec_new ... bench:1,188,553 ns/iter(+/- 13,409) = 3528MB/s
test j_read_4m::vec_with_capacity ... bench:857,714 ns/iter(+/- 8,254) = 4890MB/s
test k_read_8m::vec_new ... bench:2,302,201 ns/iter(+/- 15,746) = 3643MB/s
test k_read_8m::vec_with_capacity ... bench:1,947,089 ns/iter(+/- 10,942) = 4308MB/s
test l_read_32m::vec_new ... bench:8,990,230 ns/iter(+/- 18,718) = 3732MB/s
test l_read_32m::vec_with_capacity ... bench:8,648,669 ns/iter(+/- 23,157) = 3879MB/s
test result: ok.0passed;0 failed;0 ignored;24 measured;0 filtered out

#![feature(test)]externcrate test;externcrate tempdir;use std::fs::File;use std::io::{Write,Read};use test::Bencher;fnrun<F>(bencher:&mutBencher,size:usize,new_buffer:F)whereF:Fn(&File) -> Vec<u8>{let dir = tempdir::TempDir::new("bench").unwrap();let path = dir.path().join("something");File::create(&path).unwrap().write_all(&vec![42; size]).unwrap();
bencher.bytes = size asu64;
bencher.iter(|| {letmut file = File::open(&path).unwrap();letmut buffer = new_buffer(&file);
file.read_to_end(&mut buffer).unwrap();})}macro_rules! sizes {($( $name: ident $size: expr )+) => {
$(mod $name {usesuper::*;
#[bench]fn vec_new(bencher:&mutBencher){
run(bencher, $size, |_| Vec::new())}
#[bench]fn vec_with_capacity(bencher:&mutBencher){
run(bencher, $size, |file| {Vec::with_capacity(file.metadata().unwrap().len()asusize)})}})+
}}sizes!{
a_read_128 128
b_read_512 512
c_read_2k 2*1024
d_read_8k 8*1024
e_read_32k 32*1024
f_read_128k 128*1024
g_read_512k 512*1024
h_read_1m 1024*1024
i_read_2m 2*1024*1024
j_read_4m 4*1024*1024
k_read_8m 8*1024*1024
l_read_32m 32*1024*1024}

[bluss]

If this adds a new system call when calling read_to_end on a File, it has been discussed before. Putting a system call in size_hint (metadata) is not ideal. (I can't find it though.)

[SimonSapin]

Yes, File::size_hint as implemented in the current PR does make a system call. I agree this is not ideal. I expect this PR won’t be merged as-is, I submitted it to have some starting point to discuss what if anything should be done here.

[ollie27]

It's a minor point but this should probably use a saturating cast not as.

[SimonSapin]

Is there an API for doing saturating integer casts? Writing #[cfg(target_pointer_size = …)] code here seems tedious.

[ollie27]

Sadly not yet (rust-lang/rfcs#1218). Something like cmp::min(meta.len(), usize::max_value() as u64) as usize should work though.

[SimonSapin]

On the other hand this is a hint, the default impl returns zero without necessarily being at EOF so underestimating should be OK. And if you’re reading a file larger than your address space, you’re gonna have a hard time in read_to_end anyway.

[sfackler]

This also needs to check its current position and subtract that off, right?

[SimonSapin]

Right. However the obvious fix doesn’t work because <File as Seek>::seek takes &mut self, even with SeekFrom::Current(0). I wonder if there should be some API like File::position(&self) -> io::Result<u64>.

[SimonSapin]

Never mind, <std::fs::File as Seek>::seek calls std::sys::fs::File::seek which takes &self, so size_hint can call the latter directly.

[mbrubeck]

read_to_end still reads at most 8KB at a time, even if the entire buffer is allocated up-front. The pre-allocated case could be optimized further by reading up to the available capacity in a single read syscall.

Update: This was fixed by #46050.

[shepmaster]

random-assigning to...

r? @sfackler

[sfackler]

Does #46050 give the same improvement?

[sunfishcode]

A non-obvious detail of read_to_end's loop is that you always have to allocate at least one byte more than the file size, because it reads until read returns 0, and that last read needs to be passed some space that it won't use.

If you're calling metadata(), when it provides a size, can you trust the size (and not take it as a hint)? If so, in that case you could add alternate logic in read_to_end that just reads until it gets that many bytes. That would avoid the need to allocate extra space, and the extra read syscall at the end. In the common case you could do one fstat and one read, rather than two reads.

[mbrubeck]

You can't trust the size from metadata because the file may change between the metadata call and the read call.

[sunfishcode]

You always have to be prepared to get fewer bytes than expected, or errors, but if there are more bytes than expected, it's no different from someone appending bytes after your last read returned 0. You'll miss the newly appended bytes, but it was a race anyway.

That assumes that metadata() isn't memoized, but it doesn't currently appear to be.

[bors]

☔ The latest upstream changes (presumably #46166) made this pull request unmergeable. Please resolve the merge conflicts.

[carols10cents]

What's the status of this @SimonSapin ? I'm having a hard time telling :)

[SimonSapin]

Does #46050 give the same improvement?

I’ve run the benchmark again in rustc 1.24.0-nightly (560a5da 2017-11-27), which includes #46050. Previous results were “3% more time to 43% less time” Now the time with Vec::with_capacity is 4% to 48% less time than Vec::new.

So this PR’s improvement seems independent of #46050’s improvement.

More important IMO is whether Read::size_hint is the kind of public API we want to stabilize eventually.

Details

test a_read_128::vec_new ... bench: 1,362 ns/iter (+/- 200) = 93 MB/s
test a_read_128::vec_with_capacity ... bench: 1,158 ns/iter (+/- 199) = 110 MB/s
test b_read_512::vec_new ... bench: 1,705 ns/iter (+/- 203) = 300 MB/s
test b_read_512::vec_with_capacity ... bench: 1,172 ns/iter (+/- 301) = 436 MB/s
test c_read_2k::vec_new ... bench: 2,102 ns/iter (+/- 188) = 974 MB/s
test c_read_2k::vec_with_capacity ... bench: 1,193 ns/iter (+/- 327) = 1716 MB/s
test d_read_8k::vec_new ... bench: 2,759 ns/iter (+/- 271) = 2969 MB/s
test d_read_8k::vec_with_capacity ... bench: 1,412 ns/iter (+/- 187) = 5801 MB/s
test e_read_32k::vec_new ... bench: 4,957 ns/iter (+/- 93) = 6610 MB/s
test e_read_32k::vec_with_capacity ... bench: 2,878 ns/iter (+/- 205) = 11385 MB/s
test f_read_128k::vec_new ... bench: 14,690 ns/iter (+/- 135) = 8922 MB/s
test f_read_128k::vec_with_capacity ... bench: 9,101 ns/iter (+/- 160) = 14401 MB/s
test g_read_512k::vec_new ... bench: 30,788 ns/iter (+/- 284) = 17028 MB/s
test g_read_512k::vec_with_capacity ... bench: 21,640 ns/iter (+/- 42) = 24227 MB/s
test h_read_1m::vec_new ... bench: 444,013 ns/iter (+/- 3,361) = 2361 MB/s
test h_read_1m::vec_with_capacity ... bench: 232,138 ns/iter (+/- 2,474) = 4517 MB/s
test i_read_2m::vec_new ... bench: 633,958 ns/iter (+/- 3,849) = 3308 MB/s
test i_read_2m::vec_with_capacity ... bench: 343,065 ns/iter (+/- 813) = 6112 MB/s
test j_read_4m::vec_new ... bench: 1,114,472 ns/iter (+/- 12,811) = 3763 MB/s
test j_read_4m::vec_with_capacity ... bench: 773,090 ns/iter (+/- 2,065) = 5425 MB/s
test k_read_8m::vec_new ... bench: 2,156,756 ns/iter (+/- 7,510) = 3889 MB/s
test k_read_8m::vec_with_capacity ... bench: 1,792,980 ns/iter (+/- 5,539) = 4678 MB/s
test l_read_32m::vec_new ... bench: 8,499,907 ns/iter (+/- 24,512) = 3947 MB/s
test l_read_32m::vec_with_capacity ... bench: 8,129,977 ns/iter (+/- 22,403) = 4127 MB/s

[sunfishcode]

Reserving size_hint bytes sets up read_to_end to reallocate in the common case, because it needs extra space for the final read to detect EOF. This can create 2x or more slowdowns in some cases and the resulting buffers are twice as large as they need to be due to Vec's doubling behavior.

This can be fixed either by reserving size_hint + 1 bytes rather than size_hint bytes, or by #46340.

[SimonSapin]

Good point. I’ve added the + 1. As mbrubeck mentioned above, the approach in #46340 is racy since the file could grow in the meantime.

[sunfishcode]

To be sure, read_to_end is already racy; if someone appends to a file while read_to_end is reading it, read_to_end may or may not see the appended bytes, because if it gets to EOF before the write happens, it stops reading.

[sfackler]

Cool - something like this seems worthwhile then. Should it return an io::Result<usize>? You can always return 0 if something went wrong, but it seems like it's nice to not throw away those errors. I kind of doubt that reads would succeed if you can't stat the file anymore.

[SimonSapin]

@sfackler Good point. Changed to io::Result<usize>.

[SimonSapin]

I don’t have a strong opinion on the strategy of this PR v.s. that of #46340. A key difference is the semantics of size_hint / size_snapshot in the general Read case, not just File: size_hint is intended to be an estimate and defaults to zero (like the first component of Iterator::size_hint), size_snapshot is optional and when Some(_) is assumed to be exact.

[bors]

☔ The latest upstream changes (presumably #46485) made this pull request unmergeable. Please resolve the merge conflicts.

[sfackler]

Sorry for the delay here - should we maybe make an issue to figure out the approach to take? I'd like to avoid splitting the discussion across two PRs.

[shepmaster]

I'm going to make an executive decision and mark this and #46340 as waiting on a team decision as it seems that they cannot both exist together. Let me know if you disagree.

[sfackler]

The @rust-lang/libs team talked about this and #46340 during our triage meeting and decided that we're not looking to expand the surface area of Read in this way just yet. Instead, we can modify the implementation of fs::read to do a length lookup which should cover 99% of the use cases for this kind of thing.