Repository navigation
Add custom allocator support to Vec/Box conversions - #734
bolshoytoster wants to merge 8 commits into
Conversation
|
I don't think this is the way I think we should maintain the common having a double implementation is four times more cumbersome and I don't really see ROI what I suggest is having we must figure a way without two implementations |
|
maybe it'd be better to split this into two PRs as well?? |
|
I don't think it's possible to support with/without allocator in one implementation. The allocator one needs to be able to use |
|
are you 100% sure?? I think it'd be better to delay this and see if we find a way than to rush an implementation that breaks our codebase convention just because it now works |
|
Wouldn't we have to delay it until 1.100 is our MSRV to be able to do that? It's not just We could try using macros to reduce the duplication? |
|
I wonder if there's any possible type system hack that lets us say that for the no features case, macros might help but I'd rather not have to go into that rabbit hole, I'm already trying to figure out how to make visibility stuff easier |
|
Maybe something like this to smooth over the differences? trait Fromable<T, A> {
fn into_parts(self) -> (*mut T, usize, usize, A);
fn from_parts(*mut T, usize, usize, A) -> Self;
}
#[cfg(feature = "allocator-api")]
impl<T, A: Allocator> Fromable<T, A> for crate::Vec<T, A> {
fn into_parts(self) -> (*mut T, usize, usize, A) {
#[cfg(feature = "allocator-api2")]
self.into_raw_parts_with_alloc()
#[cfg(not(feature = "allocator-api2"))]
self.into_raw_parts_with_allocator()
}
fn from_parts(ptr: *mut T, length: usize, cap: usize, allocator: A) -> Self {
crate::Vec::from_raw_parts_in(ptr, length, cap, allocator)
}
}
#[cfg(any(not(feature = "allocator-api"), feature = "allocator-api2"))]
impl<T> Fromable<T, Global> for alloc::vec::Vec<T> {
fn into_parts(self) -> (*mut T, usize, usize, Global) {
let (ptr, length, cap) = self.into_raw_parts(),
(ptr, length, cap, Global)
}
fn from_parts(ptr: *mut T, length: usize, cap: usize, _: Global) -> Self {
alloc::vec::Vec::from_raw_parts(ptr, length, cap)
}
}Then we could implement I'm not sure if we'd run into conflicting implementations there. |
|
yeah I like that way we would have that implementation in so a and then we simply call those traits instead of dealing directly with the collection |
|
I'm not sure if this is possible actually: error[E0119]: conflicting implementations of trait `From<SmallVec<_, _, _>>` for type `SmallVec<_, _, _>`
--> src/conversions.rs:144:1
|
144 | impl<T, const N: usize, A: BaseAllocator, P: Parts<T, A>> From<P> for SmallVec<T, N, A> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `core`:
- impl<T> From<T> for T;
= note: downstream crates may implement trait `conversions::Parts<_, _>` for type `SmallVec<_, _, _>`
For more information about this error, try `rustc --explain E0119`.
error: could not compile `smallvec` (lib) due to 1 previous error |
|
give a thought as well to the same for box |
|
make |
How do I do that? Adding |
|
then simply fuck it and have the |
|
we should also consider making |
|
I could maybe have a |
|
go ahead try |
| if slice.len() > Self::inline_size() { | ||
| // Standard Rust vectors are already specialized. | ||
| Self::from_vec(Vec::from(slice)) | ||
| alloc::vec::Vec::from(slice).into() |
There was a problem hiding this comment.
we should use our Vec, not the one on alloc
why do this??
There was a problem hiding this comment.
The comment above states that standard rust vector From<&[T]> is specialized. I don't thing allocator-api2's Vec is.
There was a problem hiding this comment.
yeah but that's not our problem
if it is specialized, cool, we prefer those paths
but if it isn't idk, trying to force a vec we aren't using always doesn't look right to me
are you sure allocator-api2's vec isn't?? I thought it was a mirror of what was in std more or less, though I guess they can't have the specialization feature because it is nightly
does it matter that much to justify using another vec internally here?? I'm not sure
There was a problem hiding this comment.
I don't see the benefit of using allocator-api2's Vec if we're not using its Allocator.
We'd be sacrificing specialization for nothing.
There was a problem hiding this comment.
#[cfg(not(no_global_oom_handling))]
#[stable(feature = "rust1", since = "1.0.0")]
impl<T: Clone> From<&[T]> for Vec<T> {
/// Allocates a `Vec<T>` and fills it by cloning `s`'s items.
///
/// # Examples
///
/// ```
/// assert_eq!(Vec::from(&[1, 2, 3][..]), vec![1, 2, 3]);
/// ```
fn from(s: &[T]) -> Vec<T> {
s.to_vec()
}
}there isn't any specialization on alloc::vec::Vec
There was a problem hiding this comment.
yeah now I see
what are the potential downsides of using alloc::vec::Vec instead of crate::Vec??
I'm thinking: having a completely different type and codegen involved, maybe removing some optimizations that can come from it?
it might be the best thing to have the alloc's vec there but it isn't definitely clean or clear to explain
There was a problem hiding this comment.
Are you aware of any crates that exclusively use allocator_api2::vec::Vec (and never std Vec)?
Most uses of allocator-api seem to only use it in hot parts, using regular allocations everywhere else.
I don't think binary bloat would be much of an issue here, since to_vec_in will be either a pretty small loop or even just a memcpy.
There was a problem hiding this comment.
as far as I can see, it is just that on items that can be trivially cloned, it removes the dropguard behavior and just copies easily
I'm not aware of any crates that exclusively use allocator_api2::vec::Vec, but this only happens in our crate when we have the allocator-api2 feature enabled, because we are using crate::Vec
I wonder if this can cause us any issues in the future. it's just not good on our part that we use two different vec types when it suits us and if something changes in the future it can become messy fast
| // If M > N, we'd have to heap allocate anyway, | ||
| // so delegate for Vec for the allocation. | ||
| Self::from(Vec::from(array)) | ||
| alloc::vec::Vec::from(array).into() |
| #[cfg(not(feature = "allocator-api"))] | ||
| use Allocator as BaseAllocator; | ||
| #[cfg(feature = "allocator-api2")] | ||
| use allocator_api2::alloc::Allocator as BaseAllocator; | ||
| #[cfg(all(feature = "allocator-api", not(feature = "allocator-api2")))] | ||
| use core::alloc::Allocator as BaseAllocator; | ||
|
|
||
| trait Parts<T, A> { | ||
| fn into_parts(self) -> (*mut T, usize, usize, A); | ||
| unsafe fn from_parts(ptr: *mut T, length: usize, cap: usize, allocator: A) -> Self; | ||
| } | ||
|
|
||
| #[cfg(feature = "allocator-api")] | ||
| impl<T, A: BaseAllocator> Parts<T, A> for crate::Vec<T, A> { | ||
| fn into_parts(self) -> (*mut T, usize, usize, A) { | ||
| #[cfg(feature = "allocator-api2")] | ||
| { | ||
| self.into_raw_parts_with_alloc() | ||
| } | ||
| #[cfg(not(feature = "allocator-api2"))] | ||
| { | ||
| self.into_raw_parts_with_allocator() | ||
| } | ||
| } | ||
|
|
||
| unsafe fn from_parts(ptr: *mut T, length: usize, cap: usize, allocator: A) -> Self { | ||
| unsafe { crate::Vec::from_raw_parts_in(ptr, length, cap, allocator) } | ||
| } | ||
| } | ||
|
|
||
| impl<T, const N: usize, A: Allocator> From<SmallVec<T, N, A>> for Vec<T> { | ||
| fn from(this: SmallVec<T, N, A>) -> Self { | ||
| let (length, on_heap) = this.length.parts(); | ||
| if !on_heap { | ||
| let mut vec = Vec::with_capacity(length); | ||
| let this = ManuallyDrop::new(this); | ||
| // SAFETY: we create a new vector with sufficient capacity, copy our | ||
| // elements into it to transfer ownership and then set | ||
| // the length we don't drop the elements we previously | ||
| // held | ||
| unsafe { | ||
| copy_nonoverlapping(this.raw.as_ptr_inline(), vec.as_mut_ptr(), length); | ||
| vec.set_len(length); | ||
| #[cfg(any(not(feature = "allocator-api"), feature = "allocator-api2"))] | ||
| impl<T> Parts<T, Global> for alloc::vec::Vec<T> { | ||
| fn into_parts(self) -> (*mut T, usize, usize, Global) { | ||
| let mut this = ManuallyDrop::new(self); | ||
| (this.as_mut_ptr(), this.len(), this.capacity(), Global) | ||
| } | ||
|
|
||
| unsafe fn from_parts(ptr: *mut T, length: usize, cap: usize, _: Global) -> Self { | ||
| unsafe { alloc::vec::Vec::from_raw_parts(ptr, length, cap) } | ||
| } | ||
| } | ||
|
|
||
| impl<T, const N: usize, A: BaseAllocator> SmallVec<T, N, A> { | ||
| fn from_vec_helper(vec: impl Parts<T, A>) -> Self { | ||
| let (ptr, length, cap, allocator) = vec.into_parts(); | ||
|
|
||
| if cap == 0 { | ||
| return Self::new_in(allocator); | ||
| } | ||
|
|
||
| // SAFETY: A `Vec` always has a non-null pointer. | ||
| let ptr = unsafe { NonNull::new_unchecked(ptr) }; | ||
|
|
||
| if N < cap { | ||
| Self { | ||
| length: LocatedLength::new(length, !Self::IS_ZST), | ||
| raw: RawSmallVec { | ||
| heap: (ptr, cap) | ||
| }, | ||
| allocator | ||
| } | ||
| vec | ||
| } else { | ||
| let this = ManuallyDrop::new(this); | ||
| // SAFETY: | ||
| // - `ptr` was created with the SmallVec's allocator | ||
| // - `ptr` was created with the appropriate alignment for `T` | ||
| // - the allocation pointed to by ptr is exactly cap * sizeof(T) | ||
| // - `length` is less than or equal to `cap` | ||
| // - the first `length` entries are proper `T`-values | ||
| // - the allocation is not larger than `isize::MAX` | ||
| let mut inline = MaybeUninit::uninit(); | ||
| // SAFETY: vec.capacity() <= N | ||
| unsafe { | ||
| copy_nonoverlapping(ptr.as_ptr(), &raw mut inline as *mut T, length); | ||
| // We have to manually deallocate vec's memory, since we need to | ||
| // move its allocator out, meaning we can't drop it | ||
| allocator.deallocate( | ||
| ptr.cast(), | ||
| Layout::from_size_align_unchecked(cap * size_of::<T>(), align_of::<T>()) | ||
| ); | ||
| } | ||
| Self { | ||
| length: LocatedLength::new(length, false), | ||
| raw: RawSmallVec { | ||
| inline: ManuallyDrop::new(inline) | ||
| }, | ||
| allocator | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn into_vec_helper<P: Parts<T, A>>(self) -> P { | ||
| let (length, on_heap) = self.length.parts(); | ||
| let this = ManuallyDrop::new(self); | ||
| unsafe { | ||
| // SAFETY: we don't call any allocator-using methods on `this`, so | ||
| // it's fine to copy it out | ||
| let allocator = core::ptr::read(&this.allocator); | ||
| if !on_heap { | ||
| let layout = | ||
| Layout::from_size_align_unchecked(length * size_of::<T>(), align_of::<T>()); | ||
| let ptr = Allocator::allocate(&allocator, layout) | ||
| .unwrap_or_else(|| handle_alloc_error(layout)) | ||
| .as_ptr() as *mut T; | ||
|
|
||
| copy_nonoverlapping(this.raw.as_ptr_inline(), ptr, length); | ||
|
|
||
| P::from_parts(ptr, length, length, allocator) | ||
| } else { | ||
| // SAFETY: | ||
| // - `ptr` was created with the SmallVec's allocator | ||
| // - `ptr` was created with the appropriate alignment for `T` | ||
| // - the allocation pointed to by ptr is exactly cap * sizeof(T) | ||
| // - `length` is less than or equal to `cap` | ||
| // - the first `length` entries are proper `T`-values | ||
| // - the allocation is not larger than `isize::MAX` | ||
| let (ptr, cap) = this.raw.heap; | ||
| Vec::from_raw_parts(ptr.as_ptr(), length, cap) | ||
| P::from_parts(ptr.as_ptr(), length, cap, allocator) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T, const N: usize, A: Allocator> From<SmallVec<T, N, A>> for Box<[T]> { | ||
| #[cfg(feature = "allocator-api")] | ||
| impl<T, const N: usize, A: BaseAllocator> From<crate::Vec<T, A>> for SmallVec<T, N, A> { | ||
| fn from(vec: crate::Vec<T, A>) -> Self { | ||
| Self::from_vec_helper(vec) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(any(not(feature = "allocator-api"), feature = "allocator-api2"))] | ||
| impl<T, const N: usize> From<alloc::vec::Vec<T>> for SmallVec<T, N, Global> { | ||
| fn from(vec: alloc::vec::Vec<T>) -> Self { | ||
| Self::from_vec_helper(vec) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "allocator-api")] | ||
| impl<T, const N: usize, A: BaseAllocator> From<SmallVec<T, N, A>> for crate::Vec<T, A> { | ||
| fn from(this: SmallVec<T, N, A>) -> Self { | ||
| Vec::from(this).into_boxed_slice() | ||
| this.into_vec_helper() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(any(not(feature = "allocator-api"), feature = "allocator-api2"))] | ||
| impl<T, const N: usize> From<SmallVec<T, N, Global>> for alloc::vec::Vec<T> { | ||
| fn from(this: SmallVec<T, N, Global>) -> Self { | ||
| this.into_vec_helper() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "allocator-api")] | ||
| impl<T, const N: usize, A: BaseAllocator> From<SmallVec<T, N, A>> | ||
| for crate::allocator::Box<[T], A> | ||
| { | ||
| fn from(this: SmallVec<T, N, A>) -> Self { | ||
| crate::Vec::from(this).into_boxed_slice() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(any(not(feature = "allocator-api"), feature = "allocator-api2"))] | ||
| impl<T, const N: usize> From<SmallVec<T, N, Global>> for alloc::boxed::Box<[T]> { | ||
| fn from(this: SmallVec<T, N, Global>) -> Self { | ||
| alloc::vec::Vec::from(this).into_boxed_slice() |
There was a problem hiding this comment.
this code isn't clean, and we shouldn't have any type of cfg regarding the allocator features in conversions.rs, it violates separation of concerns
There was a problem hiding this comment.
we have to do it in some other way
There was a problem hiding this comment.
I'm not aware of any better ways to do this.
There was a problem hiding this comment.
let's sit on it for a while and see if we can figure out something better
There was a problem hiding this comment.
okay I think I've figured it out
the way to do it is with an associated type on an uninhabited type that also has another associated type offloading the maybe allocator
it's complex but I think it can work
after #739 this should be straightforward, more or less
There was a problem hiding this comment.
Should I close this PR and let you implement that?
There was a problem hiding this comment.
no don't close it, you've also done the proper deprecation
so keep the codegen move and the deprecation
just change the deprecation version to 2.0.0-beta.3
| } | ||
|
|
||
| #[inline] | ||
| #[deprecated(since = "2.0.0", note = "use `From::<Vec<T>>::from` instead")] |
|
I've managed to make it work it'll bleed your eyes when you see it |
2 participants
Closes #702 and #714.
This deprecates
SmallVec::from_vecin favour ofFrom::from.This also changes the behaviour of
from_vec(nowfrom). Previously, when the vec's capacity was smaller than the inline capacity, it was resized to maintain theN < capacityinvariant. This caused an unnecessary reallocation. Now it just copies the contents to the inline buffer.It also doesn't specially handle ZSTs now, because that wasn't needed.
This PR contains 2 implementations for each trait: 1 for the generic conversion (
SmallVec<_, _, A>,Vec<_, A>andBox<_, A>) which is used with either the nightly API orallocator-api2. The other implementation is specifically for conversions usingGlobal. This is used when there is either no allocator API, or withallocator-api2, in order to always support the commonSmallVec<_, _, Global> <-> Vec<_, Global>conversion regardless of the feature flags.Using specialization, it could be possible to support converting with mismatched allocators (i.e.
SmallVec<_, _, Bumpalo> -> Vec<_, Global>) by just copying the items. I'm not sure if that's wanted.I've removed
into_vecandinto_boxed_slicebecause of issues caused by dealing with 2 seperateAllocatortraits.