Skip to content

Inline read/write functions for Cursors #33916

New issue

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

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

Already on GitHub? # to your account

Closed
jameysharp opened this issue May 27, 2016 · 3 comments
Closed

Inline read/write functions for Cursors #33916

jameysharp opened this issue May 27, 2016 · 3 comments

Comments

@jameysharp
Copy link
Contributor

It would be nice if the Read and Write implementations for std::io::Cursor were inlined. Consider this program:

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

fn main() {
        let mut arr = [0u8; 4];
        let mut c = Cursor::new(&mut arr as &mut [u8]);
        let _ = c.write(&[42, 7]);
        let _ = c.write(&[127, 255]);
        println!("{:?}", c.into_inner());
}

If the write calls are inlined, then all the position computations are constant-folded and the writes compile to exactly what I'd hope for:

movl   $0xff7f072a,0x5c(%rsp)

I tested using rustc -C opt-level=2 -C lto, using rustc 1.10.0-nightly (e0fd34b 2016-05-09). I think that LTO is a pretty close approximation here to the effect that #[inline] would have, but I guess that assumption needs testing. (By the way: oddly, at opt-level=3 I got worse code.)

Without LTO, I get about 84 instructions for this sequence, counting the ones in main as well as the implementation of write itself.

That said, the Write implementation for Cursor<Vec<u8>> is more complicated and maybe shouldn't be inlined. But I think this request at least applies to the implementations for Cursor<&'a mut [u8]> and Cursor<Box<[u8]>>, and probably to the Read implementation for Cursor<T> where T: AsRef<[u8]>.

Does this sound sensible?

@alexcrichton
Copy link
Member

The generic ones should in theory already be inlined due to the generics themselves, but feel free to send a PR to inline some of the others!

jameysharp added a commit to jameysharp/rust that referenced this issue May 28, 2016
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.
@jameysharp
Copy link
Contributor Author

I didn't understand what you meant by "The generic ones should in theory already be inlined", but I guessed you meant the Read implementation since it's for Cursor<T>? So I tested that and discovered that sometimes optimizers are kind of hilariously good:

use std::io::{Cursor,Read};

fn main() {
        let arr : [u8; 4] = [0xff, 0xee, 0xdd, 0xcc];
        let mut c = Cursor::new(&arr as &[u8]);
        let mut arr2 = [0u8; 4];
        let _ = c.read(&mut arr2[0..2]);
        let _ = c.read(&mut arr2[2..4]);
        assert!(arr == arr2);
}

The body of main compiled (with rustc -C opt-level=2) down to:

push   %rax
movl   $0xccddeeff,0x4(%rsp)
movl   $0x0,(%rsp)
movl   $0xccddeeff,(%rsp)
cmpl   $0xccddeeff,0x4(%rsp)
jne    5243 <_ZN4read4main17h1a4d109b8431532bE+0x23>
pop    %rax
retq   

(where 5243 is the address of the unwind call). I'm a little surprised LLVM didn't constant-fold the comparison and reduce the whole thing to a single retq, but still, I'm impressed.

So anyway I've submitted a pull request that only inlines write for two of the three implementations for Cursors, and doesn't change read since it apparently doesn't need it.

Manishearth added a commit to Manishearth/rust that referenced this issue 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
@pmarcelll
Copy link
Contributor

The PR was merged, but bors didn't close this issue.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants