[FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely

Hendrik Leppkes h.leppkes at gmail.com
Fri Jan 25 02:28:51 EET 2019


On Thu, Jan 24, 2019 at 11:06 PM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> On Thu, Jan 24, 2019 at 06:08:57PM -0300, James Almer wrote:
> > On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote:
> > > On Thu, 24 Jan 2019 at 20:38, Marton Balint <cus at passwd.hu> wrote:
> > >
> > >> Signed-off-by: Marton Balint <cus at passwd.hu>
> > >> ---
> > >>  doc/APIchanges         | 3 +++
> > >>  libavutil/attributes.h | 8 ++++++++
> > >>  libavutil/version.h    | 2 +-
> > >>  3 files changed, 12 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/doc/APIchanges b/doc/APIchanges
> > >> index a39a3ff2ba..38b5b980a6 100644
> > >> --- a/doc/APIchanges
> > >> +++ b/doc/APIchanges
> > >> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> > >>
> > >>  API changes, most recent first:
> > >>
> > >> +2019-01-xx - xxxxxxxxxx - lavu 56.27.100 - attributes.h
> > >> +  Add av_likely and av_unlikely
> > >> +
> > >>  2019-01-08 - xxxxxxxxxx - lavu 56.26.100 - frame.h
> > >>    Add AV_FRAME_DATA_REGIONS_OF_INTEREST
> > >>
> > >> diff --git a/libavutil/attributes.h b/libavutil/attributes.h
> > >> index ced108aa2c..60972e5109 100644
> > >> --- a/libavutil/attributes.h
> > >> +++ b/libavutil/attributes.h
> > >> @@ -164,4 +164,12 @@
> > >>  #    define av_noreturn
> > >>  #endif
> > >>
> > >> +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__)
> > >> +# define av_likely(x)      __builtin_expect(!!(x), 1)
> > >> +# define av_unlikely(x)    __builtin_expect(!!(x), 0)
> > >> +#else
> > >> +# define av_likely(x)      (x)
> > >> +# define av_unlikely(x)    (x)
> > >> +#endif
> > >> +
> > >>  #endif /* AVUTIL_ATTRIBUTES_H */
> > >> diff --git a/libavutil/version.h b/libavutil/version.h
> > >> index 1fcdea95bf..12b4f9fc3a 100644
> > >> --- a/libavutil/version.h
> > >> +++ b/libavutil/version.h
> > >> @@ -79,7 +79,7 @@
> > >>   */
> > >>
> > >>  #define LIBAVUTIL_VERSION_MAJOR  56
> > >> -#define LIBAVUTIL_VERSION_MINOR  26
> > >> +#define LIBAVUTIL_VERSION_MINOR  27
> > >>  #define LIBAVUTIL_VERSION_MICRO 100
> > >>
> > >>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> > >> --
> > >> 2.16.4
> > >>
> > >> _______________________________________________
> > >> ffmpeg-devel mailing list
> > >> ffmpeg-devel at ffmpeg.org
> > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > >
> > >
> > > NAK, NAK, NAK.
> > > I will NAK the hell out of any patch that does that. They're completely
> > > unnecessary, they're commonly used by complete idiots who add them thinking
> > > their code will go faster (and it might - only on their antiquated GCC
> > > 3.1), they're religiously used by the same people and won't back down on
> > > using them and finally they're ugly when added to every single bloody
> > > branch and the people who maintain such code will demand they be added to
> > > every single bloody branch for no reason other that what I've just iterated
> > > on.
> > > The ONLY way I'll accept them is in platform-specific files, e.g.
> > > libavcodec/ppc/*, and even then only on non-x86 arches (which need them for
> > > having bad compilers with primitive processors having awful branch
> > > prediction) and even then I'd never accept their inclusioin into
> > > system-installed headers.
> >
> > What about hot loops with branches where you can use these as a hint for
> > the compiler to assume a specific outcome is highly unlikely to happen,
> > like alloc errors, corner cases in some codec, etc, which it simply has
> > no way to correctly guess at compile time?
> >
> > I don't think it should be NAKed because it's misused in other projects.
> > We're not other projects. We should instead simply ask the patch writer
> > to provide numbers that prove using it makes a difference in their code
> > with a recent version of GCC/Clang, and if it's negligible or within the
> > margin of error we'll simply ask to remove it to keep the code clean.
>
> if we want to ensure this, it can be enforced easily
> we have fate-source, that can check litterally for each av_(un)likely
> to contain a comment on the same line listing a non negligible performance
> effect. as in // branch hint increases speed by 5%

I'm generally not a fan of these hints at all, since the majority of
cases its just noise. The performance impact can vary extremely
between compilers and CPUs used, and in average its probably minimal
at best.
Even if you comment it with some speed number, it'll most of the time
be limited to one specific combination of compiler and CPU, and as
such any documented numbers are mostly meaningless.

Which is the entire problem with these likely/unlikely hints in the
first place. If you want to enforce using them in places where it
"matters", then how do you define that? One compiler on one system?
Two compiler? Two systems? Two architectures even?
You easily run into a huge potential for either endless bickering
about numbers, or a lot of questionable changes with unreproducable
"improvements" - ie. the abuse you see in every other project that has
them.

>
> OTOH, it may make more sense to gather branch statistics at runtime and
> include that in the next build without smudging the source
>

PGO (Profile Guided Optimizations) is of course something that exists
today, but pulling it off on a broad scale with a project like ffmpeg
would be quite a challenge.
Of course if all you care about is that one specific component is
fast, then it can probably be done.

- Hendrik


More information about the ffmpeg-devel mailing list