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

Fri Jan 25 10:30:17 EET 2019

On Fri, Jan 25, 2019 at 01:28:51AM +0100, Hendrik Leppkes wrote:
> 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.

I have no oppinion about these branch hints but this is starting to
feel alot more like a debate about code indention or some religious
debate than a technical feature. 

The arguments ive seen (and remember) ATM basically states or feel as:
* "we cannot as a group benchmark code in a usefull way", even though we
  seem able to do that in other cases
* "It will spread" even though it hasnt for example the (un)likely()
  macros in our own alpha code have been there peacefully for a very
  long term.
* Theres bad code "out there" using them so all use of them is bad
  or leads to bad code. compare "goto" use where similar arguments
  exists, we use gotos i think in many cases our use of gotos makes
  code clearer while you can do really really bad code with gotos if
  used unwisely

and no iam not suggesting we start using these branch hints more, i
just think they should be treated like any other tool or feature.

thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190125/8afd2223/attachment.sig>