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.

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

Implement O(1) slice::Iter{,Mut} methods. #24701

Merged
merged 3 commits into from
Apr 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 79 additions & 25 deletions src/libcore/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,36 @@ impl<'a, T> IntoIterator for &'a mut [T] {
}
}

#[inline(always)]
fn size_from_ptr<T>(_: *const T) -> usize {
mem::size_of::<T>()
}


// Use macro to be generic over const/mut
macro_rules! slice_offset {
($ptr:expr, $by:expr) => {{
let ptr = $ptr;
if size_from_ptr(ptr) == 0 {
transmute(ptr as usize + $by)
} else {
ptr.offset($by)
}
}};
}

macro_rules! slice_ref {
($ptr:expr) => {{
let ptr = $ptr;
if size_from_ptr(ptr) == 0 {
// Use a non-null pointer value
&mut *(1 as *mut _)
} else {
transmute(ptr)
}
}};
}

// The shared definition of the `Iter` and `IterMut` iterators
macro_rules! iterator {
(struct $name:ident -> $ptr:ty, $elem:ty) => {
Expand All @@ -641,20 +671,9 @@ macro_rules! iterator {
if self.ptr == self.end {
None
} else {
if mem::size_of::<T>() == 0 {
// purposefully don't use 'ptr.offset' because for
// vectors with 0-size elements this would return the
// same pointer.
self.ptr = transmute(self.ptr as usize + 1);

// Use a non-null pointer value
Some(&mut *(1 as *mut _))
} else {
let old = self.ptr;
self.ptr = self.ptr.offset(1);

Some(transmute(old))
}
let old = self.ptr;
self.ptr = slice_offset!(self.ptr, 1);
Some(slice_ref!(old))
}
}
}
Expand All @@ -666,6 +685,22 @@ macro_rules! iterator {
let exact = diff / (if size == 0 {1} else {size});
(exact, Some(exact))
}

#[inline]
fn count(self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this does change the semantics of count from the default implementation which exhausts the iterator. Perhaps this could modify start to equal end so retain equivalent semantics?

Copy link
Member

Choose a reason for hiding this comment

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

Er actually this applies to all of the functions below as well as in the semantics have changed with respect to the default implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm mistaken, this consumes the iterator so it shouldn't matter. Is that not the case?

Copy link
Member

Choose a reason for hiding this comment

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

I think that size_hint doesn't consume the iterator, so I don't think this consumes the iterator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No but the function itself (count(self)) does.

Copy link
Member

Choose a reason for hiding this comment

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

Oh dear that is a very good point! After rereading nth I see it's updating the pointers already, in which case you can just completely ignore me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

it changes the behavior if this was called on a by_ref() iterator. But these have odd behavior anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but then this count won't be called (it can't because it needs to consume the iterator by value). Instead the default (slow) Iterator count will be called.

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole comment thread says to me that we need a specific consume method for just running through an iterator, because people abuse count for this today.

self.size_hint().0
}

#[inline]
fn nth(&mut self, n: usize) -> Option<$elem> {
// Call helper method. Can't put the definition here because mut versus const.
self.iter_nth(n)
}

#[inline]
fn last(mut self) -> Option<$elem> {
self.next_back()
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand All @@ -679,17 +714,8 @@ macro_rules! iterator {
if self.end == self.ptr {
None
} else {
if mem::size_of::<T>() == 0 {
// See above for why 'ptr.offset' isn't used
self.end = transmute(self.end as usize - 1);

// Use a non-null pointer value
Some(&mut *(1 as *mut _))
} else {
self.end = self.end.offset(-1);

Some(transmute(self.end))
}
self.end = slice_offset!(self.end, -1);
Some(slice_ref!(self.end))
}
}
}
Expand Down Expand Up @@ -785,6 +811,20 @@ impl<'a, T> Iter<'a, T> {
pub fn as_slice(&self) -> &'a [T] {
make_slice!(T => &'a [T]: self.ptr, self.end)
}

// Helper function for Iter::nth
fn iter_nth(&mut self, n: usize) -> Option<&'a T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be possible to share code between the iter_nth implementations but I don't know how safe transmuting/casting between const/mut pointers is...

match self.as_slice().get(n) {
Some(elem_ref) => unsafe {
self.ptr = slice_offset!(elem_ref as *const _, 1);
Some(slice_ref!(elem_ref))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% clear on why the slice_ref macro is needed here -- doesn't elem_ref already have the correct type and semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hence my question:

Why do slice iterators return &mut *(1 as *mut) (references to 0x1) instead of the actual pointer into the slice (self.ptr)?

elem_ref is the actual pointer but slice_ref! will convert it to &mut *(1 as *mut _) if the type is zero sized.

Copy link
Member

Choose a reason for hiding this comment

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

This may be a relic of an era since long past, I think elem_ref should be usable as-is today?

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 wonder if making this an actual pointer will have a performance impact. As-is, iterators over zero-sized types always yield constants.

Copy link
Member

Choose a reason for hiding this comment

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

Hm that's true, I think it's fine to investigate this on the side though as the rest of this PR should be good to go, thanks @Stebalien!

},
None => {
self.ptr = self.end;
None
}
}
}
}

iterator!{struct Iter -> *const T, &'a T}
Expand Down Expand Up @@ -914,6 +954,20 @@ impl<'a, T> IterMut<'a, T> {
pub fn into_slice(self) -> &'a mut [T] {
make_mut_slice!(T => &'a mut [T]: self.ptr, self.end)
}

// Helper function for IterMut::nth
fn iter_nth(&mut self, n: usize) -> Option<&'a mut T> {
match make_mut_slice!(T => &'a mut [T]: self.ptr, self.end).get_mut(n) {
Some(elem_ref) => unsafe {
self.ptr = slice_offset!(elem_ref as *mut _, 1);
Some(slice_ref!(elem_ref))
},
None => {
self.ptr = self.end;
None
}
}
}
}

iterator!{struct IterMut -> *mut T, &'a mut T}
Expand Down
31 changes: 31 additions & 0 deletions src/libcoretest/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,34 @@ fn iterator_to_slice() {
test!([1u8,2,3]);
test!([(),(),()]);
}

#[test]
fn test_iterator_nth() {
let v: &[_] = &[0, 1, 2, 3, 4];
for i in 0..v.len() {
assert_eq!(v.iter().nth(i).unwrap(), &v[i]);
}
assert_eq!(v.iter().nth(v.len()), None);

let mut iter = v.iter();
assert_eq!(iter.nth(2).unwrap(), &v[2]);
assert_eq!(iter.nth(1).unwrap(), &v[4]);
}

#[test]
fn test_iterator_last() {
let v: &[_] = &[0, 1, 2, 3, 4];
assert_eq!(v.iter().last().unwrap(), &4);
assert_eq!(v[..1].iter().last().unwrap(), &0);
}

#[test]
fn test_iterator_count() {
let v: &[_] = &[0, 1, 2, 3, 4];
assert_eq!(v.iter().count(), 5);

let mut iter2 = v.iter();
iter2.next();
iter2.next();
assert_eq!(iter2.count(), 3);
}