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

Add read_to_end implementation to &[u8]'s Read impl #45083

Merged
merged 1 commit into from
Oct 8, 2017

Conversation

fhartwig
Copy link
Contributor

@fhartwig fhartwig commented Oct 7, 2017

The default impl for read_to_end does a bunch of bookkeeping
that isn't necessary for slices and is about 4 times slower
on my machine.

The following benchmark takes about 30 ns before this change and about 7 ns after:

#[bench]
fn bench_read_std(b: &mut Bencher) {
    let data = vec![0u8; 100];
    let mut v = Vec::with_capacity(200);
    b.iter(|| {
        let mut s = data.as_slice();
        v.clear();         
        s.read_to_end(&mut v).unwrap();
    });
}

This solves the easy part of #44819 (I think extending this to Take<&[u8]> would require specialization)

@bluss
Copy link
Member

bluss commented Oct 7, 2017

This looks good to me.

Only a super minor curiosity, does it matter to us where the slice's pointer is when read_to_end returns? The remaining slice here points into a static [] instead of somewhere in bounds of the original slice. The super backwards compatible approach would split the slice at len and set *self to the empty tail.

@fhartwig
Copy link
Contributor Author

fhartwig commented Oct 7, 2017

I was briefly wondering the same thing and then I somehow got distracted and forgot. I guess it makes sense to set self to &self[len..] to minimise the odds of breaking something.

@sfackler
Copy link
Member

sfackler commented Oct 7, 2017

I don't think anyone would run into issues in practice with the current implementation, but switching to &self[len..] seems reasonable to me.

The default impl for read_to_end does a bunch of bookkeeping
that isn't necessary for slices and is about 4 times slower
on my machine.
@fhartwig
Copy link
Contributor Author

fhartwig commented Oct 7, 2017

I've changed the implementation. The method now sets self to its empty tail.

@bluss
Copy link
Member

bluss commented Oct 7, 2017

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 7, 2017

📌 Commit d52acbe has been approved by bluss

@bluss bluss added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 8, 2017
kennytm added a commit to kennytm/rust that referenced this pull request Oct 8, 2017
Add read_to_end implementation to &[u8]'s Read impl

The default impl for read_to_end does a bunch of bookkeeping
that isn't necessary for slices and is about 4 times slower
on my machine.

The following benchmark takes about 30 ns before this change and about 7 ns after:

```
#[bench]
fn bench_read_std(b: &mut Bencher) {
    let data = vec![0u8; 100];
    let mut v = Vec::with_capacity(200);
    b.iter(|| {
        let mut s = data.as_slice();
        v.clear();
        s.read_to_end(&mut v).unwrap();
    });
}
```

This solves the easy part of  rust-lang#44819 (I think extending this to `Take<&[u8]> `would require specialization)
bors added a commit that referenced this pull request Oct 8, 2017
Rollup of 10 pull requests

- Successful merges: #45018, #45042, #45052, #45053, #45058, #45060, #45081, #45083, #45090, #45094
- Failed merges:
@bors bors merged commit d52acbe into rust-lang:master Oct 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants