-
Notifications
You must be signed in to change notification settings - Fork 185
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
vec: remove code duplication due to VecView. #486
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 main
Heapless 0.8
This PR
The 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! |
src/storage.rs
Outdated
/// 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
Vec
methods forward to their
VecView
counterparts, 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
View
types without so much duplicationfirst, then add it to all containers.
This PR implements a possible solution I've found that removes all the duplication:
Storage
trait with two impls, for sized and unsized.Storage
instead of over the buffer type. The buffer type is now aBuffer
associated type.Storage
is sealed. TheBuffer
associated type is private. The user cannot learn theBuffer
type for theStorage
impls, they can just use it withVecInner
and it makes it behave differently magically.VecInner<S>
for anyS: Storage
. This makes themavailable on both
Vec
andVecView
without duplicating anything.Advantages:
Storage
now, so you can e.g. compare aVec
with aVecView
. Before you couldn't, you could only doVec-Vec
orVecView-VecView
.Docs work fine on latest stable, it doesn't seem to need any workarounds:
Vec
show theVec
-only methods and the storage-independent methods.VecView
show theVecView
-only methods (if we had some, we don't) and the storage-independent methods.Vecinner
show all methods.I also propose making
mod vec
public, includingStorage, VecInner
. The reasons are:vec::IntoIter
was already in the public API, but was previously unnameable.Storage, VecInner
can be useful for users that want to write code or trait impls generic over the storage.Storage
is a sealed trait and theBuffer
type 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, VecView
reexports in the crate root, but this is left for a future PR.cc @sosthene-nitrokey