On Sat, Nov 28, 2015 at 4:41 AM, Michael Niedermayer <michae...@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 <gajjanaga...@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. [...] _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
