Skip to content
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

Inline more small functions. #15

Merged
merged 2 commits into from
Sep 30, 2022
Merged

Inline more small functions. #15

merged 2 commits into from
Sep 30, 2022

Conversation

yotamofek
Copy link
Contributor

@yotamofek yotamofek commented Sep 30, 2022

This helps with #7 , but doesn't necessarily close it (might be more places that should be inlined).

This PR adds the #[inline] attr to some the ByteSlice and ByteSliceMut trait impls, along with the aligned_to util fn, which otherwise seem to not get inlined, though they probably should. Tested this on yesterday's nightly, and latest stable. Even lto = "fat" and codegen-units = 1 weren't helpful here.

I only saw concrete need for inlining in the ByteSlice impl for &[u8], but added it for the rest of the impls because it seemed likely they might suffer from the same thing.

The second commit in this PR (which is independent from the first one) just coalesces ByteSlice::as_ptr and ByteSliceMut::as_mut_ptr into provided default impls on the trait declarations. This allows for less repetition and I think was originally an oversight, but I can revert it if it's undesired.

Reproduction code (should be in external crate):

use std::{io::Read, mem::size_of};

use zerocopy::{AsBytes, FromBytes, LayoutVerified};

#[derive(AsBytes, FromBytes)]
#[repr(packed)]
struct MyStruct {
    a: u32,
    b: u16,
}

pub fn test(r: &mut std::io::Cursor<Vec<u8>>) -> std::io::Result<Option<(u32, u16)>> {
    let mut buf = vec![0; size_of::<MyStruct>()];
    r.read_exact(&mut buf)?;

    match LayoutVerified::<&[u8], MyStruct>::new(&buf) {
        Some(s) => Ok(Some((s.a, s.b))),
        None => Ok(None),
    }
}

Assembly before this PR:

➜  zerocopy-repro git:(master) ✗ cargo asm test
   Compiling zerocopy-repro v0.1.0 (/home/yotam/zerocopy-repro)
    Finished release [optimized] target(s) in 0.14s
zerocopy_repro::test:
.Lfunc_begin1:
        .cfi_startproc
        .cfi_personality 155, DW.ref.rust_eh_personality
        .cfi_lsda 27, .Lexception0
        push r15
        .cfi_def_cfa_offset 16
        push r14
        .cfi_def_cfa_offset 24
        push rbx
        .cfi_def_cfa_offset 32
        sub rsp, 16
        .cfi_def_cfa_offset 48
        .cfi_offset rbx, -32
        .cfi_offset r14, -24
        .cfi_offset r15, -16
        mov rbx, rsi
        mov r14, rdi
.Ltmp14:
        mov edi, 6
        mov esi, 1
.Ltmp15:
        call qword ptr [rip + __rust_alloc_zeroed@GOTPCREL]
.Ltmp16:
        test rax, rax
        je .LBB1_13
.Ltmp17:
        mov r15, rax
.Ltmp18:
        mov rdx, qword ptr [rbx + 16]
.Ltmp19:
        mov rax, qword ptr [rbx + 24]
.Ltmp20:
        cmp rax, rdx
        mov rcx, rdx
        cmovb rcx, rax
.Ltmp21:
        sub rdx, rcx
.Ltmp22:
        cmp rdx, 5
        ja .LBB1_3
.Ltmp23:
        lea rax, [rip + .L__unnamed_1]
.Ltmp24:
        mov qword ptr [r14 + 8], rax
        mov dword ptr [r14], 1
.Ltmp25:
        jmp .LBB1_6
.Ltmp26:
.LBB1_3:
        mov rdx, qword ptr [rbx]
.Ltmp27:
        movzx esi, word ptr [rdx + rcx + 4]
        mov word ptr [r15 + 4], si
        mov ecx, dword ptr [rdx + rcx]
.Ltmp28:
        mov dword ptr [r15], ecx
.Ltmp29:
        add rax, 6
        mov qword ptr [rbx + 24], rax
.Ltmp30:
.Ltmp4:
        mov esi, 6
        mov edx, 1
.Ltmp31:
        mov rdi, r15
        call qword ptr [rip + zerocopy::aligned_to@GOTPCREL]
.Ltmp32:
.Ltmp5:
        test al, al
        je .LBB1_5
.Ltmp33:
        mov qword ptr [rsp], r15
        mov qword ptr [rsp + 8], 6
.Ltmp34:
.Ltmp6:
        mov rdi, rsp
.Ltmp35:
        call qword ptr [rip + <&[u8] as zerocopy::ByteSlice>::as_ptr@GOTPCREL]
.Ltmp36:
.Ltmp7:
        mov ebx, dword ptr [rax]
.Ltmp37:
.Ltmp8:
        mov rdi, rsp
.Ltmp38:
        call qword ptr [rip + <&[u8] as zerocopy::ByteSlice>::as_ptr@GOTPCREL]
.Ltmp39:
.Ltmp9:
        movzx eax, word ptr [rax + 4]
        mov dword ptr [r14 + 8], ebx
        mov word ptr [r14 + 12], ax
        movabs rax, 4294967296
        mov qword ptr [r14], rax
        jmp .LBB1_6
.Ltmp40:
.LBB1_5:
        mov qword ptr [r14], 0
.Ltmp41:
.LBB1_6:
        mov esi, 6
        mov edx, 1
        mov rdi, r15
        call qword ptr [rip + __rust_dealloc@GOTPCREL]
        mov rax, r14
        add rsp, 16
        .cfi_def_cfa_offset 32
        pop rbx
        .cfi_def_cfa_offset 24
        pop r14
        .cfi_def_cfa_offset 16
        pop r15
        .cfi_def_cfa_offset 8
        ret
.Ltmp42:
.LBB1_13:
        .cfi_def_cfa_offset 48
        mov edi, 6
        mov esi, 1
        call qword ptr [rip + alloc::alloc::handle_alloc_error@GOTPCREL]
.Ltmp43:
        ud2
.Ltmp44:
.LBB1_11:
.Ltmp10:
        mov rbx, rax
.Ltmp11:
        mov esi, 6
        mov rdi, r15
        call core::ptr::drop_in_place<alloc::vec::Vec<u8>>
.Ltmp12:
.Ltmp45:
        mov rdi, rbx
        call _Unwind_Resume@PLT
        ud2
.Ltmp46:
.LBB1_10:
.Ltmp13:
        call qword ptr [rip + core::panicking::panic_no_unwind@GOTPCREL]
        ud2
.Ltmp47:
.Lfunc_end1:
        .size   zerocopy_repro::test, .Lfunc_end1-zerocopy_repro::test
        .cfi_endproc
➜  zerocopy-repro git:(master) ✗ 

Assembly after this PR:

➜  zerocopy-repro git:(master) ✗ cargo asm test
   Compiling zerocopy-repro v0.1.0 (/home/yotam/zerocopy-repro)
    Finished release [optimized] target(s) in 0.37s
zerocopy_repro::test:
.Lfunc_begin0:
        .cfi_startproc
        push r14
        .cfi_def_cfa_offset 16
        push rbx
        .cfi_def_cfa_offset 24
        push rax
        .cfi_def_cfa_offset 32
        .cfi_offset rbx, -24
        .cfi_offset r14, -16
        mov rbx, rsi
        mov r14, rdi
.Ltmp0:
        mov edi, 6
        mov esi, 1
.Ltmp1:
        call qword ptr [rip + __rust_alloc_zeroed@GOTPCREL]
.Ltmp2:
        test rax, rax
        je .LBB0_5
.Ltmp3:
        mov rsi, qword ptr [rbx + 16]
.Ltmp4:
        mov rcx, qword ptr [rbx + 24]
.Ltmp5:
        cmp rcx, rsi
        mov rdx, rsi
        cmovb rdx, rcx
.Ltmp6:
        sub rsi, rdx
.Ltmp7:
        cmp rsi, 5
        ja .LBB0_4
.Ltmp8:
        lea rcx, [rip + .L__unnamed_1]
.Ltmp9:
        mov qword ptr [r14 + 8], rcx
        mov ecx, 1
.Ltmp10:
        jmp .LBB0_3
.Ltmp11:
.LBB0_4:
        mov rsi, qword ptr [rbx]
.Ltmp12:
        movzx edi, word ptr [rsi + rdx + 4]
        mov word ptr [rax + 4], di
        mov edx, dword ptr [rsi + rdx]
.Ltmp13:
        mov dword ptr [rax], edx
.Ltmp14:
        add rcx, 6
        mov qword ptr [rbx + 24], rcx
.Ltmp15:
        mov ecx, dword ptr [rax]
        movzx edx, word ptr [rax + 4]
        mov dword ptr [r14 + 4], 1
        mov dword ptr [r14 + 8], ecx
        mov word ptr [r14 + 12], dx
        xor ecx, ecx
.Ltmp16:
.LBB0_3:
        mov dword ptr [r14], ecx
.Ltmp17:
        mov esi, 6
        mov edx, 1
        mov rdi, rax
        call qword ptr [rip + __rust_dealloc@GOTPCREL]
.Ltmp18:
        mov rax, r14
        add rsp, 8
        .cfi_def_cfa_offset 24
        pop rbx
.Ltmp19:
        .cfi_def_cfa_offset 16
        pop r14
        .cfi_def_cfa_offset 8
        ret
.Ltmp20:
.LBB0_5:
        .cfi_def_cfa_offset 32
        mov edi, 6
        mov esi, 1
        call qword ptr [rip + alloc::alloc::handle_alloc_error@GOTPCREL]
.Ltmp21:
        ud2
.Ltmp22:
.Lfunc_end0:
        .size   zerocopy_repro::test, .Lfunc_end0-zerocopy_repro::test
        .cfi_endproc

@joshlf
Copy link
Member

joshlf commented Sep 30, 2022

Looks great, thanks so much! Same as with the other PR, can you remove the trailing periods from the commit messages?

I'm still figuring out the details of GitHub's PR system (when zerocopy was in Fuchia's tree, we used Gerrit, which works much differently), so I may end up asking for more changes once I figure out how this is going to show up once it's merged (I have squash merging enabled, but I'm not sure what the commit message is going to be).

@yotamofek
Copy link
Contributor Author

Addressed the CR comment and removed trailing dots from commit messages.
Don't hesitate to ask for any more changes :)
I'm glad this crate is on GH, makes collaboration easier.

Just FYI: when you use the "squash" option for PRs on GH, the commit message becomes the PR's title as the first line, along with every commit message in the following lines. (But the outcome is just one commit, still.) I think whoever does the actual merge can edit the commit message just before it gets committed.

@joshlf
Copy link
Member

joshlf commented Sep 30, 2022

Ah that's good to know, thanks! And I just updated CONTRIBUTING.md with code guidelines.

@joshlf joshlf merged commit a727aca into google:main Sep 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants