[FFmpeg-devel] [PATCH] avcodec/mpegaudio_tablegen: more dynamic initialization speedups

[Ganesh Ajjanagadde]

On Sat, Nov 28, 2015 at 4:41 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sat, Nov 28, 2015 at 12:46:31AM -0500, Ganesh Ajjanagadde wrote:
>> This further speeds up runtime initialization, with identical generated tables.
>>
>> Sample benchmark (x86-64, Haswell, GNU/Linux):
>>
>> old:
>> 34441423 decicycles in mpegaudio_tableinit,    8192 runs,      0 skips
>>
>> new:
>> 10776291 decicycles in mpegaudio_tableinit,    8192 runs,      0 skips
>>
>> Most low hanging fruit is taken care of here. For some idea, note that
>> 83,064 array elements totalling 233,722 bytes need to be initialized.
>> Thus, with this patch, we average ~ 12.9 cycles per element or ~ 4.6
>> cycles per byte.
>>
>> I personally consider this net ~ 10x and overall perf numbers sufficient
>> for using dynamic initialization all the time here, especially since the
>> tables are large.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> -------------------------------------------------------------------------------
>> The reason this is being posted before pushing in the other one is that if
>> people agree to do dynamic initialization here, the introduction of avutil/tablegen
>> can be deferred to a future date.
>>
>> Note that if one had a ~8000 element static lut for the pow_43 values,
>> one can bring down the cost slightly, to ~ 8-10 cycles per element but not more,
>> so definitely not an order of improvement like the current patches.
>> I personally do not like this "middle path" as I find it too complex for a slight
>> speed gain, while still having a large ~ 64,000 byte size cost.
>> -------------------------------------------------------------------------------
>>  libavcodec/mpegaudio_tablegen.h | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/libavcodec/mpegaudio_tablegen.h b/libavcodec/mpegaudio_tablegen.h
>> index dd67a09..91b29cb 100644
>> --- a/libavcodec/mpegaudio_tablegen.h
>> +++ b/libavcodec/mpegaudio_tablegen.h
>
> seems this doesnt apply

It was not meant to be directly applied, but only after the other
mpegaudio_tablegen.h patch:
https://ffmpeg.org/pipermail/ffmpeg-devel/2015-November/183914.html
has been applied with the first speedup.

There might also be an issue with my comments placed in ---- //comment
----; I don't know if that is the right way of adding such things.

The comment was added simply to clarify - because if we go forward and
decide to do this dynamically, I will create another patch for getting
rid of hard-coded tables ifdefry here (and make it into a nice 3 patch
series).
In such a case, I don't want to bother with avutil/tablegen yet as it
has greater scope for regressions.
And if we don't want to do it dynamically, then there is no point at
all to these patches.

Lastly, the third option of retaining the hardcoded tables ifdefry
here to me seems pointless - I really like wm4's idea of deciding
cleanly which ones to do dynamically and which ones to do at build
time. This is one (as the comment suggests) where I personally feel
like doing it at runtime.

This file lacks a MAINTAINERS entry, but it seems like you maintain
other mpeg stuff. Thus, in the end and in absence of other comments, I
think it is your call. Here are the options, in decreasing order of my
personal preference:
1. Do this always at run time - this patch and the previous one are
worthy, but should be combined with removal of the ifdefry to get a 3
patch series.
2. Do this always at build time - then this patch and the previous one
in the link are useless and may be dropped.
3. Continue to be on the fence - this patch and the previous one are
worthy, since most clients do not do --enable-hardcoded-tables (vlc,
mpv, chromium?). But it needs to be combined with the avutil/tablegen
stuff, which has not been pushed/fully reviewed yet. This one I
strongly dislike, and agree with wm4 on this note.

[...]