[FFmpeg-devel] [PATCH 1/4] avformat/au: Store strings instead of pointers to strings in array

[Alexander Strasser]

On 2020-07-19 23:38 +0200, Andreas Rheinhardt wrote:
> Alexander Strasser:
[...]
> >
> > On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
> >> Andreas Rheinhardt:
> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> >>> ---
> >>>  libavformat/au.c | 13 ++++++-------
> >>>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/libavformat/au.c b/libavformat/au.c
> >>> index f92863e400..b419c9ed95 100644
> >>> --- a/libavformat/au.c
> >>> +++ b/libavformat/au.c
> >>> @@ -68,13 +68,13 @@ static int au_probe(const AVProbeData *p)
> >>>
> >>>  static int au_read_annotation(AVFormatContext *s, int size)
> >>>  {
> >>> -    static const char * keys[] = {
> >>> +    static const char keys[][7] = {
> >>>          "title",
> >>>          "artist",
> >>>          "album",
> >>>          "track",
> >>>          "genre",
> >>> -        NULL };
> >>> +    };
> >
> > Sorry, I couldn't test myself but wouldn't just
> >
> >     static const char * keys[] = {
> >         "title",
> >         "artist",
> >         "album",
> >         "track",
> >         "genre",
> >     };
> >
> > have been the better choice with the same benefits?
> >
> > I'm not sure without looking up the C standard and writing some
> > little test programs. From my guts I would say it's equivalent,
> > but the syntax is more convenient this way.
> >
> > That's also another issue with the commit message, since I don't
> > think it is true that in your version strings are stored in the
> > array. No offense intended and I sure might be mistaken.
> >
> But it is true.

Yes, you're right. I read the array dimension the wrong way around :(

Sorry for the noise.


> The strings are stored directly in the array, so that
> each array takes up 5 * 7 bytes. Before the change, the array itself
> took up 5 * sizeof(char *). And on top of that comes the space for the
> strings itself.
>
> Storing the strings itself in the array instead of pointers to the
> strings saves the space of the pointers, but it forces the shortest
> string to the size of the longest string. Therefore it is worthwhile if
> the average size differs from the longest size by less than sizeof(char
> *); there is furthermore one exception to this rule: If one stores
> pointers, different pointers may point to the same string (or to a part
> of a larger string). In the case here, the strings itself are smaller
> than sizeof(char *) on 64bit systems, so this alone shows that one saves
> space.

So it's about 40 bytes on systems with 64 addresses, right?


> Also notice that there are two more differences:
> a) The earlier version consisted of an array of nonconst pointers to
> const strings. Making the array entries const has been forgotten (it
> would go like this: "static const char *const keys[]"). Given that
> arrays are automatically nonmodifiable, this problem doesn't exist.
> (Given that these arrays are static and given that the compiler can see
> that they are not modified means that the compiler could infer that they
> are const and put them in the appropriate section for const data.)
> b) The actual access to the string is different: If the array contains
> pointer to strings, then one has to read the array entry (containing the
> pointer to the string) and pass it to av_strcasecmp() etc. If the array
> contains the strings, then one just has to get the address of the array
> entry (which coincides with the address of the string itself) and pass
> it to av_strcasecmp() etc..

I agree the new version is technically superior.

It's also more rigid, but that should probably not matter much in
this specific case.

Still I think the savings are marginal, so I guess you did them
while looking into the code for fixing the things later in this
patch set.

Fair enough.

A small additional sentence in the commit message body, would have
been better IMHO:

   Save a bit of space and one layer of indirection.



Best regards,
  Alexander