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

Inline simple Cursor write calls #33921

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Inline simple Cursor write calls #33921

merged 1 commit into from
Jun 1, 2016

Conversation

jameysharp
Copy link
Contributor

Implementing the Write trait for Cursors over slices is so light-weight that under some circumstances multiple writes can be fused into a single instruction. In general I think inlining these functions is a good idea because most of the code can be constant-folded and copy-propagated away.

Closes issue #33916.

r? @alexcrichton

Implementing the Write trait for Cursors over slices is so light-weight that under some circumstances multiple writes can be fused into a single instruction. In general I think inlining these functions is a good idea because most of the code can be constant-folded and copy-propagated away.

Closes issue rust-lang#33916.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

@bors: r+ 80d7333

Thanks!

@bors
Copy link
Contributor

bors commented May 31, 2016

⌛ Testing commit 80d7333 with merge 6b930d1...

@bors
Copy link
Contributor

bors commented May 31, 2016

💔 Test failed - auto-linux-64-cross-netbsd

@alexcrichton
Copy link
Member

@alexcrichton
Copy link
Member

@bors: retry

On Mon, May 30, 2016 at 11:09 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cross-netbsd
http://buildbot.rust-lang.org/builders/auto-linux-64-cross-netbsd/builds/462


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33921 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95BSOQTH4JWH4M5isQllQW9GQgCk4ks5qG9B-gaJpZM4Io-e0
.

@bors
Copy link
Contributor

bors commented May 31, 2016

⌛ Testing commit 80d7333 with merge 2319d2f...

@bors
Copy link
Contributor

bors commented May 31, 2016

💔 Test failed - auto-linux-64-cross-armhf

@alexcrichton
Copy link
Member

@bors: retry

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 1, 2016
Inline simple Cursor write calls

Implementing the Write trait for Cursors over slices is so light-weight that under some circumstances multiple writes can be fused into a single instruction. In general I think inlining these functions is a good idea because most of the code can be constant-folded and copy-propagated away.

Closes issue rust-lang#33916.

r? @alexcrichton
bors added a commit that referenced this pull request Jun 1, 2016
Rollup of 11 pull requests

- Successful merges: #33385, #33606, #33841, #33892, #33896, #33915, #33921, #33967, #33970, #33973, #33977
- Failed merges:
@bors bors merged commit 80d7333 into rust-lang:master Jun 1, 2016
petertodd added a commit to petertodd/rust that referenced this pull request May 9, 2019
Centril added a commit to Centril/rust that referenced this pull request May 9, 2019
…lice, r=sfackler

Inline some Cursor calls for slices

(Partially) brings back rust-lang#33921

I've noticed in some serialization code I was writing that writes to slices produce much, much, worse code than you'd expect even with optimizations turned on. For example, you'd expect something like this to be zero cost:

```
use std::io::{self, Cursor, Write};

pub fn serialize((a, b): (u64, u64)) -> [u8;8+8] {
    let mut r = [0u8;16];
    {
        let mut w = Cursor::new(&mut r[..]);

        w.write(&a.to_le_bytes()).unwrap();
        w.write(&b.to_le_bytes()).unwrap();
    }
    r
}
```

...but it compiles down to [dozens of instructions](https://rust.godbolt.org/z/bdwDzb) because the `slice_write()` calls aren't inlined, which in turn means `unwrap()` can't be optimized away, and so on.

To be clear, this pull-req isn't sufficient by itself: if we want to go down that path we also need to add `#[inline]`'s to the default implementations for functions like `write_all()` in the `Write` trait and so on, or implement them separately in the `Cursor` impls. But I figured I'd start a conversation about what tradeoffs we're expecting here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants