[vlc-devel] [PATCH] modules: use 16-bit ints for option counts

[Lyndon Brown]

On Sun, 2020-09-27 at 21:56 +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 27. syyskuuta 2020, 21.38.08 EEST Lyndon Brown a écrit
> :
> > On Sun, 2020-09-27 at 09:27 +0300, Rémi Denis-Courmont wrote:
> > > No. size_t is the specified type for byte and array size in C.
> > 
> > Sorry? Where does the C standard define that size_t is the one and
> > only
> > correct type to use whenever handling/storing array sizes?
> 
> It's the result type of the sizeof(), and thus of our own
> ARRAY_SIZE() macro.

Sure, but that in itself doesn't exclude storing our counts in
something else when we'd like to, as long as we are sensible about it.

> > But I don't see any reason for holding us back from using only the
> > storage space that we actually require here.
> 
> VLC is not kernel code where very last byte matters. Readability and 
> maintainability is more important. And compared to the bytes wasted
> in, say, 
> picture buffer padding, this is negligible.

I appreciate all of that, but just because a space saving is small,
doesn't mean we shouldn't bother. I don't think that
readability/maintainability is exactly negatively impacted.

> > We don't need more than two bytes per count here, ever.
> 
> In reasonable cases. But in pathological cases, this patch is broken
> as it 
> lacks error handling. Meanwhile, with size_t, we know overflow is
> impossible.

? The largest case by far is that of the core which has in the region
of 430, with the complete total of all options being in the region of
about 2-3 thousand I seem to recall. In what scenario other than some
utter screw up of a bug is any one plugin ever going to approach
anywhere close to UINT16_MAX?

I don't think it's very fair to call the patch broken, an assert is
added to the function where options are added into the system as a
precaution to catching the unlikely scenario of there ever being such a
bug causing a huge volume of creation requests.

On Sun, 2020-09-27 at 22:02 +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 27. syyskuuta 2020, 21.38.08 EEST Lyndon Brown a écrit
> :
> > Whenever we rarely use the counts for indexing or iteration or
> > allocation,
> > they'll get auto-zero-extended to int/size_t size as necessary.
> 
> That's not entirely correct.
> 
> Historically, some ISA lacked instructions for loading and storing
> non-native 
> sizes, notably 16-bit values on 32-bit systems. While that is
> generally not a 
> problem anymore, contemporary ISAs still have limitations on 16-bit
> accesses.
> 
> For instance the ARM A64 ISA can load 2 contiguous 64-bit, signed 32-
> bit, 
> unsigned 32-bit values in a single instruction. It can't do that with
> 16-bit 
> values.

Okay, good to know, thanks. Though in this case the data is used so
little that it's also not a big deal; on the scale of triviality of the
space saving...