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

GnomedDev · 2024-07-18T20:11:10Z

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.

diondokter

Not a full review from me, I've not read every line. This is a fun change and really cool. I do have some concerns though

src/vec/mod.rs

diondokter · 2024-07-18T20:36:33Z

src/len_type.rs

+impl_lentodefault!(u8: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255);
+impl_lentodefault!(u16: 256, 300, 400, 500, 512, 600, 700, 800, 900, 1000, 1024, 2000, 2048, 4000, 4096, 8000, 8192, 16000, 16384, 32000, 32768, 65000, 65535);
+impl_lentodefault!(u32: 65536, 131072, 262144, 524288, 1048576, 2097152, 4194304, 8388608, 16777216, 33554432, 67108864, 134217728, 268435456, 536870912, 1073741824, 2147483648);


Oh this is quite fun!
I wonder if there's a way to cover all numbers without breaking the compiler :P

This doesn't actually cause too much stress on the compiler, it's surprising how fast the trait resolver is. From my knowledge (and I have done quite a bit of advanced type system logic, see https://github.com/GnomedDev/aformat) this is the only possible stable way to do this.

Since these containers probably shouldn't be used for N values above u8::MAX, let alone u16::MAX, I'm sure this will be fine, especially with the diagnostic::on_unimplemented making it super easy for users to fix.

In fact, if you'd like to go further about space efficiency, you can use less than a whole byte for cases where capacity < 255 by using enums with fewer variants, so that niche optimization could encode enum discriminants into the unused bit patterns.

On the other hand, with element type T's alignment > 1, it's often easier to find space in the padding bytes than to squeeze space from the actual LenType, but with the current default behavior of the type system (the contents of padding bytes are unspecified rather than fixed), I don't know the idiomatic way to let the container make its padding bytes specified.

A possible approach would be to make (T, ZeroPad) a LenType with the same behavior as <T as LenType>

This could be a future PR, but I'm trying to get this initial massive improvement before we start working on niche opt stuff.

t-moe · 2024-07-18T21:50:45Z

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.

GnomedDev · 2024-07-18T22:29:31Z

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).

src/vec/mod.rs

src/len_type.rs

src/string/mod.rs

GnomedDev · 2024-07-19T15:27:41Z

Sorted out all of your review comments, @reitermarkus!

YuhanLiin · 2024-07-25T05:59:17Z

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

GnomedDev · 2024-07-25T10:35:55Z

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 force-pushed the vec-generic-length branch from 7f97b86 to d52af1e Compare July 18, 2024 20:15

diondokter reviewed Jul 18, 2024

View reviewed changes

GnomedDev force-pushed the vec-generic-length branch from 011ee41 to 33c8b2e Compare July 18, 2024 21:46