-
Couldn't load subscription status.
- Fork 228
vec: remove code duplication due to VecView. #486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Tested code size on company firmware: slight 0.02% bloat, but imo it's within the margin of error. |
|
I thought about this approach initially but didn't go for it because:
I'm not sure I see the benefits of that. My idea was that we would end up using On the other hand, I agree with the benefits of this approach:
|
|
Ok, on our firmware (we have 4 versions) we see the following sizes (in bytes): Current mainHeapless 0.8This PRThe biggest difference we can see is from heapless 0.8 to heapless main, with a difference of 0.3% smaller firmware size. The difference is lower that what I measured previously, but I'm not sure why (I tested with heapless 0.7 at the time). |
src/vec.rs
Outdated
| /// Trait defining how the data for a Vec is stored. | ||
| /// | ||
| /// There's two implementations available: | ||
| /// | ||
| /// - [`VecStorage`]: the storage for a [`Vec`]. Stores the data in an array `[MaybeUninit<T>; N]` whose size is known at compile time. | ||
| /// - [`VecViewStorage`]: the storage for a [`VecView`]. Stores the data in an unsized `[MaybeUninit<T>]`. | ||
| /// | ||
| /// This allows [`VecInner`] (the base struct for vectors) to be generic over either sized | ||
| /// or unsized storage. | ||
| /// | ||
| /// This trait is sealed, so you cannot implement it for your own types. You can only use | ||
| /// the implementations provided by this crate. | ||
| #[allow(private_bounds)] | ||
| pub trait Storage: SealedStorage {} | ||
|
|
||
| /// Implementation of [`Storage`] for [`Vec`]. | ||
| pub enum VecStorage<const N: usize> {} | ||
| impl<const N: usize> Storage for VecStorage<N> {} | ||
| impl<const N: usize> SealedStorage for VecStorage<N> { | ||
| type Buffer<T> = [MaybeUninit<T>; N]; | ||
| } | ||
|
|
||
| impl<T> VecBuffer for [MaybeUninit<T>] { | ||
| type T = T; | ||
|
|
||
| fn as_vecview(vec: &VecInner<Self>) -> &VecView<Self::T> { | ||
| vec | ||
| } | ||
| fn as_mut_vecview(vec: &mut VecInner<Self>) -> &mut VecView<Self::T> { | ||
| vec | ||
| } | ||
| /// Implementation of [`Storage`] for [`VecView`]. | ||
| pub enum VecViewStorage {} | ||
| impl Storage for VecViewStorage {} | ||
| impl SealedStorage for VecViewStorage { | ||
| type Buffer<T> = [MaybeUninit<T>]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think It would make sense to for these structs be moved in a common module for the crate, and rename the structs to Owned and View, so that the same structs can be used for other containers ?
I think it's clearer to use the same structs for all containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, done!
|
I would instead use: trait SealedStorage {
type Buffer<T>: ?Sized + Borrow<[T]> + BorrowMut<[T]>;
}And have pub struct VecInner<T, S: Storage>
len: usize,
buffer: S::Buffer<MaybeUninit<T>>,
}That would make the pub struct MpMcQueueInner<S: Storage> {
pub(super) dequeue_pos: AtomicTargetSize,
pub(super) enqueue_pos: AtomicTargetSize,
pub(super) buffer: UnsafeCell<S::Buffer<Cell<T>>>,
}Which can't be done if the |
yeah it's a rather weak advantage, but I can't see why not allow it.
very true, not all containers use MaybeUninit. Changed! |
| /// Trait defining how data for a container is stored. | ||
| /// | ||
| /// There's two implementations available: | ||
| /// | ||
| /// - [`OwnedStorage`]: stores the data in an array `[T; N]` whose size is known at compile time. | ||
| /// - [`ViewStorage`]: stores the data in an unsized `[T]`. | ||
| /// | ||
| /// This allows containers to be generic over either sized or unsized storage. | ||
| /// | ||
| /// This trait is sealed, so you cannot implement it for your own types. You can only use | ||
| /// the implementations provided by this crate. | ||
| #[allow(private_bounds)] | ||
| pub trait Storage: SealedStorage {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an intra-doc link to the documentation of VecView and Vec::as_view to explain what these traits are used for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed much cleaner for maintenance than the current duplicated documentation.
Since the size benefits are minor, I'm all for this PR.
I just wish it were possible to have the examples in the documentation of VecView to actually use VecView and not Vec but well, we can live with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation for Serialize for Vec should be over VecInner and generic over the Storage
|
Using For example |
done. I was also missing defmt, ufmt, done that too. |
String: implement `StringView` on top of #486
`MpMcQueue`: add `MpMcQueueView`, similar to `VecView` on top of #486
LinearMap: add LinearMapView, similar to VecView on top of #486
Implement HistoryBufferView on top of #486
BinaryHeap: implement BinaryHeapView on top of #486
Implement DequeView on top of #486
Queue: implement QueueView on top of #486
SortedLinkedList: add SortedLinkedListView, similar to VecView on top of #486
In 0.8.0 it was const, but this was removed in rust-embedded#486 The other container types did not make the `capacity` method const, and therefore can kept with the normal name and the generic implementation.
In 0.8.0 it was const, but this was removed in rust-embedded#486 The other container types did not make the `capacity` method const, and therefore can kept with the normal name and the generic implementation.
I'm a bit concerned about the code duplication introduced by #425. Most
Vecmethods forward to their
VecViewcounterparts, but the function signatureand the doc comments are still duplicated. This means every time we do a doc
fix or add a new method we have to remember to do it on both sides.
It has worked fine for for experimenting and proving the concept. However,
now we're at a stage where it's proven and we have to commit to
add it to all the other containers: #452 #459 #479 #480 #481 #485.
Each of these PRs adds 400-600 lines to the codebase, mostly duplicated
signatures and docs.
IMO we should find a way of adding
Viewtypes without so much duplicationfirst, then add it to all containers.
This PR implements a possible solution I've found that removes all the duplication:
Storagetrait with two impls, for sized and unsized.Storageinstead of over the buffer type. The buffer type is now aBufferassociated type.Storageis sealed. TheBufferassociated type is private. The user cannot learn theBuffertype for theStorageimpls, they can just use it withVecInnerand it makes it behave differently magically.VecInner<S>for anyS: Storage. This makes themavailable on both
VecandVecViewwithout duplicating anything.Advantages:
Storagenow, so you can e.g. compare aVecwith aVecView. Before you couldn't, you could only doVec-VecorVecView-VecView.Docs work fine on latest stable, it doesn't seem to need any workarounds:
Vecshow theVec-only methods and the storage-independent methods.VecViewshow theVecView-only methods (if we had some, we don't) and the storage-independent methods.Vecinnershow all methods.I also propose making
mod vecpublic, includingStorage, VecInner. The reasons are:vec::IntoIterwas already in the public API, but was previously unnameable.Storage, VecInnercan be useful for users that want to write code or trait impls generic over the storage.Storageis a sealed trait and theBuffertype is hidden, it's not leaking implementation details anymore. There's no[MaybeUninit<T>]in the public API, we can change it at any time without it being a breaking change.I think we also should remove the
Vec, VecViewreexports in the crate root, but this is left for a future PR.cc @sosthene-nitrokey