Fixed capacity vec

I made a fixed capacity Vec for my BTreeMap implementation. Code here or as below. Note this is an internal module, any unsafety is encapsulated in the crate ( FixedCapVec methods do not check index/capacity when the unsafe-optim feature is enabled ). As in the comment at the top, I mostly cribbed the code from The Rustonomicon. I think it is fairly straight-forward, but nothing is ever entirely simple when it comes to unsafe code! It is not intended to handle zero-sized types, as I don't need them for BTreeMap (although maybe I need to enforce this).

I have not yet figured out why my implementation of binary search appears to be faster than the std::slice::binary_search_by , I don't see why it should be any different, but when I do tests it does seem to be faster.

use std::{
    alloc,
    alloc::Layout,
    cmp::Ordering,
    mem,
    ops::{Deref, DerefMut},
    ptr,
    ptr::NonNull,
};

/// Basic vec, does not have own capacity or length, just a pointer to memory.
/// Kind-of cribbed from <https://doc.rust-lang.org/nomicon/vec/vec-final.html>.
struct BasicVec<T> {
    p: NonNull<T>,
}

unsafe impl<T: Send> Send for BasicVec<T> {}
unsafe impl<T: Sync> Sync for BasicVec<T> {}

impl<T> Default for BasicVec<T> {
    fn default() -> Self {
        Self::new()
    }
}

impl<T> BasicVec<T> {
    /// Construct new BasicVec.
    pub fn new() -> Self {
        Self {
            p: NonNull::dangling(),
        }
    }

    /// Get mutable raw pointer to specified element.
    /// # Safety
    /// index must be < set capacity.
    #[inline]
    pub unsafe fn ix(&self, index: usize) -> *mut T {
        self.p.as_ptr().add(index)
    }

    /// Set capacity ( allocate or reallocate memory ).
    /// # Safety
    ///
    /// old_cap must be the previous capacity set, or 0 if no capacity has yet been set.
    pub unsafe fn set_cap(&mut self, old_cap: usize, new_cap: usize) {
        assert!(mem::size_of::<T>() != 0);

        let new_layout = Layout::array::<T>(new_cap).unwrap();

        let new_ptr = if old_cap == 0 {
            alloc::alloc(new_layout)
        } else {
            let old_layout = Layout::array::<T>(old_cap).unwrap();
            let old_ptr = self.p.as_ptr() as *mut u8;
            alloc::realloc(old_ptr, old_layout, new_layout.size())
        };

        // If allocation fails, `new_ptr` will be null, in which case we abort.
        self.p = match NonNull::new(new_ptr as *mut T) {
            Some(p) => p,
            None => alloc::handle_alloc_error(new_layout),
        };
    }

    /// Set value.
    /// # Safety
    ///
    /// ix must be < capacity, and the element must be unset.
    #[inline]
    pub unsafe fn set(&mut self, ix: usize, elem: T) {
        ptr::write(self.ix(ix), elem);
    }

    /// Get value.
    /// # Safety
    ///
    /// ix must be less < capacity, and the element must have been set.
    #[inline]
    pub unsafe fn get(&mut self, ix: usize) -> T {
        ptr::read(self.ix(ix))
    }

    /// Get whole as slice.
    /// # Safety
    ///
    /// len must be <= capacity and 0..len elements must have been set.
    #[inline]
    pub unsafe fn slice(&self, len: usize) -> &[T] {
        std::slice::from_raw_parts(self.p.as_ptr(), len)
    }

    /// Get whole as mut slice.
    /// # Safety
    ///
    /// len must be <= capacity and 0..len elements must have been set.
    #[inline]
    pub unsafe fn slice_mut(&mut self, len: usize) -> &mut [T] {
        std::slice::from_raw_parts_mut(self.p.as_ptr(), len)
    }

    /// Move elements.
    /// # Safety
    ///
    /// The set status of the elements changes in the obvious way. from, to and len must be in range.
    pub unsafe fn move_self(&mut self, from: usize, to: usize, len: usize) {
        ptr::copy(self.ix(from), self.ix(to), len);
    }

    /// Move elements from another BasicVec.
    /// # Safety
    ///
    /// The set status of the elements changes in the obvious way. from, to and len must be in range.
    pub unsafe fn move_from(&mut self, from: usize, src: &mut Self, to: usize, len: usize) {
        ptr::copy_nonoverlapping(src.ix(from), self.ix(to), len);
    }

    /// Free memory.
    /// # Safety
    ///
    /// The capacity must be the last capacity set.
    pub unsafe fn free(&mut self, cap: usize) {
        let elem_size = mem::size_of::<T>();

        if cap != 0 && elem_size != 0 {
            alloc::dealloc(self.p.as_ptr() as *mut u8, Layout::array::<T>(cap).unwrap());
        }
    }
}

/// In debug mode or feature unsafe-optim not enabled, same as assert! otherwise does nothing.
#[cfg(any(debug_assertions, not(feature = "unsafe-optim")))]
macro_rules! safe_assert {
    ( $cond: expr ) => {
        assert!($cond)
    };
}

/// In debug mode or feature unsafe-optim not enabled, same as assert! otherwise does nothing.
#[cfg(all(not(debug_assertions), feature = "unsafe-optim"))]
macro_rules! safe_assert {
    ( $cond: expr ) => {};
}

/// Vec with fixed capacity.
pub struct FixedCapVec<const CAP: usize, T> {
    len: usize,
    v: BasicVec<T>,
}

impl<const CAP: usize, T> FixedCapVec<CAP, T> {
    ///
    pub fn new() -> Self {
        let mut v = BasicVec::new();
        unsafe {
            v.set_cap(0, CAP);
        }
        Self { len: 0, v }
    }

    ///
    #[inline]
    pub fn len(&self) -> usize {
        self.len
    }

    ///
    #[inline]
    pub fn is_empty(&self) -> bool {
        self.len == 0
    }

    ///
    #[inline]
    pub fn push(&mut self, value: T) {
        safe_assert!(self.len < CAP);
        unsafe {
            self.v.set(self.len, value);
        }
        self.len += 1;
    }

    ///
    #[inline]
    pub fn pop(&mut self) -> Option<T> {
        if self.len == 0 {
            None
        } else {
            self.len -= 1;
            unsafe { Some(self.v.get(self.len)) }
        }
    }

    ///
    pub fn insert(&mut self, at: usize, value: T) {
        safe_assert!(at <= self.len && self.len < CAP);
        unsafe {
            if at < self.len {
                self.v.move_self(at, at + 1, self.len - at);
            }
            self.v.set(at, value);
            self.len += 1;
        }
    }

    ///
    pub fn remove(&mut self, at: usize) -> T {
        safe_assert!(at < self.len);
        unsafe {
            let result = self.v.get(at);
            self.v.move_self(at + 1, at, self.len - at - 1);
            self.len -= 1;
            result
        }
    }

    ///
    pub fn split_off(&mut self, at: usize) -> Self {
        safe_assert!(at < self.len);
        let len = self.len - at;
        let mut result = Self::new();
        unsafe {
            result.v.move_from(at, &mut self.v, 0, len);
        }
        result.len = len;
        self.len -= len;
        result
    }

    ///
    pub fn retain_mut<F>(&mut self, mut f: F)
    where
        F: FnMut(&mut T) -> bool,
    {
        unsafe {
            let mut i = 0;
            let mut r = 0;
            while i < self.len {
                if f(&mut *self.v.ix(i)) {
                    if r != i {
                        let v = self.v.get(i);
                        self.v.set(r, v);
                    }
                    r += 1;
                } else {
                    self.v.get(i);
                }
                i += 1;
            }
            self.len -= i - r;
        }
    }

    /// Get reference to ith element.
    #[inline]
    pub fn ix(&self, ix: usize) -> &T {
        safe_assert!(ix < self.len);
        unsafe { &*self.v.ix(ix) }
    }

    /// Get mutable reference to ith element.
    #[inline]
    pub fn ixm(&mut self, ix: usize) -> &mut T {
        safe_assert!(ix < self.len);
        unsafe { &mut *self.v.ix(ix) }
    }

    /// Same as binary_search_by, but for some obscure reason this seems to be faster.
    pub fn search<F>(&self, mut f: F) -> Result<usize, usize>
    where
        F: FnMut(&T) -> Ordering,
    {
        let (mut i, mut j) = (0, self.len);
        while i < j {
            let m = i + (j - i) / 2;
            match f(self.ix(m)) {
                Ordering::Equal => {
                    return Ok(m);
                }
                Ordering::Less => i = m + 1,
                Ordering::Greater => j = m,
            }
        }
        Err(i)
    }
}

impl<const CAP: usize, T> Deref for FixedCapVec<CAP, T> {
    type Target = [T];
    #[inline]
    fn deref(&self) -> &[T] {
        let len: usize = FixedCapVec::len(self);
        unsafe { self.v.slice(len) }
    }
}

impl<const CAP: usize, T> DerefMut for FixedCapVec<CAP, T> {
    #[inline]
    fn deref_mut(&mut self) -> &mut [T] {
        let len: usize = FixedCapVec::len(self);
        unsafe { self.v.slice_mut(len) }
    }
}

impl<const CAP: usize, T> Default for FixedCapVec<CAP, T> {
    fn default() -> Self {
        Self::new()
    }
}

impl<const CAP: usize, T> Drop for FixedCapVec<CAP, T> {
    fn drop(&mut self) {
        let mut len = self.len;
        while len > 0 {
            len -= 1;
            unsafe {
                self.v.get(len);
            }
        }
        unsafe {
            self.v.free(CAP);
        }
    }
}

#[test]
fn test() {
    let mut v = BasicVec::<u64>::new();
    unsafe {
        v.set_cap(0, 10);
        v.set(0, 99);
        v.set(1, 97);
        v.move_self(1, 2, 1);
        v.set(1, 98);

        {
            let s = v.slice_mut(3);
            for i in 0..3 {
                println!("v[{}] = {}", i, s[i]);
                s[i] += 1;
            }
        }

        {
            let s = v.slice(3);
            for i in 0..3 {
                println!("v[{}] = {}", i, s[i]);
            }
        }

        for i in 0..3 {
            println!("v.get({})={}", i, v.get(i));
        }

        v.free(10);
    }

    println!("testing FixedCapVec");

    let mut v = FixedCapVec::<10, u64>::new();
    v.push(99);
    v.push(97);
    v.insert(1, 98);

    for i in 0..v.len() {
        println!("v[{}] = {}", i, v[i]);
        v[i] += 1;
    }

    let mut w = v.split_off(1);
    for i in 0..v.len() {
        println!("v[{}] = {}", i, v[i]);
    }
    for i in 0..w.len() {
        println!("w[{}] = {}", i, w[i]);
    }

    w.remove(1);

    for i in 0..w.len() {
        println!("w[{}] = {}", i, w[i]);
    }
}

That may have UB when the assert is not running and should therefore also be marked as unsafe. Same for other methods. I have a mindset where I say I need to check all requirements before calling unsafe methods. This means either give deductive reasoning why something can or cannot occur (so called safety-comments), or dynamically checking any preconditions.

2 Likes

What you say is true, but it is internal and making the functions unsafe would mean maybe 30-50 unsafe blocks in the module above, which I don't think really accomplishes much, and makes that code less readable. I make a comment at at the top of the crate:

"Note: some (crate) private methods of FixedCapVec are techically unsafe in release mode when the unsafe_optim feature is enabled, but are not declared as such to avoid littering the code with unsafe blocks."

I did start putting loads of unsafe blocks in, then I decided it really didn't make much sense, The requirements are range checks which are in some sense "trivial" in the context of a BTreeMap ( where the entire basic logic is concerned with splitting vecs to keep them at a certain size ).

The alternative (which I do) is to use the unconditional assert! to guard against UB. Then the function itself does not need to be marked unsafe.

I am still considering the issue. The "normal" (and default) mode is assert!s which limit the unsafety to FixedCapVec. The purpose of the unsafe-optim feature is partly to allow measurement of how much index checks are costing. I could also just use the unsafe functions in situations where the compiler cannot automatically remove the asserts, which it can in most situations.

The thing is, a "hazard warning" loses effectiveness if it is plastered all over the place. I'd prefer to reserve it for the places where the unsafety risk is high... ( for example where I am now using raw pointers, which is super-scary stuff ).

See here for example:

"When a warning sign becomes mundane to employees, it no longer serves its intended purpose. If people are bombarded with too many safety communications, they may ignore a warning sign in a process area or skip over the cautions mentioned in a procedure."

https://www.aiche.org/resources/publications/cep/2019/may/process-safety-beacon-surrounded-too-many-warning-signs

The point of unsafe is to highlight the points in the code where you need to make sure that invariants are uphold. You can ofc have private unsoud functions but I would strongly advise to rather declare them unsafe and have a // SAFETY: comment that describes why the length is shorter than the capacity. The optimal solution would ofc be a proper safe interface that uses the type system. For example you could add a function try_reserve(&mut self) -> Option<Slot<'_>> that is called further up in the code.

5 Likes

Is it though? it's declared as pub. or are you referring to the entire module?

Either way this is a pretty bad case of soundness bug in my opinion. calling push on a full vec is instant UB.

in my mind unsafe is more than just a warning (that's why it's an actual language construct rather than comments). it's also a guarantee that certain invariants are upheld (in the case of blocks) or a contract that the caller must uphold them (in the case of functions).

4 Likes

For clarity, I now declare it as pub(crate), but at the crate level it was always imported as use not pub use. So it was always crate internal. The BTree logic ensures that push is not called when the vec is full, the asserts are extra belt-and-braces, which are normally enabled anyway. The "unsafe-optim" is a feature that is saying, "I trust the crate logic, don't worry about performing internal safety checks, I just want to go as fast as possible".

If everything can be misused, then everything deserves the hazard warning.

In order to use it correctly you have to know what the function requires and check it yourself on every call. So you should write your justification in the comments, which is what // SAFETY comments are all about.

If you haven't thought about those things, how do you know it's correct?

1 Like

I have thought about those things, and it is also a situation where testing is effective. Plastering unsafe all over the code will not make it any safer, if anything the risk is the other way.

I really wonder if this is the cause of confusion or disagreement. If this were an unconditional assert, or an assert that is only disabled for benchmarking, then the functions would clearly not need to be declared as unsafe -- under anyone's definition -- is that correct?

Yes, that is correct, if the asserts were unconditional, then the methods would no longer need to be marked unsafe. The purpose of the unsafe-optim feature is (a) to see if the extra checks make any difference ( in practice it is very hard to measure ) and (b) if someone trusts the crate logic and wants to go faster (maybe in a competition context... for example) or to save electricity, the asserts can be disabled.

Maybe you're getting pushback because this isn't normally done in Rust. Reviewing of unsafe code is meant to find problems that might happen in rare conditions. So allowing the asserts to be disabled means the functions are unsafe, and must be marked unsafe according to the design of unsafe Rust in general. It seems worthwhile to at least consider making the asserts unconditional, or perhaps to only allow disabling them by changing the code itself when running benchmarks.

4 Likes

It might be useful to review the definition of an unsafe block from the rust reference:

unsafe blocks state that all relevant proof obligations of functions or operations called inside the block have been discharged. There are many ways to discharge proof obligations; for example, there could be run-time checks or data structure invariants that guarantee that certain properties are definitely true, or the unsafe block could be inside an unsafe fn, in which case the block can use the proof obligations of that function to discharge the proof obligations arising inside the block.

Nota that if someone enables your feature it will be enabled for all crates in the dependency graph that depend on your crate, even those that might not have carefully checked all their logic.

2 Likes

The unsafety is completely encapsulated in the crate, the only logic that needs checking (to ensure memory safety) is the logic in the btree_experiment crate itself. Publishing FixedCapVec itself (without declaring the methods as unsafe) would I agree be wrong and against the normal expectation that unsafe methods are declared as such. As far as the feature being enabled, I think that it would be unusual for a library to do that, as it is a deployment choice ( so should be left to cargo install or cargo test time ).

You don't have control over that.

When it comes to the standard library, you are not given the choice, you either use the crate with unsafe code in it (std::collections::BTreeMap has unsafe even though it isn't strictly needed except for going faster), or write your own. I am actually giving people a choice, via the feature. I mean it does have unsafe code either way, but you get the choice to have more or less. Declaring the methods as unsafe (given they are with the feature enabled) would make the code in the main module significantly more unreadable, for no real gain. Pretty much every other line of (executable) code would be in an unsafe block. The chance of a logic error resulting in unsafe (and UB) leaking out of the crate would probably rise (due to the code becoming unreadable).

I completely understand why you're doing this. But I think you're really swimming upstream in terms of convincing others here that it is ok to not mark the functions unsafe. The Rust culture is all about providing safety guarantees, rather than providing safe and unsafe configuration options.

2 Likes

Rust is certainly about minimising and controlling memory-unsafe code, and that is a very good thing. I can understand the existence of the unsafe-optim option being controversial, and in fact for this crate I think it is probably not justified (I have other modules where I would say it is more justified as it can make a significant (albeit unspectacular) difference in some cases), but it is (somewhat) useful to me to try and eliminate index checks as a source of performance difference compared to the standard library implementation. With quite a lot of extra work (checking code generation) I could probably eliminate some of it, likely even most.