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

Lyndon Brown jnqnfe at gmail.com
Tue Sep 29 02:33:04 CEST 2020


On Mon, 2020-09-28 at 09:07 +0200, Alexandre Janniaux wrote:
> Hi,
> 
> In addition to Rémi's argument, I'll add that saving 16 bytes
> for a hundred plugins by adding more core to handle errors,
> while we are usually allocating more frames than needed is a
> micro optimization bringing complexity more than anything,
> which is quite different from a «free» save of space.

Hi,

As mentioned, the limit check (whether an assert or otherwise) serves
two purposes. 1) catching unexpected overflow with the smaller type, 2)
adding a missing check to catch unlikely bugs causing a runaway
generation of items (as unlikely as I feel that would be to ever
happen), rather than allow it to reach an OOM condition, just as some
other limits are enforced on plugin descriptors. The latter was an
enhancement I thought I should submit for consideration anyway, but had
the issue of arbitrarily picking a limit. Since the type change needed
a limit of UINT16_MAX, for simplicity I just let that be the limit, and
unfortunately chose to bundle that into the type change commit without
comment, which has not helped clear discussion.

If we consider the second purpose of the check to be a good idea, which
is an entirely separate matter to changing the types, then the change
of types is purely a change of types, no addition of checks.

> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Sat, Sep 26, 2020 at 10:41:23PM +0100, Lyndon Brown wrote:
> > attached. preview:
> > 
> > From: Lyndon Brown <jnqnfe at gmail.com>
> > Date: Wed, 10 Apr 2019 03:48:17 +0100
> > Subject: modules: use 16-bit ints for option counts
> > 
> > nothings is going to need more than UINT16_MAX options, let's be
> > sensible
> > and save the space. note, i may have instead used 8-bit ints, but
> > that's
> > way too small for the core set.
> > 
> > added an assert on size when adding modules in entry.c to be safe
> > though.
> > 
> > on 64-bit this saves 16 bytes per plugin (of which there are
> > typically
> > hundreds). i.e. we cut the storage space for this plugin conf data
> > to half
> > of what it currently is.
> > 
> > diff --git a/src/modules/entry.c b/src/modules/entry.c
> > index fc7f3cdbb6..cb06085b9c 100644
> > --- a/src/modules/entry.c
> > +++ b/src/modules/entry.c
> > @@ -142,6 +142,8 @@ static module_config_t
> > *vlc_config_create(vlc_plugin_t *plugin, int type)
> >      unsigned confsize = plugin->conf.size;
> >      module_config_t *tab = plugin->conf.items;
> > 
> > +    assert(confsize < UINT16_MAX);
> > +
> >      if ((confsize & 0xf) == 0)
> >      {
> >          tab = realloc_or_free (tab, (confsize + 17) * sizeof
> > (*tab));
> > diff --git a/src/modules/modules.h b/src/modules/modules.h
> > index 93cc1ca289..752b138c84 100644
> > --- a/src/modules/modules.h
> > +++ b/src/modules/modules.h
> > @@ -40,9 +40,9 @@ typedef struct vlc_plugin_t
> >      struct
> >      {
> >          module_config_t *items; /**< Table of configuration items
> > */
> > -        size_t size; /**< Total count of all items */
> > -        size_t count; /**< Count of real options (excludes hints)
> > */
> > -        size_t booleans; /**< Count of options that are of boolean
> > type */
> > +        uint16_t size; /**< Total count of all items */
> > +        uint16_t count; /**< Count of real options (excludes
> > hints) */
> > +        uint16_t booleans; /**< Count of options that are of
> > boolean type */
> >      } conf;
> > 
> >  #ifdef HAVE_DYNAMIC_PLUGINS
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel



More information about the vlc-devel mailing list