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

Use NonNull in slice::Iter and slice::IterMut. #67588

Merged
merged 1 commit into from
Dec 28, 2019
Merged

Use NonNull in slice::Iter and slice::IterMut. #67588

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 25 additions & 22 deletions src/libcore/slice/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use crate::mem;
use crate::ops::{self, FnMut, Range};
use crate::option::Option;
use crate::option::Option::{None, Some};
use crate::ptr;
use crate::ptr::{self, NonNull};
use crate::result::Result;
use crate::result::Result::{Err, Ok};

Expand Down Expand Up @@ -628,7 +628,7 @@ impl<T> [T] {
ptr.add(self.len())
};

Iter { ptr, end, _marker: marker::PhantomData }
Iter { ptr: NonNull::new_unchecked(ptr as *mut T), end, _marker: marker::PhantomData }
}
}

Expand Down Expand Up @@ -656,7 +656,7 @@ impl<T> [T] {
ptr.add(self.len())
};

IterMut { ptr, end, _marker: marker::PhantomData }
IterMut { ptr: NonNull::new_unchecked(ptr), end, _marker: marker::PhantomData }
}
}

Expand Down Expand Up @@ -3095,7 +3095,7 @@ macro_rules! is_empty {
// The way we encode the length of a ZST iterator, this works both for ZST
// and non-ZST.
($self: ident) => {
$self.ptr == $self.end
$self.ptr.as_ptr() as *const T == $self.end
};
}
// To get rid of some bounds checks (see `position`), we compute the length in a somewhat
Expand All @@ -3105,17 +3105,17 @@ macro_rules! len {
#![allow(unused_unsafe)] // we're sometimes used within an unsafe block

let start = $self.ptr;
let size = size_from_ptr(start);
let size = size_from_ptr(start.as_ptr());
if size == 0 {
// This _cannot_ use `unchecked_sub` because we depend on wrapping
// to represent the length of long ZST slice iterators.
($self.end as usize).wrapping_sub(start as usize)
($self.end as usize).wrapping_sub(start.as_ptr() as usize)
} else {
// We know that `start <= end`, so can do better than `offset_from`,
// which needs to deal in signed. By setting appropriate flags here
// we can tell LLVM this, which helps it remove bounds checks.
// SAFETY: By the type invariant, `start <= end`
let diff = unsafe { unchecked_sub($self.end as usize, start as usize) };
let diff = unsafe { unchecked_sub($self.end as usize, start.as_ptr() as usize) };
// By also telling LLVM that the pointers are apart by an exact
// multiple of the type size, it can optimize `len() == 0` down to
// `start == end` instead of `(end - start) < size`.
Expand Down Expand Up @@ -3161,7 +3161,7 @@ macro_rules! iterator {
// Helper function for creating a slice from the iterator.
#[inline(always)]
fn make_slice(&self) -> &'a [T] {
unsafe { from_raw_parts(self.ptr, len!(self)) }
unsafe { from_raw_parts(self.ptr.as_ptr(), len!(self)) }
}

// Helper function for moving the start of the iterator forwards by `offset` elements,
Expand All @@ -3171,10 +3171,10 @@ macro_rules! iterator {
unsafe fn post_inc_start(&mut self, offset: isize) -> * $raw_mut T {
if mem::size_of::<T>() == 0 {
zst_shrink!(self, offset);
self.ptr
self.ptr.as_ptr()
} else {
let old = self.ptr;
self.ptr = self.ptr.offset(offset);
let old = self.ptr.as_ptr();
self.ptr = NonNull::new_unchecked(self.ptr.as_ptr().offset(offset));
old
}
}
Expand All @@ -3186,7 +3186,7 @@ macro_rules! iterator {
unsafe fn pre_dec_end(&mut self, offset: isize) -> * $raw_mut T {
if mem::size_of::<T>() == 0 {
zst_shrink!(self, offset);
self.ptr
self.ptr.as_ptr()
} else {
self.end = self.end.offset(-offset);
self.end
Expand Down Expand Up @@ -3215,7 +3215,7 @@ macro_rules! iterator {
fn next(&mut self) -> Option<$elem> {
// could be implemented with slices, but this avoids bounds checks
unsafe {
assume(!self.ptr.is_null());
assume(!self.ptr.as_ptr().is_null());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now implied by the NonNull type, right? Should we remove this assume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is it's an optimization when it comes to code generation, not layout optimization. But maybe rustc is smart enough about that? I'm not that familiar with how NonNull works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we could remove it. Is that example good enough evidence? Or should we reach out to someone who knows better?

Copy link
Contributor

Choose a reason for hiding this comment

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

The "this pointer is non-null" information added by rustc to LLVM IR automatically is limited to particular positions (function parameters, function return values, loads from memory) which can get eliminated by optimizations (such as inlining and SROA) before they have served their purpose. In contrast, assumes stay around basically for the entire optimization pipeline. So I am not confident that we can remove this assume without regressions. You're welcome to try to evaluate that on real world code (the example linked above is far too isolated, I would recommend history spelunking to find the benchmarks that motivated adding the assume in the first place) but I would recommend doing that in a separate PR.

if mem::size_of::<T>() != 0 {
assume(!self.end.is_null());
}
Expand Down Expand Up @@ -3245,9 +3245,12 @@ macro_rules! iterator {
if mem::size_of::<T>() == 0 {
// We have to do it this way as `ptr` may never be 0, but `end`
// could be (due to wrapping).
self.end = self.ptr;
self.end = self.ptr.as_ptr();
} else {
self.ptr = self.end;
unsafe {
// End can't be 0 if T isn't ZST because ptr isn't 0 and end >= ptr
self.ptr = NonNull::new_unchecked(self.end as *mut T);
}
}
return None;
}
Expand Down Expand Up @@ -3308,7 +3311,7 @@ macro_rules! iterator {
fn next_back(&mut self) -> Option<$elem> {
// could be implemented with slices, but this avoids bounds checks
unsafe {
assume(!self.ptr.is_null());
assume(!self.ptr.as_ptr().is_null());
if mem::size_of::<T>() != 0 {
assume(!self.end.is_null());
}
Expand All @@ -3324,7 +3327,7 @@ macro_rules! iterator {
fn nth_back(&mut self, n: usize) -> Option<$elem> {
if n >= len!(self) {
// This iterator is now empty.
self.end = self.ptr;
self.end = self.ptr.as_ptr();
return None;
}
// We are in bounds. `pre_dec_end` does the right thing even for ZSTs.
Expand Down Expand Up @@ -3365,7 +3368,7 @@ macro_rules! iterator {
/// [slices]: ../../std/primitive.slice.html
#[stable(feature = "rust1", since = "1.0.0")]
pub struct Iter<'a, T: 'a> {
ptr: *const T,
ptr: NonNull<T>,
end: *const T, // If T is a ZST, this is actually ptr+len. This encoding is picked so that
// ptr == end is a quick test for the Iterator being empty, that works
// for both ZST and non-ZST.
Expand Down Expand Up @@ -3467,7 +3470,7 @@ impl<T> AsRef<[T]> for Iter<'_, T> {
/// [slices]: ../../std/primitive.slice.html
#[stable(feature = "rust1", since = "1.0.0")]
pub struct IterMut<'a, T: 'a> {
ptr: *mut T,
ptr: NonNull<T>,
end: *mut T, // If T is a ZST, this is actually ptr+len. This encoding is picked so that
// ptr == end is a quick test for the Iterator being empty, that works
// for both ZST and non-ZST.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can they both be NonNull<T>? IIUC it's an invariant that ptr <= end, so a null end is invalid.

We don't use multiple niches yet, but a future optimization might, and it could help communicate the invariant better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to a comment further down end might be 0 in case of ZST.

Thanks for review, BTW! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah of course: a ZST slice can have an arbitrary starting pointer, and len is encoded as the wrapping subtraction of end - start. Thus a slice of length 1 with the max-int pointer results in end = 0.

You know, now that we have unions a simpler way to do all this could be:

union LenOrEnd<T> {
    end: NonNull<T>,
    len: usize,
}

pub struct IterMut<'a, T> {
    marker: PhantomData<&'a mut T>,
    start: NonNull<T>,
    len_or_end: LenOrEnd<T>,
}

A union of SizedIterMut and ZeroSizedIterMut is another possibility. Except that currently union's don't participate in layout optimization. :/

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll bet you this will have slightly better performance too by representing len directly rather than needing to do arithmetic to derive it.

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 was thinking that the code might be improved in different ways too. I just think that we should go with smaller increments. The PR already improves something, new improvements could be in the next PR.

Anyway, I think the layout would have to be determined in type, not in union in order to enable optimizations. I'm not an expert but I suspect that making rustc smart enough to understand which part of the union is used is based on type and conditionally optimize layout would be very difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree that an incremental improvement is fine right now.

The above does get the NonNull<T> layout optimization as the NonNull<T> is in the struct, rather than the union: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=82eb70c78825be8dd5e48fd5d8d521cc

Expand Down Expand Up @@ -3522,7 +3525,7 @@ impl<'a, T> IterMut<'a, T> {
/// ```
#[stable(feature = "iter_to_slice", since = "1.4.0")]
pub fn into_slice(self) -> &'a mut [T] {
unsafe { from_raw_parts_mut(self.ptr, len!(self)) }
unsafe { from_raw_parts_mut(self.ptr.as_ptr(), len!(self)) }
}

/// Views the underlying data as a subslice of the original data.
Expand Down Expand Up @@ -5682,7 +5685,7 @@ impl_marker_for!(BytewiseEquality,
#[doc(hidden)]
unsafe impl<'a, T> TrustedRandomAccess for Iter<'a, T> {
unsafe fn get_unchecked(&mut self, i: usize) -> &'a T {
&*self.ptr.add(i)
&*self.ptr.as_ptr().add(i)
}
fn may_have_side_effect() -> bool {
false
Expand All @@ -5692,7 +5695,7 @@ unsafe impl<'a, T> TrustedRandomAccess for Iter<'a, T> {
#[doc(hidden)]
unsafe impl<'a, T> TrustedRandomAccess for IterMut<'a, T> {
unsafe fn get_unchecked(&mut self, i: usize) -> &'a mut T {
&mut *self.ptr.add(i)
&mut *self.ptr.as_ptr().add(i)
}
fn may_have_side_effect() -> bool {
false
Expand Down