heapless icon indicating copy to clipboard operation
heapless copied to clipboard

Implement a generic length parameter for Vec<T, N>

Open GnomedDev opened this issue 1 year ago • 5 comments

Currently, Vec<T, N> always uses usize for the length field, which leads to silly situations like Vec<u8, 4> being 8 bytes large on 32 bit platforms. This PR introduces a generic to provide this length field as well as a default for many common length values to avoid much user code changing.

I am okay to revert sections of this PR if wanted, such as

  • ~~The change of usize to LenT in many arguments and return types, if they are considered too breaking.~~ Done, usize is still used for indexing and as the len return type.
  • Adding the length generic to VecView, this can be changed to only store usize at the cost of only Vec<T, N, usize> being able to unsize (or requiring a Vec::cast_len_type call before unsizing, at least).

This lays the ground work for followup PRs to implement a similar length generic on the other containers, but I'm starting with just Vec to test the waters.

GnomedDev avatar Jul 18 '24 20:07 GnomedDev

May I ask what your motivation is for this change? Do you have any real-world use cases for it?

This seems like a premature optimization to me, and it might confuse users who are new to Rust, especially when they need to specify the full type of Vec.

If this change is necessary, a better approach might be to add LenT only to VecInner and let Vec use it with LenT=usize. This would keep the API simple while still allowing advanced users to leverage VecInner when needed.

t-moe avatar Jul 18 '24 21:07 t-moe

I have been interested in performing this kind of optimization to a stack array for a while, and in-fact have a PR open to arrayvec to implement this, but the case that pushed me over the edge was embedded-tls's usage of many 1 byte large Ts in nested Vec's in enums leading to 160 bytes of data to be take up 328 bytes of storage.

Users who are new to Rust will most likely not hit this, as they will not be generic over the length type and therefore will get the exact same experience due to the default. If they do stray into being generic or using large N values, it is explained in documentation and in the error message what to do (pick the smallest integer type for your N, or usize).

GnomedDev avatar Jul 18 '24 22:07 GnomedDev

Sorted out all of your review comments, @reitermarkus!

GnomedDev avatar Jul 19 '24 15:07 GnomedDev

Interestingly, I made a similar MR in #204 2 years ago, but it was rejected for complexity.

YuhanLiin avatar Jul 25 '24 05:07 YuhanLiin

It doesn't look like it was rejected for complexity, although that PR is significantly more complex due to typenum bounds, it was just closed because of the migration to const generics.

GnomedDev avatar Jul 25 '24 10:07 GnomedDev

@GnomedDev, sorry for the long wait. Can you have a look at rebasing this?

reitermarkus avatar Apr 05 '25 23:04 reitermarkus

I'll take a look at rebasing it in the next week, if I don't get around to it after that time feel free to close or take over this PR.

GnomedDev avatar Apr 05 '25 23:04 GnomedDev

It looks like this PR is going to have to be practically rewritten due to the VecStorage change... but I'll see if I can do that.

GnomedDev avatar Apr 06 '25 10:04 GnomedDev

@reitermarkus Done, please note that I just did this from scratch instead of rebasing as it was conflicting so much, so keep that in mind while reviewing.

GnomedDev avatar Apr 06 '25 13:04 GnomedDev

@GnomedDev, needs another rebase, but should be good to go after that.

Adding the length generic to VecView, this can be changed to only store usize at the cost of only Vec<T, N, usize> being able to unsize (or requiring a Vec::cast_len_type call before unsizing, at least).

I'm quite split on this: If VecView only supports usize, it's easier to use, but then Vec will have to use usize by default or it gets confusing, so the space-saving becomes opt-in. I sightly prefer the space-saving by default, since it applies to the most common sizes with N < 256, and keeping Vec and VecView more consistent.

reitermarkus avatar Apr 08 '25 21:04 reitermarkus

@reitermarkus I've rebased, happy for this to finally get merged in.

GnomedDev avatar Apr 10 '25 16:04 GnomedDev

Thanks!

reitermarkus avatar Apr 10 '25 17:04 reitermarkus