Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take does not reserve its limit on read_to_end #51746

Closed
MichaelTheriot opened this issue Jun 23, 2018 · 2 comments
Closed

Take does not reserve its limit on read_to_end #51746

MichaelTheriot opened this issue Jun 23, 2018 · 2 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@MichaelTheriot
Copy link

MichaelTheriot commented Jun 23, 2018

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.

@Mark-Simulacrum Mark-Simulacrum added I-slow Issue: Problems and improvements with respect to performance of generated code. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 24, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Aug 1, 2018
Make io::Read::read_to_end consider io::Take::limit

Add a custom implementation of `io::Read::read_to_end` for `io::Take` that doesn't reserve the default 32 bytes but rather `Take::limit` if `Take::limit < 32`.

It's a conservative adjustment that preserves the default behavior for `Take::limit >= 32`.

Fixes rust-lang#51746.
pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 1, 2018
Make io::Read::read_to_end consider io::Take::limit

Add a custom implementation of `io::Read::read_to_end` for `io::Take` that doesn't reserve the default 32 bytes but rather `Take::limit` if `Take::limit < 32`.

It's a conservative adjustment that preserves the default behavior for `Take::limit >= 32`.

Fixes rust-lang#51746.
@MichaelTheriot
Copy link
Author

@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) and buf is 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
Copy link
Contributor

ljedrz commented 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 limit is 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;.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants