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.

Sign up for GitHub

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

Jump to bottom

Add inline tags to UninitSlice methods #443

Merged
merged 3 commits into from
Apr 10, 2021
Merged

Add inline tags to UninitSlice methods #443

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
645bd69
Add inline tags to UninitSlice methods
danburkert Nov 9, 2020
70c5525
Update src/buf/uninit_slice.rs
danburkert Nov 9, 2020
7c8af6d
ptr::add -> index
danburkert Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/buf/uninit_slice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl UninitSlice {
///
/// let slice = unsafe { UninitSlice::from_raw_parts_mut(ptr, len) };
/// ```
#[inline]
pub unsafe fn from_raw_parts_mut<'a>(ptr: *mut u8, len: usize) -> &'a mut UninitSlice {
let maybe_init: &mut [MaybeUninit<u8>] =
core::slice::from_raw_parts_mut(ptr as *mut _, len);
Expand All @@ -64,10 +65,11 @@ impl UninitSlice {
///
/// assert_eq!(b"boo", &data[..]);
/// ```
#[inline]
pub fn write_byte(&mut self, index: usize, byte: u8) {
assert!(index < self.len());

unsafe { self[index..].as_mut_ptr().write(byte) }
unsafe { self.as_mut_ptr().add(index).write(byte) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect the bounds check to have been elided, since it's asserted just before... It's not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check the assembly, but empirically I found in #442 that a similar change had a measurable impact on microbenchmarks in the prost repo. It's very possible that the issue there was that the UninitSlice index call was not inlined, and so the similar double check wasn't eligible to be optimized away. Realistically I'm not going to have time to read the ASM here, but I'm happy to back out this part of the change if you'd prefer that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using s[i] vs s.as_ptr().add(i) gets me the same thing with the assert in the function:

example::index:
        push    rax
        cmp     rsi, rdx
        jbe     .LBB7_1
        mov     al, byte ptr [rdi + rdx]
        pop     rcx
        ret
.LBB7_1:
        lea     rdi, [rip + .L__unnamed_4]
        call    std::panicking::begin_panic
        ud2

example::ptr:
        push    rax
        cmp     rsi, rdx
        jbe     .LBB8_2
        mov     al, byte ptr [rdi + rdx]
        pop     rcx
        ret
.LBB8_2:
        lea     rdi, [rip + .L__unnamed_5]
        call    std::panicking::begin_panic
        ud2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, would you like me to switch it back?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems reasonable to me to keep to safe things if they make no performance difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, updated.

}

/// Copies bytes from `src` into `self`.
Expand All @@ -90,6 +92,7 @@ impl UninitSlice {
///
/// assert_eq!(b"bar", &data[..]);
/// ```
#[inline]
pub fn copy_from_slice(&mut self, src: &[u8]) {
use core::ptr;

Expand All @@ -116,6 +119,7 @@ impl UninitSlice {
/// let mut slice = &mut data[..];
/// let ptr = BufMut::bytes_mut(&mut slice).as_mut_ptr();
/// ```
#[inline]
pub fn as_mut_ptr(&mut self) -> *mut u8 {
self.0.as_mut_ptr() as *mut _
}
Expand All @@ -133,6 +137,7 @@ impl UninitSlice {
///
/// assert_eq!(len, 3);
/// ```
#[inline]
pub fn len(&self) -> usize {
self.0.len()
}
Expand All @@ -150,13 +155,15 @@ macro_rules! impl_index {
impl Index<$t> for UninitSlice {
type Output = UninitSlice;

#[inline]
fn index(&self, index: $t) -> &UninitSlice {
let maybe_uninit: &[MaybeUninit<u8>] = &self.0[index];
unsafe { &*(maybe_uninit as *const [MaybeUninit<u8>] as *const UninitSlice) }
}
}

impl IndexMut<$t> for UninitSlice {
#[inline]
fn index_mut(&mut self, index: $t) -> &mut UninitSlice {
let maybe_uninit: &mut [MaybeUninit<u8>] = &mut self.0[index];
unsafe { &mut *(maybe_uninit as *mut [MaybeUninit<u8>] as *mut UninitSlice) }
Expand Down