Make File's specialisation of read_to_end use the length of the file to size the Vec it gets passed

[AlisdairO]

Performance optimisation for File::read_to_end. Currently reallocing in the vector takes a very large percentage of runtime for loading a large file. This is unnecessary because File allows us to work out how much we have left to read, and we can use that information to size the vec in advance.

Performance figures:

Large (230MB+ file, 1024 initial vec capacity)

test sys_common::io::tests::bench_uninitialized_file ... bench: 283,419,418 ns/iter (+/- 33,257,419)
test sys_common::io::tests::bench_uninitialized_file_hint ... bench: 153,119,795 ns/iter (+/- 34,983,881)

Small files:

Since I was concerned about small files - that there might be overhead when working out the remaining size of the file - I also did a test loading a large variety of 4KB files filled with random data. I then experimented with different sizes of Vec being passed into the read_to_end function. It looks like in the worst case (where the Vec is exactly sized to the file), the new code is a little slower, but otherwise it's generally faster. I'm not too concerned about this - if you've already worked out the exact size of the file, you might just as well use read() instead.

Using Vec::new()

test sys_common::io::tests::bench_uninitialized_file_small ... bench: 9,811 ns/iter (+/- 1,346)
test sys_common::io::tests::bench_uninitialized_file_small_hint ... bench: 6,789 ns/iter (+/- 579)

Using Vec::with_capacity(1024)

test sys_common::io::tests::bench_uninitialized_file_small ... bench: 6,193 ns/iter (+/- 955)
test sys_common::io::tests::bench_uninitialized_file_small_hint ... bench: 5,896 ns/iter (+/- 525)

Using Vec::with_capacity(4096)

test sys_common::io::tests::bench_uninitialized_file_small ... bench: 5,123 ns/iter (+/- 2,129)
test sys_common::io::tests::bench_uninitialized_file_small_hint ... bench: 5,936 ns/iter (+/- 605)

'Real' application

I tested loading the large file in a simple rust application, and the runtime of the app averaged ~200ms with this optimisation and ~350ms without.

[rust-highfive]

r? @huonw

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

[huonw]

r? @alexcrichton

[alexcrichton]

I feel like this is becoming too magical of an implementation. We already do the right thing in using any pre-allocated space in the vector to issue fewer reads, so I think this kind of optimization should be left up to the caller of read_to_end. Mixing multiple I/O calls in such a core function can be quite surprising, especially if you're expecting read_to_end to only call read. An example where this needs to be guaranteed is for syscall whitelists.

Additionally, I wouldn't be confident in saying that all situations everywhere would succeed in a call to stat if and only if read would also succeed. Put another way there may be situations where stat fails and a further read will succeed.

This also seems like an easy enough (and perhaps not necessarily ubiquitously needed) optimization to applications individually, so I'm not totally inclined to merge.

[AlisdairO]

I can see the argument regarding syscall whitelists. On the other hand, I'm not so sure I agree on it being surprising that read_to_end might make other calls - it's a higher-level convenience function after all.

One option might be to change the code such that if the stat call fails, it continues but doesn't provide a size hint - I don't know if that would make this implementation more acceptable?

[alexcrichton]

Unfortunately I don't think that's sufficient, you can see #22879 for some more info. Syscalls that aren't expected need to be avoided entirely as they don't always return an error and sometimes trigger termination of the entire process.

[bors]

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

[AlisdairO]

Fair enough - it seems like a bit of a shame to give up such a simple performance win for non-user-optimised code, but if it won't work it won't work!

[Stebalien]

Fair enough - it seems like a bit of a shame to give up such a simple performance win for non-user-optimised code, but if it won't work it won't work!

There has been some talk of adding a convenience fs::read_file(path) method which would be another way to solve this.