Remove Populatable and BytesConvertable traits #2019

rphmeier · 2016-08-30T10:46:15Z

Our safety guarantees around Populatable were really weak and BytesConvertable just duplicated AsRef<[u8]>. I also cleaned up the fixed_binary_size macro to require the type be Copy, since again that's necessary for strong safety guarantees about not serializing types containing pointers over IPC.

Goes after #2018

debris · 2016-08-30T11:08:58Z

I like the changes so far 👍

coveralls · 2016-08-30T11:33:50Z

Coverage increased (+0.02%) to 86.361% when pulling 08594a8 on remove-populatable into 6da60af on master.

NikVolf · 2016-08-30T12:18:57Z

ipc/rpc/src/binary.rs

 				};
-				let mut res: Self = unsafe { ::std::mem::uninitialized() };


if the size is checked in advance, how fill with zero is safer than just unintialized (which is no-op)?

I don't think it's strictly safer but in general it's better practice than using uninitialized. LLVM can optimize out the zero-ing so I doubt we'd see a performance penalty. uninitialized imo should be used only for FFI and dirty tricks.

Well it's most used operation in ipc serialization
Every hash, every vector element go through it, so we better be sure imo

coveralls · 2016-08-30T13:13:41Z

Coverage decreased (-0.02%) to 86.351% when pulling 2c862af on remove-populatable into 6f321d9 on master.

coveralls · 2016-08-31T11:22:09Z

Coverage decreased (-0.02%) to 86.314% when pulling aaf21d1 on remove-populatable into a88440e on master.

rphmeier · 2016-08-31T11:49:36Z

A couple of examples:

BinaryConvertable::from_bytes for BlockError, generated using the binary_fixed_size! macro

Before:

    .section    ".text._ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h68bda87c1f8302ffE","ax",@progbits
    .globl  _ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h68bda87c1f8302ffE
    .align  16, 0x90
    .type   _ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h68bda87c1f8302ffE,@function
_ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h68bda87c1f8302ffE:
.Lfunc_begin1463:
    .cfi_startproc
    pushq   %rbp
.Ltmp26724:
    .cfi_def_cfa_offset 16
.Ltmp26725:
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
.Ltmp26726:
    .cfi_def_cfa_register %rbp
    pushq   %rbx
    pushq   %rax
.Ltmp26727:
    .cfi_offset %rbx, -24
    movq    %rdi, %rbx
    leaq    8(%rbx), %rdi
    .loc    85 678 0
    cmpq    $520, %rdx
    jne .LBB1463_1
    .loc    207 12 0
    movl    $520, %edx
    callq   memcpy@PLT
    xorl    %eax, %eax
    jmp .LBB1463_3
.LBB1463_1:
    .loc    207 9 0
    movl    $520, %esi
    callq   _ZN11ethcore_ipc6binary18BinaryConvertError4size17hea6e073b993df4fbE@PLT
    movl    $1, %eax
.LBB1463_3:
    .loc    207 12 0
    movq    %rax, (%rbx)
    .loc    207 12 0 is_stmt 0
    movq    %rbx, %rax
    addq    $8, %rsp
    popq    %rbx
    popq    %rbp
    retq

After:

    .section    ".text._ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h8374b5e05199295bE","ax",@progbits
    .globl  _ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h8374b5e05199295bE
    .align  16, 0x90
    .type   _ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h8374b5e05199295bE,@function
_ZN85_$LT$ethcore..error..BlockError$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h8374b5e05199295bE:
.Lfunc_begin1465:
    .cfi_startproc
    pushq   %rbp
.Ltmp26802:
    .cfi_def_cfa_offset 16
.Ltmp26803:
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
.Ltmp26804:
    .cfi_def_cfa_register %rbp
    pushq   %rbx
    pushq   %rax
.Ltmp26805:
    .cfi_offset %rbx, -24
    movq    %rdi, %rbx
    leaq    8(%rbx), %rdi
    .loc    85 678 0
    cmpq    $520, %rdx
    jne .LBB1465_1
    .loc    207 13 0
    movl    $520, %edx
    callq   memcpy@PLT
    xorl    %eax, %eax
    jmp .LBB1465_3
.LBB1465_1:
    .loc    207 9 0
    movl    $520, %esi
    callq   _ZN11ethcore_ipc6binary18BinaryConvertError4size17hea6e073b993df4fbE@PLT
    movl    $1, %eax
.LBB1465_3:
    .loc    207 13 0
    movq    %rax, (%rbx)
    .loc    207 13 0 is_stmt 0
    movq    %rbx, %rax
    addq    $8, %rsp
    popq    %rbx
    popq    %rbp
    retq

<H2048 as BinaryConvertable>::from_bytes

Before:

    .section    ".text._ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17ha3b6e8fd82cf9e0dE","ax",@progbits
    .globl  _ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17ha3b6e8fd82cf9e0dE
    .align  16, 0x90
    .type   _ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17ha3b6e8fd82cf9e0dE,@function
_ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17ha3b6e8fd82cf9e0dE:
.Lfunc_begin75:
    .cfi_startproc
    pushq   %rbp
.Ltmp308:
    .cfi_def_cfa_offset 16
.Ltmp309:
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
.Ltmp310:
    .cfi_def_cfa_register %rbp
    pushq   %rbx
    pushq   %rax
.Ltmp311:
    .cfi_offset %rbx, -24
    movq    %rdi, %rbx
    .loc    10 678 0
    cmpq    $256, %rdx
    jne .LBB75_1
    .loc    4 717 0
    leaq    1(%rbx), %rdi
    movl    $256, %edx
    callq   memcpy@PLT
    xorl    %eax, %eax
    jmp .LBB75_3
.LBB75_1:
    .loc    6 69 0
    movq    $1, 8(%rbx)
    xorps   %xmm0, %xmm0
    .loc    7 297 0
    movups  %xmm0, 16(%rbx)
    .loc    4 50 0
    movq    $256, 40(%rbx)
    .loc    4 51 0
    movq    %rdx, 48(%rbx)
    movb    $0, 32(%rbx)
    movb    $1, %al
.LBB75_3:
    .loc    4 717 0
    movb    %al, (%rbx)
    .loc    4 718 0
    movq    %rbx, %rax
    addq    $8, %rsp
    popq    %rbx
    popq    %rbp
    retq

After:

    .section    ".text._ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h99ac2fdb585a06a9E","ax",@progbits
    .globl  _ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h99ac2fdb585a06a9E
    .align  16, 0x90
    .type   _ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h99ac2fdb585a06a9E,@function
_ZN78_$LT$bigint..hash..H2048$u20$as$u20$ethcore_ipc..binary..BinaryConvertable$GT$10from_bytes17h99ac2fdb585a06a9E:
.Lfunc_begin75:
    .cfi_startproc
    pushq   %rbp
.Ltmp308:
    .cfi_def_cfa_offset 16
.Ltmp309:
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
.Ltmp310:
    .cfi_def_cfa_register %rbp
    pushq   %rbx
    pushq   %rax
.Ltmp311:
    .cfi_offset %rbx, -24
    movq    %rdi, %rbx
    .loc    10 678 0
    cmpq    $256, %rdx
    jne .LBB75_1
    .loc    4 723 0
    leaq    1(%rbx), %rdi
    movl    $256, %edx
    callq   memcpy@PLT
    xorl    %eax, %eax
    jmp .LBB75_3
.LBB75_1:
    .loc    6 69 0
    movq    $1, 8(%rbx)
    xorps   %xmm0, %xmm0
    .loc    7 297 0
    movups  %xmm0, 16(%rbx)
    .loc    4 49 0
    movq    $256, 40(%rbx)
    .loc    4 50 0
    movq    %rdx, 48(%rbx)
    movb    $0, 32(%rbx)
    movb    $1, %al
.LBB75_3:
    .loc    4 723 0
    movb    %al, (%rbx)
    .loc    4 724 0
    movq    %rbx, %rax
    addq    $8, %rsp
    popq    %rbx
    popq    %rbp
    retq

rphmeier · 2016-08-31T11:50:05Z

@NikVolf As you can see the zeroing has been optimized out and has no effect on the produced code.

rphmeier added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Aug 30, 2016

get rid of populatable and bytesconvertable traits

2c862af

rphmeier force-pushed the remove-populatable branch from 08594a8 to 2c862af Compare August 30, 2016 12:04

NikVolf reviewed Aug 30, 2016
View reviewed changes

Merge branch 'master' into remove-populatable

aaf21d1

rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 31, 2016

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 31, 2016

arkpar merged commit 9a5668f into master Sep 1, 2016

debris deleted the remove-populatable branch September 1, 2016 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Populatable and BytesConvertable traits #2019

Remove Populatable and BytesConvertable traits #2019

rphmeier commented Aug 30, 2016

debris commented Aug 30, 2016

coveralls commented Aug 30, 2016

NikVolf Aug 30, 2016

rphmeier Aug 30, 2016

NikVolf Aug 30, 2016

coveralls commented Aug 30, 2016 •

edited

Loading

coveralls commented Aug 31, 2016

rphmeier commented Aug 31, 2016 •

edited

Loading

rphmeier commented Aug 31, 2016

		};
		let mut res: Self = unsafe { ::std::mem::uninitialized() };

Remove Populatable and BytesConvertable traits #2019

Remove Populatable and BytesConvertable traits #2019

Conversation

rphmeier commented Aug 30, 2016

debris commented Aug 30, 2016

coveralls commented Aug 30, 2016

NikVolf Aug 30, 2016

Choose a reason for hiding this comment

rphmeier Aug 30, 2016

Choose a reason for hiding this comment

NikVolf Aug 30, 2016

Choose a reason for hiding this comment

coveralls commented Aug 30, 2016 • edited Loading

coveralls commented Aug 31, 2016

rphmeier commented Aug 31, 2016 • edited Loading

rphmeier commented Aug 31, 2016

coveralls commented Aug 30, 2016 •

edited

Loading

rphmeier commented Aug 31, 2016 •

edited

Loading