rust-cv / header-vec

Allows one to store a header struct and a vector all inline in the same memory on the heap and share weak versions for minimizing random lookups in data structures
MIT License
5 stars 2 forks source link

Isn't the initial capacity insufficient whenever `align_of::<H> >= align_of::<T>`? #5

Closed makoConstruct closed 3 years ago

makoConstruct commented 3 years ago

Let's say we have HeaderVec<u32, u8>::new(7u32). new allocates an inital capacity of 1, so from the looks of things, that would get us an allocation layout of

alloc::alloc::Layout::from_size_align(
    1, // Self::elems_to_mem_bytes(capacity),
    4, // cmp::max(mem::align_of::<H>(), mem::align_of::<T>()),
)

Which, the documentation seems to suggest would round the allocation up to 4 bytes. This is only enough to store the header val. The remaining u8 in that the vector is supposed to be able to hold wouldn't actually be allocated for?

The problem is that the size of the memory layout has to be bigger than the capacity of the vec, but they're being treated as if they're the same.

This would only affect the last size_of::<H> - (size_of<T> % size_of<H>) elements of the vec, or something like that, and I'd guess they'd often be kept protected from being messed with by other allocations by the allocator rounding up the allocation cell to contain more than is needed, so it's possible that the consequences of this have not been noticed yet.

I'll try to fix this... after lunch

vadixidav commented 3 years ago

Note the following code:

    /// Gives the offset in units of T (as if the pointer started at an array of T) that the slice actually starts at.
    #[inline(always)]
    fn offset() -> usize {
        // We need to first compute the first location we can start in align units.
        // Then we go from align units to offset units using mem::align_of::<T>() / mem::size_of::<T>().
        (mem::size_of::<HeaderVecHeader<H>>() + mem::size_of::<T>() - 1) / mem::size_of::<T>()
    }

    /// Compute the number of elements (in units of T) to allocate for a given capacity.
    #[inline(always)]
    fn elems_to_mem_elems(capacity: usize) -> usize {
        Self::offset() + capacity
    }

    /// Compute the number of elements (in units of T) to allocate for a given capacity.
    #[inline(always)]
    fn elems_to_mem_bytes(capacity: usize) -> usize {
        Self::elems_to_mem_elems(capacity) * mem::size_of::<T>()
    }

    /// Compute the number of elements (in units of T) to allocate for a given capacity.
    #[inline(always)]
    fn layout(capacity: usize) -> alloc::alloc::Layout {
        alloc::alloc::Layout::from_size_align(
            Self::elems_to_mem_bytes(capacity),
            cmp::max(mem::align_of::<H>(), mem::align_of::<T>()),
        )
        .expect("unable to produce memory layout with Hrc key type (is it a zero sized type? they are not permitted)")
    }

This is where from_size_align is used. It is passed Self::elems_to_mem_bytes(capacity).

Self::elems_to_mem_bytes(capacity) returns Self::elems_to_mem_elems(capacity) * mem::size_of::<T>().

Self::elems_to_mem_elems(capacity) returns Self::offset() + capacity

Self::offset() returns (mem::size_of::<HeaderVecHeader<H>>() + mem::size_of::<T>() - 1) / mem::size_of::<T>()

Self::offset() gets the first offset in size units of T. Basically, it reserves space for the header by reserving several T-sized units at the beginning.

Self::elems_to_mem_elems(capacity) then returns the offset (in your example 4) plus the capacity 1, so it returns 5.

Self::elems_to_mem_bytes(capacity) then multiplies 5 by 1 (the number of bytes per T)

So ultimately, you will get:

alloc::alloc::Layout::from_size_align(
    5,
    4,
)

Let me know if this is incorrect though, but I believe this should be fine.

makoConstruct commented 3 years ago

Ah, sorry about that. I will try to read more thoroughly in future. (I think I sort of assumed some vague combination of some vague readings that either elems_to_mem_elems wasn't internal or just did what I guessed it would do)