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

[Andreas Rheinhardt]

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?
> 
6 pointers amount to 48 bytes, but one also has to waste 4 bytes to make
the shorter strings as long as the longest.
> 
>> 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.
> 
Indeed, it is usually not suitable when one has too many strings.

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

- Andreas