diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index be36f98..7caffbd 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -10,8 +10,7 @@ jobs: name: Lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - + - uses: actions/checkout@v3 - name: Install nightly toolchain with lint tools available uses: actions-rs/toolchain@v1 with: @@ -40,8 +39,7 @@ jobs: name: Test Stable runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - + - uses: actions/checkout@v3 - name: Install stable toolchain uses: actions-rs/toolchain@v1 with: @@ -54,12 +52,11 @@ jobs: command: test test-beta: - name: Test Beta + Coverage + name: Test Beta runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 - + - uses: actions/checkout@v3 - name: Install beta toolchain uses: actions-rs/toolchain@v1 with: @@ -67,27 +64,15 @@ jobs: toolchain: beta override: true - - name: rust-tarpaulin - uses: actions-rs/tarpaulin@v0.1.3 - with: - args: --all-features --out Xml - - - name: Upload to codecov.io - uses: codecov/codecov-action@v1.0.2 - with: - token: ${{ secrets.CODECOV_TOKEN }} - - - name: Archive code coverage results - uses: actions/upload-artifact@v1 + - name: Run cargo test + uses: actions-rs/cargo@v1 with: - name: code-coverage-report - path: cobertura.xml - + command: test test-nightly: name: Test Nightly runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install nightly toolchain uses: actions-rs/toolchain@v1 diff --git a/Cargo.toml b/Cargo.toml index c3db58b..2cfd32a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,8 @@ categories = ["data-structures"] license = "MIT" [dev-dependencies] -criterion = "0.1.2" +criterion = "0.4.0" +compiletest_rs = "0.9.0" [features] default = ["alloc"] diff --git a/benches/bench.rs b/benches/bench.rs index 8b524fa..ceaab49 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -1,13 +1,13 @@ #![cfg(not(tarpaulin))] use criterion::{black_box, criterion_group, criterion_main, Bencher, Criterion}; -use ringbuffer::{AllocRingBuffer, ConstGenericRingBuffer, RingBufferExt}; +use ringbuffer::{AllocRingBuffer, ConstGenericRingBuffer, RingBufferExt, RingBufferWrite}; fn benchmark_push, F: Fn() -> T>(b: &mut Bencher, new: F) { b.iter(|| { let mut rb = new(); for i in 0..1_000_000 { - rb.push(i) + black_box(rb.push(i)); } rb @@ -19,23 +19,23 @@ fn benchmark_push_dequeue, F: Fn() -> T>(b: &mut Bencher, let mut rb = new(); for _i in 0..100_000 { - rb.push(1); - rb.push(2); + black_box(rb.push(1)); + black_box(rb.push(2)); - assert_eq!(rb.dequeue(), Some(1)); - assert_eq!(rb.dequeue(), Some(2)); + assert_eq!(black_box(rb.dequeue()), Some(1)); + assert_eq!(black_box(rb.dequeue()), Some(2)); - rb.push(1); - rb.push(2); + black_box(rb.push(1)); + black_box(rb.push(2)); - assert_eq!(rb.dequeue(), Some(1)); - assert_eq!(rb.dequeue(), Some(2)); + assert_eq!(black_box(rb.dequeue()), Some(1)); + assert_eq!(black_box(rb.dequeue()), Some(2)); - rb.push(1); - rb.push(2); + black_box(rb.push(1)); + black_box(rb.push(2)); - assert_eq!(rb.get(-1), Some(&2)); - assert_eq!(rb.get(-2), Some(&1)); + assert_eq!(black_box(rb.get(-1)), Some(&2)); + assert_eq!(black_box(rb.get(-2)), Some(&1)); } rb @@ -47,7 +47,7 @@ fn benchmark_various, F: Fn() -> T>(b: &mut Bencher, new: let mut rb = new(); for i in 0..100_000 { - rb.push(i); + black_box(rb.push(i)); black_box(rb.get(-1)); } @@ -63,6 +63,13 @@ macro_rules! generate_benches { })); )* }; + (non_power_two, $c: tt, $rb: tt, $ty: tt, $fn: tt, $bmfunc: tt, $($i:tt),*) => { + $( + $c.bench_function(&format!("{} {} 1M capacity not power of two {}", stringify!($rb), stringify!($bmfunc), stringify!($i)), |b| $bmfunc(b, || { + $rb::<$ty, _>::$fn($i) + })); + )* + }; (typed, $c: tt, $rb: tt, $ty: tt, $fn: tt, $bmfunc: tt, $($i:tt),*) => { $( @@ -73,9 +80,19 @@ macro_rules! generate_benches { }; } -fn criterion_benchmark(c: &mut Criterion) { - c.with_plots(); +fn benchmark_non_power_of_two(b: &mut Bencher) { + b.iter(|| { + let mut rb = AllocRingBuffer::with_capacity_non_power_of_two(L); + for i in 0..1_000_000 { + black_box(rb.push(i)); + } + + rb + }) +} + +fn criterion_benchmark(c: &mut Criterion) { // TODO: Improve benchmarks // * What are representative operations // * Make sure it's accurate @@ -153,6 +170,20 @@ fn criterion_benchmark(c: &mut Criterion) { 4096, 8192 ]; + generate_benches![ + non_power_two, + c, + AllocRingBuffer, + i32, + with_capacity_non_power_of_two, + benchmark_various, + 16, + 17, + 1024, + 4096, + 8192, + 8195 + ]; } criterion_group!(benches, criterion_benchmark); diff --git a/src/lib.rs b/src/lib.rs index 824afc3..1a4e2fe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -81,6 +81,12 @@ const fn mask(cap: usize, index: usize) -> usize { index & (cap - 1) } +/// Used internally. Computes the bitmask used to properly wrap the ringbuffers. +#[inline] +const fn mask_modulo(cap: usize, index: usize) -> usize { + index % cap +} + #[cfg(test)] #[allow(non_upper_case_globals)] mod tests { diff --git a/src/with_alloc.rs b/src/with_alloc.rs index ea201b3..b1b7e02 100644 --- a/src/with_alloc.rs +++ b/src/with_alloc.rs @@ -6,9 +6,46 @@ extern crate alloc; // We need vecs so depend on alloc use alloc::vec::Vec; use core::iter::FromIterator; +use core::marker::PhantomData; use core::mem; use core::mem::MaybeUninit; +#[derive(Debug, Copy, Clone)] +pub struct PowerOfTwo; +#[derive(Debug, Copy, Clone)] +pub struct NonPowerOfTwo; +mod private { + use crate::with_alloc::{NonPowerOfTwo, PowerOfTwo}; + + pub trait Sealed {} + impl Sealed for PowerOfTwo {} + impl Sealed for NonPowerOfTwo {} +} +pub trait RingbufferMode: private::Sealed { + fn mask(cap: usize, index: usize) -> usize; + fn must_be_power_of_two() -> bool; +} +impl RingbufferMode for PowerOfTwo { + #[inline] + fn mask(cap: usize, index: usize) -> usize { + crate::mask(cap, index) + } + + fn must_be_power_of_two() -> bool { + true + } +} +impl RingbufferMode for NonPowerOfTwo { + #[inline] + fn mask(cap: usize, index: usize) -> usize { + crate::mask_modulo(cap, index) + } + + fn must_be_power_of_two() -> bool { + false + } +} + /// The `AllocRingBuffer` is a `RingBufferExt` which is based on a Vec. This means it allocates at runtime /// on the heap, and therefore needs the [`alloc`] crate. This struct and therefore the dependency on /// alloc can be disabled by disabling the `alloc` (default) feature. @@ -36,27 +73,34 @@ use core::mem::MaybeUninit; /// assert_eq!(buffer.to_vec(), vec![42, 1]); /// ``` #[derive(Debug)] -pub struct AllocRingBuffer { +pub struct AllocRingBuffer { buf: Vec>, capacity: usize, readptr: usize, writeptr: usize, + mode: PhantomData, } -impl Drop for AllocRingBuffer { +impl Drop for AllocRingBuffer { fn drop(&mut self) { self.drain().for_each(drop); } } -impl Clone for AllocRingBuffer { +impl Clone for AllocRingBuffer { fn clone(&self) -> Self { - let mut new = Self::with_capacity(self.capacity); + debug_assert_ne!(self.capacity, 0); + debug_assert!(!MODE::must_be_power_of_two() || self.capacity.is_power_of_two()); + + // whatever the previous capacity was, we can just use the same one again. + // It should be valid. + let mut new = unsafe { Self::with_capacity_unchecked(self.capacity) }; self.iter().cloned().for_each(|i| new.push(i)); new } } -impl PartialEq for AllocRingBuffer { + +impl PartialEq for AllocRingBuffer { fn eq(&self, other: &Self) -> bool { self.capacity == other.capacity && self.len() == other.len() @@ -64,19 +108,19 @@ impl PartialEq for AllocRingBuffer { } } -impl Eq for AllocRingBuffer {} +impl Eq for AllocRingBuffer {} /// The capacity of a `RingBuffer` created by new or default (`1024`). // must be a power of 2 pub const RINGBUFFER_DEFAULT_CAPACITY: usize = 1024; -unsafe impl RingBufferExt for AllocRingBuffer { +unsafe impl RingBufferExt for AllocRingBuffer { impl_ringbuffer_ext!( get_unchecked, get_unchecked_mut, readptr, writeptr, - crate::mask + MODE::mask ); #[inline] @@ -92,12 +136,12 @@ unsafe impl RingBufferExt for AllocRingBuffer { } } -impl RingBufferRead for AllocRingBuffer { +impl RingBufferRead for AllocRingBuffer { fn dequeue(&mut self) -> Option { if self.is_empty() { None } else { - let index = crate::mask(self.capacity, self.readptr); + let index = MODE::mask(self.capacity, self.readptr); let res = mem::replace(&mut self.buf[index], MaybeUninit::uninit()); self.readptr += 1; @@ -111,7 +155,7 @@ impl RingBufferRead for AllocRingBuffer { impl_ringbuffer_read!(); } -impl Extend for AllocRingBuffer { +impl Extend for AllocRingBuffer { fn extend>(&mut self, iter: A) { let iter = iter.into_iter(); @@ -121,12 +165,12 @@ impl Extend for AllocRingBuffer { } } -impl RingBufferWrite for AllocRingBuffer { +impl RingBufferWrite for AllocRingBuffer { #[inline] fn push(&mut self, value: T) { if self.is_full() { let previous_value = mem::replace( - &mut self.buf[crate::mask(self.capacity, self.readptr)], + &mut self.buf[MODE::mask(self.capacity, self.readptr)], MaybeUninit::uninit(), ); // make sure we drop whatever is being overwritten @@ -140,7 +184,7 @@ impl RingBufferWrite for AllocRingBuffer { self.readptr += 1; } - let index = crate::mask(self.capacity, self.writeptr); + let index = MODE::mask(self.capacity, self.writeptr); if index >= self.buf.len() { // initializing the maybeuninit when values are inserted/pushed @@ -154,7 +198,7 @@ impl RingBufferWrite for AllocRingBuffer { } } -impl RingBuffer for AllocRingBuffer { +impl RingBuffer for AllocRingBuffer { #[inline] unsafe fn ptr_capacity(rb: *const Self) -> usize { (*rb).capacity @@ -163,24 +207,54 @@ impl RingBuffer for AllocRingBuffer { impl_ringbuffer!(readptr, writeptr); } -impl AllocRingBuffer { +impl AllocRingBuffer { /// Creates a `AllocRingBuffer` with a certain capacity. This capacity is fixed. /// for this ringbuffer to work, cap must be a power of two and greater than zero. + /// + /// # Safety + /// Only safe if the capacity is greater than zero, and a power of two. + /// Only if Mode == NonPowerOfTwo can the capacity be not a power of two, in which case this function is also safe. #[inline] - pub fn with_capacity_unchecked(cap: usize) -> Self { + unsafe fn with_capacity_unchecked(cap: usize) -> Self { Self { buf: Vec::with_capacity(cap), capacity: cap, readptr: 0, writeptr: 0, + mode: Default::default(), } } +} +impl AllocRingBuffer { + /// Creates a `AllocRingBuffer` with a certain capacity. This capacity is fixed. + /// for this ringbuffer to work, and must not be zero. + /// + /// Note, that not using a power of two means some operations can't be optimized as well. + /// For example, bitwise ands might become modulos. + /// + /// For example, on push operations, benchmarks have shown that a ringbuffer with a power-of-two + /// capacity constructed with `with_capacity_non_power_of_two` (so which don't get the same optimization + /// as the ones constructed with `with_capacity`) can be up to 3x slower + /// + /// # Panics + /// if the capacity is zero + #[inline] + pub fn with_capacity_non_power_of_two(cap: usize) -> Self { + assert_ne!(cap, 0, "Capacity must be greater than 0"); + + // Safety: Mode is NonPowerOfTwo and we checked above that the capacity isn't zero + unsafe { Self::with_capacity_unchecked(cap) } + } +} + +impl AllocRingBuffer { /// Creates a `AllocRingBuffer` with a certain capacity. The actual capacity is the input to the /// function raised to the power of two (effectively the input is the log2 of the actual capacity) #[inline] pub fn with_capacity_power_of_2(cap_power_of_two: usize) -> Self { - Self::with_capacity_unchecked(1 << cap_power_of_two) + // Safety: 1 << n is always a power of two, and nonzero + unsafe { Self::with_capacity_unchecked(1 << cap_power_of_two) } } #[inline] @@ -191,7 +265,8 @@ impl AllocRingBuffer { assert_ne!(cap, 0, "Capacity must be greater than 0"); assert!(cap.is_power_of_two(), "Capacity must be a power of two"); - Self::with_capacity_unchecked(cap) + // Safety: assertions check that cap is a power of two and nonzero + unsafe { Self::with_capacity_unchecked(cap) } } /// Creates an `AllocRingBuffer` with a capacity of [`RINGBUFFER_DEFAULT_CAPACITY`]. @@ -204,7 +279,10 @@ impl AllocRingBuffer { /// Get a reference from the buffer without checking it is initialized. /// Caller must be sure the index is in bounds, or this will panic. #[inline] -unsafe fn get_unchecked<'a, T>(rb: *const AllocRingBuffer, index: usize) -> &'a T { +unsafe fn get_unchecked<'a, T, MODE: RingbufferMode>( + rb: *const AllocRingBuffer, + index: usize, +) -> &'a T { let p = &(*rb).buf[index]; // Safety: caller makes sure the index is in bounds for the ringbuffer. // All in bounds values in the ringbuffer are initialized @@ -214,7 +292,10 @@ unsafe fn get_unchecked<'a, T>(rb: *const AllocRingBuffer, index: usize) -> & /// Get a mut reference from the buffer without checking it is initialized. /// Caller must be sure the index is in bounds, or this will panic. #[inline] -unsafe fn get_unchecked_mut(rb: *mut AllocRingBuffer, index: usize) -> *mut T { +unsafe fn get_unchecked_mut( + rb: *mut AllocRingBuffer, + index: usize, +) -> *mut T { let p = (*rb).buf.as_mut_ptr().add(index); // Safety: caller makes sure the index is in bounds for the ringbuffer. @@ -222,7 +303,7 @@ unsafe fn get_unchecked_mut(rb: *mut AllocRingBuffer, index: usize) -> *mu p.cast() } -impl FromIterator for AllocRingBuffer { +impl FromIterator for AllocRingBuffer { fn from_iter>(iter: T) -> Self { let mut res = Self::default(); for i in iter { @@ -233,7 +314,7 @@ impl FromIterator for AllocRingBuffer { } } -impl Default for AllocRingBuffer { +impl Default for AllocRingBuffer { /// Creates a buffer with a capacity of [`crate::RINGBUFFER_DEFAULT_CAPACITY`]. #[inline] fn default() -> Self { @@ -242,11 +323,12 @@ impl Default for AllocRingBuffer { capacity: RINGBUFFER_DEFAULT_CAPACITY, readptr: 0, writeptr: 0, + mode: Default::default(), } } } -impl Index for AllocRingBuffer { +impl Index for AllocRingBuffer { type Output = T; fn index(&self, index: isize) -> &Self::Output { @@ -254,7 +336,7 @@ impl Index for AllocRingBuffer { } } -impl IndexMut for AllocRingBuffer { +impl IndexMut for AllocRingBuffer { fn index_mut(&mut self, index: isize) -> &mut Self::Output { self.get_mut(index).expect("index out of bounds") } @@ -263,10 +345,45 @@ impl IndexMut for AllocRingBuffer { #[cfg(test)] mod tests { use super::alloc::vec::Vec; + use crate::with_alloc::RingbufferMode; use crate::{ - AllocRingBuffer, RingBuffer, RingBufferExt, RingBufferWrite, RINGBUFFER_DEFAULT_CAPACITY, + AllocRingBuffer, RingBuffer, RingBufferExt, RingBufferRead, RingBufferWrite, + RINGBUFFER_DEFAULT_CAPACITY, }; + // just test that this compiles + #[test] + fn test_generic_clone() { + fn helper( + a: &AllocRingBuffer, + ) -> AllocRingBuffer { + a.clone() + } + + _ = helper(&AllocRingBuffer::with_capacity(2)); + _ = helper(&AllocRingBuffer::with_capacity_non_power_of_two(5)); + } + #[test] + fn test_not_power_of_two() { + let mut rb = AllocRingBuffer::with_capacity_non_power_of_two(10); + const NUM_VALS: usize = 1000; + + // recycle the ringbuffer a bunch of time to see if noneof the logic + // messes up + for _ in 0..100 { + for i in 0..NUM_VALS { + rb.enqueue(i); + } + assert!(rb.is_full()); + + for i in 0..10 { + assert_eq!(Some(i + NUM_VALS - rb.capacity()), rb.dequeue()) + } + + assert!(rb.is_empty()) + } + } + #[test] fn test_default() { let b: AllocRingBuffer = AllocRingBuffer::default(); diff --git a/src/with_const_generics.rs b/src/with_const_generics.rs index 442f17d..4a25505 100644 --- a/src/with_const_generics.rs +++ b/src/with_const_generics.rs @@ -72,12 +72,22 @@ impl PartialEq for ConstGenericRingBuffer Eq for ConstGenericRingBuffer {} impl ConstGenericRingBuffer { + const ERROR_CAPACITY_IS_NOT_ALLOWED_TO_BE_ZERO: () = + assert!(CAP != 0, "Capacity is not allowed to be zero"); + /// Creates a const generic ringbuffer, size is passed as a const generic. + /// + /// Note that the size does not have to be a power of two, but that not using a power + /// of two might be significantly (up to 3 times) slower. #[inline] pub const fn new() -> Self { - assert!(CAP != 0, "Capacity is not allowed to be zero"); - assert!(CAP.is_power_of_two(), "Capacity must be a power of two"); + #[allow(clippy::let_unit_value)] + let _ = Self::ERROR_CAPACITY_IS_NOT_ALLOWED_TO_BE_ZERO; + // allow here since we are constructing an array of MaybeUninit + // which explicitly *is* defined behavior + // https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init + #[allow(clippy::uninit_assumed_init)] Self { buf: unsafe { MaybeUninit::uninit().assume_init() }, writeptr: 0, @@ -115,7 +125,7 @@ impl RingBufferRead for ConstGenericRingBuffer { if self.is_empty() { None } else { - let index = crate::mask(CAP, self.readptr); + let index = crate::mask_modulo(CAP, self.readptr); let res = mem::replace(&mut self.buf[index], MaybeUninit::uninit()); self.readptr += 1; @@ -144,7 +154,7 @@ impl RingBufferWrite for ConstGenericRingBuffer fn push(&mut self, value: T) { if self.is_full() { let previous_value = mem::replace( - &mut self.buf[crate::mask(CAP, self.readptr)], + &mut self.buf[crate::mask_modulo(CAP, self.readptr)], MaybeUninit::uninit(), ); // make sure we drop whatever is being overwritten @@ -156,7 +166,7 @@ impl RingBufferWrite for ConstGenericRingBuffer } self.readptr += 1; } - let index = crate::mask(CAP, self.writeptr); + let index = crate::mask_modulo(CAP, self.writeptr); self.buf[index] = MaybeUninit::new(value); self.writeptr += 1; } @@ -168,7 +178,7 @@ unsafe impl RingBufferExt for ConstGenericRingBuffer::new(); - } + fn test_not_power_of_two() { + let mut rb = ConstGenericRingBuffer::::new(); + const NUM_VALS: usize = 1000; + + // recycle the ringbuffer a bunch of time to see if noneof the logic + // messes up + for _ in 0..100 { + for i in 0..NUM_VALS { + rb.enqueue(i); + } + assert!(rb.is_full()); - #[test] - #[should_panic] - fn test_with_capacity_no_power_of_two() { - let _ = ConstGenericRingBuffer::::new(); + for i in 0..10 { + assert_eq!(Some(i + NUM_VALS - rb.capacity()), rb.dequeue()) + } + + assert!(rb.is_empty()) + } } #[test] diff --git a/tests/compile-fail/test_const_generic_array_zero_length.rs b/tests/compile-fail/test_const_generic_array_zero_length.rs new file mode 100644 index 0000000..b9caa56 --- /dev/null +++ b/tests/compile-fail/test_const_generic_array_zero_length.rs @@ -0,0 +1,9 @@ +extern crate ringbuffer; + +use ringbuffer::ConstGenericRingBuffer; + +fn main() { + let _ = ConstGenericRingBuffer::::new(); + //~^ note: the above error was encountered while instantiating `fn ringbuffer::ConstGenericRingBuffer::::new` + // ringbuffer can't be zero length +} \ No newline at end of file diff --git a/tests/compiletests.rs b/tests/compiletests.rs new file mode 100644 index 0000000..4fa9e1d --- /dev/null +++ b/tests/compiletests.rs @@ -0,0 +1,21 @@ +extern crate compiletest_rs as compiletest; + +use std::path::PathBuf; + +fn run_mode(mode: &'static str) { + let mut config = compiletest::Config::default(); + + config.mode = mode.parse().expect("Invalid mode"); + config.src_base = PathBuf::from(format!("tests/{}", mode)); + config.link_deps(); // Populate config.target_rustcflags with dependencies on the path + config.clean_rmeta(); // If your tests import the parent crate, this helps with E0464 + + compiletest::run_tests(&config); +} + +#[test] +#[cfg_attr(miri, ignore)] +#[cfg(not(tarpaulin))] +fn compile_test() { + run_mode("compile-fail"); +}