From 7a666df5fa6295e82d4350a6eb105c0370aca7a1 Mon Sep 17 00:00:00 2001 From: Colin Sherratt Date: Sun, 19 Oct 2014 16:19:07 -0400 Subject: [PATCH 1/6] Expand the benchmarking and unit test suite for ring_buf. -Adds unit tests for fn get() and fn get_mut() which are currently untested -Adds unit tests to verify growth of the ringbuffer when reserve is called. -Adds unit tests to confirm that dropping of items is correct Move ringbuf to use a raw buffer instead of Option --- src/libcollections/ring_buf.rs | 685 +++++++++++++++++++++++---------- 1 file changed, 486 insertions(+), 199 deletions(-) diff --git a/src/libcollections/ring_buf.rs b/src/libcollections/ring_buf.rs index 46fd86df98701..fcf9e33cc9d9e 100644 --- a/src/libcollections/ring_buf.rs +++ b/src/libcollections/ring_buf.rs @@ -15,14 +15,19 @@ use core::prelude::*; -use core::cmp; use core::default::Default; use core::fmt; use core::iter; -use core::slice; +use core::raw::Slice as RawSlice; +use core::ptr; +use core::kinds::marker; +use core::mem; +use core::num; + use std::hash::{Writer, Hash}; +use std::cmp; -use vec::Vec; +use alloc::heap; static INITIAL_CAPACITY: uint = 8u; // 2^3 static MINIMUM_CAPACITY: uint = 2u; @@ -33,11 +38,37 @@ static MINIMUM_CAPACITY: uint = 2u; /// `RingBuf` is a circular buffer that implements `Deque`. -#[deriving(Clone)] pub struct RingBuf { - nelts: uint, - lo: uint, - elts: Vec> + // tail and head are pointers into the buffer. Tail always points + // to the first element that could be read, Head always points + // to where data should be written. + // If tail == head the buffer is empty. The length of the ringbuf + // is defined as the distance between the two. + + tail: uint, + head: uint, + cap: uint, + ptr: *mut T +} + +impl Clone for RingBuf { + fn clone(&self) -> RingBuf { + self.iter().map(|t| t.clone()).collect() + } +} + +#[unsafe_destructor] +impl Drop for RingBuf { + fn drop(&mut self) { + self.clear(); + unsafe { + if mem::size_of::() != 0 { + heap::deallocate(self.ptr as *mut u8, + self.cap * mem::size_of::(), + mem::min_align_of::()) + } + } + } } impl Default for RingBuf { @@ -45,6 +76,30 @@ impl Default for RingBuf { fn default() -> RingBuf { RingBuf::new() } } +impl RingBuf { + /// Turn ptr into a slice + #[inline] + unsafe fn buffer_as_slice(&self) -> &[T] { + mem::transmute(RawSlice { data: self.ptr as *const T, len: self.cap }) + } + + /// Moves an element out of the buffer + #[inline] + unsafe fn buffer_read(&mut self, off: uint) -> T { + ptr::read(self.ptr.offset(off as int) as *const T) + } + + /// Writes an element into the buffer, moving it. + #[inline] + unsafe fn buffer_write(&mut self, off: uint, t: T) { + ptr::write(self.ptr.offset(off as int), t); + } + + /// Returns true iff the buffer is at capacity + #[inline] + fn is_full(&self) -> bool { self.cap - self.len() == 1 } +} + impl RingBuf { /// Creates an empty `RingBuf`. #[unstable = "matches collection reform specification, waiting for dust to settle"] @@ -55,8 +110,21 @@ impl RingBuf { /// Creates an empty `RingBuf` with space for at least `n` elements. #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn with_capacity(n: uint) -> RingBuf { - RingBuf{nelts: 0, lo: 0, - elts: Vec::from_fn(cmp::max(MINIMUM_CAPACITY, n), |_| None)} + // +1 since the ringbuffer always leaves one space empty + let cap = num::next_power_of_two(cmp::max(n + 1, MINIMUM_CAPACITY)); + let size = cap.checked_mul(&mem::size_of::()) + .expect("capacity overflow"); + + RingBuf { + tail: 0, + head: 0, + cap: cap, + ptr: if mem::size_of::() != 0 { + unsafe { heap::allocate(size, mem::min_align_of::()) as *mut T } + } else { + heap::EMPTY as *mut T + } + } } /// Retrieves an element in the `RingBuf` by index. @@ -74,9 +142,11 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn get(&self, i: uint) -> Option<&T> { - match self.elts.get(i) { - None => None, - Some(opt) => opt.as_ref(), + if i < self.len() { + let idx = wrap_index(self.tail + i, self.cap); + unsafe { Some(&*self.ptr.offset(idx as int)) } + } else { + None } } @@ -102,9 +172,11 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn get_mut(&mut self, i: uint) -> Option<&mut T> { - match self.elts.get_mut(i) { - None => None, - Some(opt) => opt.as_mut(), + if i < self.len() { + let idx = wrap_index(self.tail + i, self.cap); + unsafe { Some(&mut *self.ptr.offset(idx as int)) } + } else { + None } } @@ -130,15 +202,11 @@ impl RingBuf { pub fn swap(&mut self, i: uint, j: uint) { assert!(i < self.len()); assert!(j < self.len()); - let ri = self.raw_index(i); - let rj = self.raw_index(j); - self.elts.as_mut_slice().swap(ri, rj); - } - - /// Returns the index in the underlying `Vec` for a given logical element - /// index. - fn raw_index(&self, idx: uint) -> uint { - raw_index(self.lo, self.elts.len(), idx) + let ri = wrap_index(self.tail + i, self.cap); + let rj = wrap_index(self.tail + j, self.cap); + unsafe { + ptr::swap(self.ptr.offset(ri as int), self.ptr.offset(rj as int)) + } } /// Returns the number of elements the `RingBuf` can hold without @@ -150,14 +218,11 @@ impl RingBuf { /// use std::collections::RingBuf; /// /// let buf: RingBuf = RingBuf::with_capacity(10); - /// assert_eq!(buf.capacity(), 10); + /// assert!(buf.capacity() >= 10); /// ``` #[inline] #[unstable = "matches collection reform specification, waiting for dust to settle"] - pub fn capacity(&self) -> uint { - // FXIME(Gankro): not the actual usable capacity if you use reserve/reserve_exact - self.elts.capacity() - } + pub fn capacity(&self) -> uint { self.cap - 1 } /// Reserves the minimum capacity for exactly `additional` more elements to be inserted in the /// given `RingBuf`. Does nothing if the capacity is already sufficient. @@ -181,8 +246,7 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn reserve_exact(&mut self, additional: uint) { - // FIXME(Gankro): this is just wrong. The ringbuf won't actually use this space - self.elts.reserve_exact(additional); + self.reserve(additional); } /// Reserves capacity for at least `additional` more elements to be inserted in the given @@ -203,8 +267,63 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn reserve(&mut self, additional: uint) { - // FIXME(Gankro): this is just wrong. The ringbuf won't actually use this space - self.elts.reserve(additional); + let new_len = self.len() + additional; + assert!(new_len + 1 > self.len(), "capacity overflow"); + if new_len > self.capacity() { + let count = num::next_power_of_two(new_len + 1); + assert!(count >= new_len + 1); + + if mem::size_of::() != 0 { + let old = self.cap * mem::size_of::(); + let new = count.checked_mul(&mem::size_of::()) + .expect("capacity overflow"); + unsafe { + self.ptr = heap::reallocate(self.ptr as *mut u8, + old, + new, + mem::min_align_of::()) as *mut T; + } + } + + // Move the shortest contiguous section of the ring buffer + // T H + // [o o o o o o o . ] + // T H + // A [o o o o o o o . . . . . . . . . ] + // H T + // [o o . o o o o o ] + // T H + // B [. . . o o o o o o o . . . . . . ] + // H T + // [o o o o o . o o ] + // H T + // C [o o o o o . . . . . . . . . o o ] + + let oldcap = self.cap; + self.cap = count; + + if self.tail <= self.head { // A + // Nop + } else if self.head < oldcap - self.tail { // B + unsafe { + ptr::copy_nonoverlapping_memory( + self.ptr.offset(oldcap as int), + self.ptr as *const T, + self.head + ); + } + self.head += oldcap; + } else { // C + unsafe { + ptr::copy_nonoverlapping_memory( + self.ptr.offset((count - (oldcap - self.tail)) as int), + self.ptr.offset(self.tail as int) as *const T, + oldcap - self.tail + ); + } + self.tail = count - (oldcap - self.tail); + } + } } /// Returns a front-to-back iterator. @@ -223,7 +342,11 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn iter(&self) -> Items { - Items{index: 0, rindex: self.nelts, lo: self.lo, elts: self.elts.as_slice()} + Items { + tail: self.tail, + head: self.head, + ring: unsafe { self.buffer_as_slice() } + } } /// Returns a front-to-back iterator which returns mutable references. @@ -244,32 +367,14 @@ impl RingBuf { /// assert_eq!(buf.iter_mut().collect::>()[], b); /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] - pub fn iter_mut(&mut self) -> MutItems { - let start_index = raw_index(self.lo, self.elts.len(), 0); - let end_index = raw_index(self.lo, self.elts.len(), self.nelts); - - // Divide up the array - if end_index <= start_index { - // Items to iterate goes from: - // start_index to self.elts.len() - // and then - // 0 to end_index - let (temp, remaining1) = self.elts.split_at_mut(start_index); - let (remaining2, _) = temp.split_at_mut(end_index); - MutItems { - remaining1: remaining1.iter_mut(), - remaining2: remaining2.iter_mut(), - nelts: self.nelts, - } - } else { - // Items to iterate goes from start_index to end_index: - let (empty, elts) = self.elts.split_at_mut(0); - let remaining1 = elts[mut start_index..end_index]; - MutItems { - remaining1: remaining1.iter_mut(), - remaining2: empty.iter_mut(), - nelts: self.nelts, - } + pub fn iter_mut<'a>(&'a mut self) -> MutItems<'a, T> { + MutItems { + tail: self.tail, + head: self.head, + cap: self.cap, + ptr: self.ptr, + marker: marker::ContravariantLifetime::<'a>, + marker2: marker::NoCopy } } @@ -286,7 +391,7 @@ impl RingBuf { /// assert_eq!(v.len(), 1); /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] - pub fn len(&self) -> uint { self.nelts } + pub fn len(&self) -> uint { count(self.tail, self.head, self.cap) } /// Returns true if the buffer contains no elements /// @@ -317,9 +422,11 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn clear(&mut self) { - for x in self.elts.iter_mut() { *x = None } - self.nelts = 0; - self.lo = 0; + while !self.is_empty() { + self.pop_front(); + } + self.head = 0; + self.tail = 0; } /// Provides a reference to the front element, or `None` if the sequence is @@ -339,7 +446,7 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn front(&self) -> Option<&T> { - if self.nelts > 0 { Some(&self[0]) } else { None } + if !self.is_empty() { Some(&self[0]) } else { None } } /// Provides a mutable reference to the front element, or `None` if the @@ -363,7 +470,7 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn front_mut(&mut self) -> Option<&mut T> { - if self.nelts > 0 { Some(&mut self[0]) } else { None } + if !self.is_empty() { Some(&mut self[0]) } else { None } } /// Provides a reference to the back element, or `None` if the sequence is @@ -383,7 +490,7 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn back(&self) -> Option<&T> { - if self.nelts > 0 { Some(&self[self.nelts - 1]) } else { None } + if !self.is_empty() { Some(&self[self.len() - 1]) } else { None } } /// Provides a mutable reference to the back element, or `None` if the @@ -407,8 +514,8 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn back_mut(&mut self) -> Option<&mut T> { - let nelts = self.nelts; - if nelts > 0 { Some(&mut self[nelts - 1]) } else { None } + let len = self.len(); + if !self.is_empty() { Some(&mut self[len - 1]) } else { None } } /// Removes the first element and returns it, or `None` if the sequence is @@ -429,12 +536,13 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn pop_front(&mut self) -> Option { - let result = self.elts[self.lo].take(); - if result.is_some() { - self.lo = (self.lo + 1u) % self.elts.len(); - self.nelts -= 1u; + if self.is_empty() { + None + } else { + let tail = self.tail; + self.tail = wrap_index(self.tail + 1, self.cap); + unsafe { Some(self.buffer_read(tail)) } } - result } /// Inserts an element first in the sequence. @@ -451,14 +559,11 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn push_front(&mut self, t: T) { - if self.nelts == self.elts.len() { - grow(self.nelts, &mut self.lo, &mut self.elts); - } - if self.lo == 0u { - self.lo = self.elts.len() - 1u; - } else { self.lo -= 1u; } - self.elts[self.lo] = Some(t); - self.nelts += 1u; + if self.is_full() { self.reserve(1) } + + self.tail = wrap_index(self.tail - 1, self.cap); + let tail = self.tail; + unsafe { self.buffer_write(tail, t); } } /// Deprecated: Renamed to `push_back`. @@ -481,12 +586,11 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn push_back(&mut self, t: T) { - if self.nelts == self.elts.len() { - grow(self.nelts, &mut self.lo, &mut self.elts); - } - let hi = self.raw_index(self.nelts); - self.elts[hi] = Some(t); - self.nelts += 1u; + if self.is_full() { self.reserve(1) } + + let head = self.head; + self.head = wrap_index(self.head + 1, self.cap); + unsafe { self.buffer_write(head, t) } } /// Deprecated: Renamed to `pop_back`. @@ -511,38 +615,51 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn pop_back(&mut self) -> Option { - if self.nelts > 0 { - self.nelts -= 1; - let hi = self.raw_index(self.nelts); - self.elts[hi].take() - } else { + if self.is_empty() { None + } else { + self.head = wrap_index(self.head - 1, self.cap); + let head = self.head; + unsafe { Some(self.buffer_read(head)) } } } } +/// Returns the index in the underlying buffer for a given logical element index. +#[inline] +fn wrap_index(index: uint, size: uint) -> uint { + // size is always a power of 2 + index & (size - 1) +} + +/// Calculate the number of elements left to be read in the buffer +#[inline] +fn count(tail: uint, head: uint, size: uint) -> uint { + // size is always a power of 2 + (head - tail) & (size - 1) +} + /// `RingBuf` iterator. pub struct Items<'a, T:'a> { - lo: uint, - index: uint, - rindex: uint, - elts: &'a [Option], + ring: &'a [T], + tail: uint, + head: uint } impl<'a, T> Iterator<&'a T> for Items<'a, T> { #[inline] fn next(&mut self) -> Option<&'a T> { - if self.index == self.rindex { + if self.tail == self.head { return None; } - let raw_index = raw_index(self.lo, self.elts.len(), self.index); - self.index += 1; - Some(self.elts[raw_index].as_ref().unwrap()) + let tail = self.tail; + self.tail = wrap_index(self.tail + 1, self.ring.len()); + unsafe { Some(self.ring.unsafe_get(tail)) } } #[inline] fn size_hint(&self) -> (uint, Option) { - let len = self.rindex - self.index; + let len = count(self.tail, self.head, self.ring.len()); (len, Some(len)) } } @@ -550,129 +667,87 @@ impl<'a, T> Iterator<&'a T> for Items<'a, T> { impl<'a, T> DoubleEndedIterator<&'a T> for Items<'a, T> { #[inline] fn next_back(&mut self) -> Option<&'a T> { - if self.index == self.rindex { + if self.tail == self.head { return None; } - self.rindex -= 1; - let raw_index = raw_index(self.lo, self.elts.len(), self.rindex); - Some(self.elts[raw_index].as_ref().unwrap()) + self.head = wrap_index(self.head - 1, self.ring.len()); + unsafe { Some(self.ring.unsafe_get(self.head)) } } } + impl<'a, T> ExactSize<&'a T> for Items<'a, T> {} impl<'a, T> RandomAccessIterator<&'a T> for Items<'a, T> { #[inline] - fn indexable(&self) -> uint { self.rindex - self.index } + fn indexable(&self) -> uint { + let (len, _) = self.size_hint(); + len + } #[inline] fn idx(&mut self, j: uint) -> Option<&'a T> { if j >= self.indexable() { None } else { - let raw_index = raw_index(self.lo, self.elts.len(), self.index + j); - Some(self.elts[raw_index].as_ref().unwrap()) + let idx = wrap_index(self.tail + j, self.ring.len()); + unsafe { Some(self.ring.unsafe_get(idx)) } } } } +// FIXME This was implemented differently from Items because of a problem +// with returning the mutable reference. I couldn't find a way to +// make the lifetime checker happy so, but there should be a way. /// `RingBuf` mutable iterator. pub struct MutItems<'a, T:'a> { - remaining1: slice::MutItems<'a, Option>, - remaining2: slice::MutItems<'a, Option>, - nelts: uint, + ptr: *mut T, + tail: uint, + head: uint, + cap: uint, + marker: marker::ContravariantLifetime<'a>, + marker2: marker::NoCopy } impl<'a, T> Iterator<&'a mut T> for MutItems<'a, T> { #[inline] fn next(&mut self) -> Option<&'a mut T> { - if self.nelts == 0 { + if self.tail == self.head { return None; } - self.nelts -= 1; - match self.remaining1.next() { - Some(ptr) => return Some(ptr.as_mut().unwrap()), - None => {} - } - match self.remaining2.next() { - Some(ptr) => return Some(ptr.as_mut().unwrap()), - None => unreachable!(), + let tail = self.tail; + self.tail = wrap_index(self.tail + 1, self.cap); + if mem::size_of::() != 0 { + unsafe { Some(&mut *self.ptr.offset(tail as int)) } + } else { + // use a none zero pointer + Some(unsafe { mem::transmute(1u) }) } } #[inline] fn size_hint(&self) -> (uint, Option) { - (self.nelts, Some(self.nelts)) + let len = count(self.tail, self.head, self.cap); + (len, Some(len)) } } impl<'a, T> DoubleEndedIterator<&'a mut T> for MutItems<'a, T> { #[inline] fn next_back(&mut self) -> Option<&'a mut T> { - if self.nelts == 0 { + if self.tail == self.head { return None; } - self.nelts -= 1; - match self.remaining2.next_back() { - Some(ptr) => return Some(ptr.as_mut().unwrap()), - None => {} - } - match self.remaining1.next_back() { - Some(ptr) => return Some(ptr.as_mut().unwrap()), - None => unreachable!(), - } + self.head = wrap_index(self.head - 1, self.cap); + unsafe { Some(&mut *self.ptr.offset(self.head as int)) } } } impl<'a, T> ExactSize<&'a mut T> for MutItems<'a, T> {} -/// Grow is only called on full elts, so nelts is also len(elts), unlike -/// elsewhere. -fn grow(nelts: uint, loptr: &mut uint, elts: &mut Vec>) { - assert_eq!(nelts, elts.len()); - let lo = *loptr; - elts.reserve_exact(nelts); - let newlen = elts.capacity(); - - /* fill with None */ - for _ in range(elts.len(), newlen) { - elts.push(None); - } - - /* - Move the shortest half into the newly reserved area. - lo ---->| - nelts ----------->| - [o o o|o o o o o] - A [. . .|o o o o o o o o|. . . . .] - B [o o o|. . . . . . . .|o o o o o] - */ - - assert!(newlen - nelts/2 >= nelts); - if lo <= (nelts - lo) { // A - for i in range(0u, lo) { - elts.as_mut_slice().swap(i, nelts + i); - } - } else { // B - for i in range(lo, nelts) { - elts.as_mut_slice().swap(i, newlen - nelts + i); - } - *loptr += newlen - nelts; - } -} - -/// Returns the index in the underlying `Vec` for a given logical element index. -fn raw_index(lo: uint, len: uint, index: uint) -> uint { - if lo >= len - index { - lo + index - len - } else { - lo + index - } -} - impl PartialEq for RingBuf { fn eq(&self, other: &RingBuf) -> bool { - self.nelts == other.nelts && + self.len() == other.len() && self.iter().zip(other.iter()).all(|(a, b)| a.eq(b)) } fn ne(&self, other: &RingBuf) -> bool { @@ -707,22 +782,14 @@ impl> Hash for RingBuf { impl Index for RingBuf { #[inline] fn index<'a>(&'a self, i: &uint) -> &'a A { - let idx = self.raw_index(*i); - match self.elts[idx] { - None => panic!(), - Some(ref v) => v, - } + self.get(*i).expect("Out of bounds access") } } impl IndexMut for RingBuf { #[inline] fn index_mut<'a>(&'a mut self, i: &uint) -> &'a mut A { - let idx = self.raw_index(*i); - match *(&mut self.elts[idx]) { - None => panic!(), - Some(ref mut v) => v - } + self.get_mut(*i).expect("Out of bounds access") } } @@ -893,31 +960,86 @@ mod tests { } #[bench] - fn bench_push_back(b: &mut test::Bencher) { - let mut deq = RingBuf::new(); + fn bench_push_back_100(b: &mut test::Bencher) { + let mut deq = RingBuf::with_capacity(100); b.iter(|| { - deq.push_back(0i); + for i in range(0i, 100) { + deq.push_back(i); + } + deq.clear(); }) } #[bench] - fn bench_push_front(b: &mut test::Bencher) { - let mut deq = RingBuf::new(); + fn bench_push_front_100(b: &mut test::Bencher) { + let mut deq = RingBuf::with_capacity(100); b.iter(|| { - deq.push_front(0i); + for i in range(0i, 100) { + deq.push_front(i); + } + deq.clear(); }) } #[bench] - fn bench_grow(b: &mut test::Bencher) { - let mut deq = RingBuf::new(); + fn bench_pop_100(b: &mut test::Bencher) { + let mut deq = RingBuf::with_capacity(100); + + b.iter(|| { + for i in range(0i, 100) { + deq.push_back(i); + } + while None != deq.pop_back() {} + }) + } + + #[bench] + fn bench_pop_front_100(b: &mut test::Bencher) { + let mut deq = RingBuf::with_capacity(100); + + b.iter(|| { + for i in range(0i, 100) { + deq.push_back(i); + } + while None != deq.pop_front() {} + }) + } + + #[bench] + fn bench_grow_1025(b: &mut test::Bencher) { + b.iter(|| { + let mut deq = RingBuf::new(); + for i in range(0i, 1025) { + deq.push_front(i); + } + }) + } + + #[bench] + fn bench_iter_1000(b: &mut test::Bencher) { + let ring: RingBuf = range(0i, 1000).collect(); + b.iter(|| { - for _ in range(0i, 65) { - deq.push_front(1i); + let mut sum = 0; + for &i in ring.iter() { + sum += i; } + sum }) } + #[bench] + fn bench_mut_iter_1000(b: &mut test::Bencher) { + let mut ring: RingBuf = range(0i, 1000).collect(); + + b.iter(|| { + for i in ring.iter_mut() { + *i += 1; + } + }) + } + + #[deriving(Clone, PartialEq, Show)] enum Taggy { One(int), @@ -1034,11 +1156,11 @@ mod tests { let mut d = RingBuf::new(); d.push_back(0u64); d.reserve(50); - assert!(d.capacity() >= 64); + assert!(d.capacity() >= 51); let mut d = RingBuf::new(); d.push_back(0u32); d.reserve(50); - assert!(d.capacity() >= 64); + assert!(d.capacity() >= 51); } #[test] @@ -1257,4 +1379,169 @@ mod tests { .collect(); assert!(format!("{}", ringbuf).as_slice() == "[just, one, test, more]"); } + + #[test] + fn test_drop() { + static mut drops: uint = 0; + struct Elem; + impl Drop for Elem { + fn drop(&mut self) { + unsafe { drops += 1; } + } + } + + let mut ring = RingBuf::new(); + ring.push_back(Elem); + ring.push_front(Elem); + ring.push_back(Elem); + ring.push_front(Elem); + drop(ring); + + assert_eq!(unsafe {drops}, 4); + } + + #[test] + fn test_drop_with_pop() { + static mut drops: uint = 0; + struct Elem; + impl Drop for Elem { + fn drop(&mut self) { + unsafe { drops += 1; } + } + } + + let mut ring = RingBuf::new(); + ring.push_back(Elem); + ring.push_front(Elem); + ring.push_back(Elem); + ring.push_front(Elem); + + drop(ring.pop_back()); + drop(ring.pop_front()); + assert_eq!(unsafe {drops}, 2); + + drop(ring); + assert_eq!(unsafe {drops}, 4); + } + + #[test] + fn test_drop_clear() { + static mut drops: uint = 0; + struct Elem; + impl Drop for Elem { + fn drop(&mut self) { + unsafe { drops += 1; } + } + } + + let mut ring = RingBuf::new(); + ring.push_back(Elem); + ring.push_front(Elem); + ring.push_back(Elem); + ring.push_front(Elem); + ring.clear(); + assert_eq!(unsafe {drops}, 4); + + drop(ring); + assert_eq!(unsafe {drops}, 4); + } + + #[test] + fn test_reserve_grow() { + // test growth path A + // [T o o H] -> [T o o H . . . . ] + let mut ring = RingBuf::with_capacity(4); + for i in range(0i, 3) { + ring.push_back(i); + } + ring.reserve(7); + for i in range(0i, 3) { + assert_eq!(ring.pop_front(), Some(i)); + } + + // test growth path B + // [H T o o] -> [. T o o H . . . ] + let mut ring = RingBuf::with_capacity(4); + for i in range(0i, 1) { + ring.push_back(i); + assert_eq!(ring.pop_front(), Some(i)); + } + for i in range(0i, 3) { + ring.push_back(i); + } + ring.reserve(7); + for i in range(0i, 3) { + assert_eq!(ring.pop_front(), Some(i)); + } + + // test growth path C + // [o o H T] -> [o o H . . . . T ] + let mut ring = RingBuf::with_capacity(4); + for i in range(0i, 3) { + ring.push_back(i); + assert_eq!(ring.pop_front(), Some(i)); + } + for i in range(0i, 3) { + ring.push_back(i); + } + ring.reserve(7); + for i in range(0i, 3) { + assert_eq!(ring.pop_front(), Some(i)); + } + } + + #[test] + fn test_get() { + let mut ring = RingBuf::new(); + ring.push_back(0i); + assert_eq!(ring.get(0), Some(&0)); + assert_eq!(ring.get(1), None); + + ring.push_back(1); + assert_eq!(ring.get(0), Some(&0)); + assert_eq!(ring.get(1), Some(&1)); + assert_eq!(ring.get(2), None); + + ring.push_back(2); + assert_eq!(ring.get(0), Some(&0)); + assert_eq!(ring.get(1), Some(&1)); + assert_eq!(ring.get(2), Some(&2)); + assert_eq!(ring.get(3), None); + + assert_eq!(ring.pop_front(), Some(0)); + assert_eq!(ring.get(0), Some(&1)); + assert_eq!(ring.get(1), Some(&2)); + assert_eq!(ring.get(2), None); + + assert_eq!(ring.pop_front(), Some(1)); + assert_eq!(ring.get(0), Some(&2)); + assert_eq!(ring.get(1), None); + + assert_eq!(ring.pop_front(), Some(2)); + assert_eq!(ring.get(0), None); + assert_eq!(ring.get(1), None); + } + + #[test] + fn test_get_mut() { + let mut ring = RingBuf::new(); + for i in range(0i, 3) { + ring.push_back(i); + } + + match ring.get_mut(1) { + Some(x) => *x = -1, + None => () + }; + + assert_eq!(ring.get_mut(0), Some(&mut 0)); + assert_eq!(ring.get_mut(1), Some(&mut -1)); + assert_eq!(ring.get_mut(2), Some(&mut 2)); + assert_eq!(ring.get_mut(3), None); + + assert_eq!(ring.pop_front(), Some(0)); + assert_eq!(ring.get_mut(0), Some(&mut -1)); + assert_eq!(ring.get_mut(1), Some(&mut 2)); + assert_eq!(ring.get_mut(2), None); + } } From ba24e3302102eb97c253ad8d0ad08a5678428ae5 Mon Sep 17 00:00:00 2001 From: Colin Sherratt Date: Sun, 9 Nov 2014 22:34:53 -0500 Subject: [PATCH 2/6] Handle allocate/reallocate errors in ring_buf Use is_some() in clear to simplify the clear loop. --- src/libcollections/ring_buf.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/libcollections/ring_buf.rs b/src/libcollections/ring_buf.rs index fcf9e33cc9d9e..79201b1db54f3 100644 --- a/src/libcollections/ring_buf.rs +++ b/src/libcollections/ring_buf.rs @@ -115,15 +115,21 @@ impl RingBuf { let size = cap.checked_mul(&mem::size_of::()) .expect("capacity overflow"); + let ptr = if mem::size_of::() != 0 { + unsafe { + let ptr = heap::allocate(size, mem::min_align_of::()) as *mut T;; + if ptr.is_null() { ::alloc::oom() } + ptr + } + } else { + heap::EMPTY as *mut T + }; + RingBuf { tail: 0, head: 0, cap: cap, - ptr: if mem::size_of::() != 0 { - unsafe { heap::allocate(size, mem::min_align_of::()) as *mut T } - } else { - heap::EMPTY as *mut T - } + ptr: ptr } } @@ -282,6 +288,7 @@ impl RingBuf { old, new, mem::min_align_of::()) as *mut T; + if self.ptr.is_null() { ::alloc::oom() } } } @@ -422,9 +429,7 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn clear(&mut self) { - while !self.is_empty() { - self.pop_front(); - } + while self.pop_front().is_some() {} self.head = 0; self.tail = 0; } @@ -720,7 +725,7 @@ impl<'a, T> Iterator<&'a mut T> for MutItems<'a, T> { if mem::size_of::() != 0 { unsafe { Some(&mut *self.ptr.offset(tail as int)) } } else { - // use a none zero pointer + // use a non-zero pointer Some(unsafe { mem::transmute(1u) }) } } From 4cae9add8c9471126aa405cacecac2623232083a Mon Sep 17 00:00:00 2001 From: Colin Sherratt Date: Mon, 10 Nov 2014 21:16:29 -0500 Subject: [PATCH 3/6] Added some extra debug_asserts to ring_buf. --- src/libcollections/ring_buf.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/libcollections/ring_buf.rs b/src/libcollections/ring_buf.rs index 79201b1db54f3..f08c53340006b 100644 --- a/src/libcollections/ring_buf.rs +++ b/src/libcollections/ring_buf.rs @@ -209,7 +209,7 @@ impl RingBuf { assert!(i < self.len()); assert!(j < self.len()); let ri = wrap_index(self.tail + i, self.cap); - let rj = wrap_index(self.tail + j, self.cap); + let rj = wrap_index(self.tail + j, self.cap);; unsafe { ptr::swap(self.ptr.offset(ri as int), self.ptr.offset(rj as int)) } @@ -320,6 +320,7 @@ impl RingBuf { ); } self.head += oldcap; + debug_assert!(self.head > self.tail); } else { // C unsafe { ptr::copy_nonoverlapping_memory( @@ -329,7 +330,10 @@ impl RingBuf { ); } self.tail = count - (oldcap - self.tail); + debug_assert!(self.head < self.tail); } + debug_assert!(self.head < self.cap); + debug_assert!(self.tail < self.cap); } } @@ -564,7 +568,10 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn push_front(&mut self, t: T) { - if self.is_full() { self.reserve(1) } + if self.is_full() { + self.reserve(1); + debug_assert!(!self.is_full()); + } self.tail = wrap_index(self.tail - 1, self.cap); let tail = self.tail; @@ -591,7 +598,10 @@ impl RingBuf { /// ``` #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn push_back(&mut self, t: T) { - if self.is_full() { self.reserve(1) } + if self.is_full() { + self.reserve(1); + debug_assert!(!self.is_full()); + } let head = self.head; self.head = wrap_index(self.head + 1, self.cap); @@ -634,7 +644,9 @@ impl RingBuf { #[inline] fn wrap_index(index: uint, size: uint) -> uint { // size is always a power of 2 - index & (size - 1) + let idx = index & (size - 1); + debug_assert!(idx < size); + idx } /// Calculate the number of elements left to be read in the buffer From 5e549d8c3c6b69fea8cbbadf8028dc4fa62bdbfb Mon Sep 17 00:00:00 2001 From: Colin Sherratt Date: Mon, 10 Nov 2014 21:57:52 -0500 Subject: [PATCH 4/6] Manually reset the ringbuffer before or after the ringbuffer push/pop tests. --- src/libcollections/ring_buf.rs | 42 ++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/libcollections/ring_buf.rs b/src/libcollections/ring_buf.rs index f08c53340006b..1024a2828b3dd 100644 --- a/src/libcollections/ring_buf.rs +++ b/src/libcollections/ring_buf.rs @@ -209,7 +209,7 @@ impl RingBuf { assert!(i < self.len()); assert!(j < self.len()); let ri = wrap_index(self.tail + i, self.cap); - let rj = wrap_index(self.tail + j, self.cap);; + let rj = wrap_index(self.tail + j, self.cap); unsafe { ptr::swap(self.ptr.offset(ri as int), self.ptr.offset(rj as int)) } @@ -972,53 +972,58 @@ mod tests { #[bench] fn bench_new(b: &mut test::Bencher) { b.iter(|| { - let _: RingBuf = RingBuf::new(); + let ring: RingBuf = RingBuf::new(); + test::black_box(ring); }) } #[bench] fn bench_push_back_100(b: &mut test::Bencher) { - let mut deq = RingBuf::with_capacity(100); + let mut deq = RingBuf::with_capacity(101); b.iter(|| { for i in range(0i, 100) { deq.push_back(i); } - deq.clear(); + deq.head = 0; + deq.tail = 0; }) } #[bench] fn bench_push_front_100(b: &mut test::Bencher) { - let mut deq = RingBuf::with_capacity(100); + let mut deq = RingBuf::with_capacity(101); b.iter(|| { for i in range(0i, 100) { deq.push_front(i); } - deq.clear(); + deq.head = 0; + deq.tail = 0; }) } #[bench] - fn bench_pop_100(b: &mut test::Bencher) { - let mut deq = RingBuf::with_capacity(100); + fn bench_pop_back_100(b: &mut test::Bencher) { + let mut deq: RingBuf = RingBuf::with_capacity(101); b.iter(|| { - for i in range(0i, 100) { - deq.push_back(i); + deq.head = 100; + deq.tail = 0; + while !deq.is_empty() { + test::black_box(deq.pop_back()); } - while None != deq.pop_back() {} }) } #[bench] fn bench_pop_front_100(b: &mut test::Bencher) { - let mut deq = RingBuf::with_capacity(100); + let mut deq: RingBuf = RingBuf::with_capacity(101); b.iter(|| { - for i in range(0i, 100) { - deq.push_back(i); + deq.head = 100; + deq.tail = 0; + while !deq.is_empty() { + test::black_box(deq.pop_front()); } - while None != deq.pop_front() {} }) } @@ -1029,6 +1034,7 @@ mod tests { for i in range(0i, 1025) { deq.push_front(i); } + test::black_box(deq); }) } @@ -1041,7 +1047,7 @@ mod tests { for &i in ring.iter() { sum += i; } - sum + test::black_box(sum); }) } @@ -1050,9 +1056,11 @@ mod tests { let mut ring: RingBuf = range(0i, 1000).collect(); b.iter(|| { + let mut sum = 0; for i in ring.iter_mut() { - *i += 1; + sum += *i; } + test::black_box(sum); }) } From 4019118ec2f5fb1e6749e80b15fc70a9d0accd6f Mon Sep 17 00:00:00 2001 From: Colin Sherratt Date: Tue, 11 Nov 2014 20:22:07 -0500 Subject: [PATCH 5/6] Added population count assertion in reserve. Cleaned up wrap_index. --- src/libcollections/ring_buf.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/libcollections/ring_buf.rs b/src/libcollections/ring_buf.rs index 1024a2828b3dd..359dffa085321 100644 --- a/src/libcollections/ring_buf.rs +++ b/src/libcollections/ring_buf.rs @@ -98,6 +98,10 @@ impl RingBuf { /// Returns true iff the buffer is at capacity #[inline] fn is_full(&self) -> bool { self.cap - self.len() == 1 } + + /// Returns the index in the underlying buffer for a given logical element index. + #[inline] + fn wrap_index(&self, idx: uint) -> uint { wrap_index(idx, self.cap) } } impl RingBuf { @@ -149,7 +153,7 @@ impl RingBuf { #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn get(&self, i: uint) -> Option<&T> { if i < self.len() { - let idx = wrap_index(self.tail + i, self.cap); + let idx = self.wrap_index(self.tail + i); unsafe { Some(&*self.ptr.offset(idx as int)) } } else { None @@ -179,7 +183,7 @@ impl RingBuf { #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn get_mut(&mut self, i: uint) -> Option<&mut T> { if i < self.len() { - let idx = wrap_index(self.tail + i, self.cap); + let idx = self.wrap_index(self.tail + i); unsafe { Some(&mut *self.ptr.offset(idx as int)) } } else { None @@ -208,8 +212,8 @@ impl RingBuf { pub fn swap(&mut self, i: uint, j: uint) { assert!(i < self.len()); assert!(j < self.len()); - let ri = wrap_index(self.tail + i, self.cap); - let rj = wrap_index(self.tail + j, self.cap); + let ri = self.wrap_index(self.tail + i); + let rj = self.wrap_index(self.tail + j); unsafe { ptr::swap(self.ptr.offset(ri as int), self.ptr.offset(rj as int)) } @@ -334,6 +338,7 @@ impl RingBuf { } debug_assert!(self.head < self.cap); debug_assert!(self.tail < self.cap); + debug_assert!(self.cap.count_ones() == 1); } } @@ -549,7 +554,7 @@ impl RingBuf { None } else { let tail = self.tail; - self.tail = wrap_index(self.tail + 1, self.cap); + self.tail = self.wrap_index(self.tail + 1); unsafe { Some(self.buffer_read(tail)) } } } @@ -573,7 +578,7 @@ impl RingBuf { debug_assert!(!self.is_full()); } - self.tail = wrap_index(self.tail - 1, self.cap); + self.tail = self.wrap_index(self.tail - 1); let tail = self.tail; unsafe { self.buffer_write(tail, t); } } @@ -604,7 +609,7 @@ impl RingBuf { } let head = self.head; - self.head = wrap_index(self.head + 1, self.cap); + self.head = self.wrap_index(self.head + 1); unsafe { self.buffer_write(head, t) } } @@ -633,7 +638,7 @@ impl RingBuf { if self.is_empty() { None } else { - self.head = wrap_index(self.head - 1, self.cap); + self.head = self.wrap_index(self.head - 1); let head = self.head; unsafe { Some(self.buffer_read(head)) } } @@ -644,9 +649,7 @@ impl RingBuf { #[inline] fn wrap_index(index: uint, size: uint) -> uint { // size is always a power of 2 - let idx = index & (size - 1); - debug_assert!(idx < size); - idx + index & (size - 1) } /// Calculate the number of elements left to be read in the buffer From 6277e3b2d9567a2f829862c713e78e4ac13bb600 Mon Sep 17 00:00:00 2001 From: Colin Sherratt Date: Fri, 14 Nov 2014 04:21:44 -0500 Subject: [PATCH 6/6] Update ring_buf.rs from fallout of #18827. --- src/libcollections/ring_buf.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libcollections/ring_buf.rs b/src/libcollections/ring_buf.rs index 359dffa085321..e95660b546ebc 100644 --- a/src/libcollections/ring_buf.rs +++ b/src/libcollections/ring_buf.rs @@ -22,7 +22,7 @@ use core::raw::Slice as RawSlice; use core::ptr; use core::kinds::marker; use core::mem; -use core::num; +use core::num::{Int, UnsignedInt}; use std::hash::{Writer, Hash}; use std::cmp; @@ -115,8 +115,8 @@ impl RingBuf { #[unstable = "matches collection reform specification, waiting for dust to settle"] pub fn with_capacity(n: uint) -> RingBuf { // +1 since the ringbuffer always leaves one space empty - let cap = num::next_power_of_two(cmp::max(n + 1, MINIMUM_CAPACITY)); - let size = cap.checked_mul(&mem::size_of::()) + let cap = cmp::max(n + 1, MINIMUM_CAPACITY).next_power_of_two(); + let size = cap.checked_mul(mem::size_of::()) .expect("capacity overflow"); let ptr = if mem::size_of::() != 0 { @@ -280,12 +280,12 @@ impl RingBuf { let new_len = self.len() + additional; assert!(new_len + 1 > self.len(), "capacity overflow"); if new_len > self.capacity() { - let count = num::next_power_of_two(new_len + 1); + let count = (new_len + 1).next_power_of_two(); assert!(count >= new_len + 1); if mem::size_of::() != 0 { let old = self.cap * mem::size_of::(); - let new = count.checked_mul(&mem::size_of::()) + let new = count.checked_mul(mem::size_of::()) .expect("capacity overflow"); unsafe { self.ptr = heap::reallocate(self.ptr as *mut u8,