Repository navigation
Take does not reserve its limit on read_to_end #51746
Description
Using readable.take(x).read_to_end(&mut buf) can cause the capacity of buf to exceed what is needed.
I tried this code:
use std::io::Read;
fn main() {
let readable = &[1];
let mut buf: Vec<u8> = Vec::with_capacity(2);
assert_eq!(buf.len(), 0);
assert_eq!(buf.capacity(), 2);
readable.take(1).read_to_end(&mut buf).unwrap();
assert_eq!(buf.len(), 1);
assert_eq!(buf.capacity(), 32);
}https://play.rust-lang.org/?gist=83401a704fddcec007bcfb0ecc85fc6d&version=nightly&mode=debug
I expected to see this happen:
readable.take(1) should reserve 1.
Take should reserve a maximum of take.limit().
Instead, this happened:
buf capacity increased because readable.take(1) reserved 32 which exceeds its limit.
Take will always reserve in increments of 32 rather than its known limit.
I'm new to Rust; my guess is implementing read_to_end specifically for Take is the way to improve this.
Activity
Rollup merge of rust-lang#52939 - ljedrz:fix_51746, r=kennytm
Rollup merge of rust-lang#52939 - ljedrz:fix_51746, r=kennytm
MichaelTheriot commentedon Aug 5, 2018
@ljedrz thanks for the PR. This solved my issue for
capacity < 32.I wanted to ask about the other scenarios where capacity is not a multiple of 32.
If I do
readable.take(34).read_to_end(&mut buf)andbufis empty with capacity of 34 it becomes 68.https://play.rust-lang.org/?gist=56ff43be5aaba973e0748ce1d84b2695&version=nightly&mode=debug&edition=2015
Is this by design? Asking because it was also a reason I reported this.
Also, are there concerns (like I/O) against simply reserving
self.limit, rather than chunking 32? That sounds like the easiest way to fix this but I noted your conservative comment.ljedrz commentedon Aug 5, 2018
@MichaelTheriot I had thought about this, but was worried about cases where the size of the readable object is unknown; in those cases you might want to e.g.
take(4096)in a loop, but the comment above the function suggested that in cases where there's a lot less to read from unnecessarily zeroing big chunks of memory was super slow.As I mentioned in my PR, I was just being cautious; if you were to benchmark different scenarios and it turned out that just reserving
limitis beneficial I'm sure such a PR would be warmly received.Adjusting the code would be easy: you'd just need to change this line to
let reservation_size = self.limit as usize;.