[FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
From: Ganesh Ajjanagadde These use __builtin_expect, and may be useful for optimizing nearly certain branches, to yield size and/or speed improvements. Note that this is used in the Linux kernel for the same purpose. For some idea as to potential benefits, see e.g http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html. Signed-off-by: Ganesh Ajjanagadde --- libavutil/attributes.h | 8 1 file changed, 8 insertions(+) diff --git a/libavutil/attributes.h b/libavutil/attributes.h index 5c6b9de..1547033 100644 --- a/libavutil/attributes.h +++ b/libavutil/attributes.h @@ -58,6 +58,14 @@ #define av_warn_unused_result #endif +#if AV_GCC_VERSION_AT_LEAST(3,0) +#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 + #if AV_GCC_VERSION_AT_LEAST(3,1) #define av_noinline __attribute__((noinline)) #elif defined(_MSC_VER) -- 2.7.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
On Thu, Feb 25, 2016 at 8:55 AM, Hendrik Leppkes wrote: > On Thu, Feb 25, 2016 at 8:27 AM, wm4 wrote: >> On Thu, 25 Feb 2016 15:48:17 +1100 >> Matt Oliver wrote: >> >>> On 25 February 2016 at 13:20, Ganesh Ajjanagadde wrote: >>> >>> > From: Ganesh Ajjanagadde >>> > >>> > These use __builtin_expect, and may be useful for optimizing nearly >>> > certain branches, to yield size and/or speed improvements. >>> > >>> > Note that this is used in the Linux kernel for the same purpose. For >>> > some idea as to potential benefits, see e.g >>> > http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html. >>> > >>> > Signed-off-by: Ganesh Ajjanagadde >>> > --- >>> > libavutil/attributes.h | 8 >>> > 1 file changed, 8 insertions(+) >>> > >>> > diff --git a/libavutil/attributes.h b/libavutil/attributes.h >>> > index 5c6b9de..1547033 100644 >>> > --- a/libavutil/attributes.h >>> > +++ b/libavutil/attributes.h >>> > @@ -58,6 +58,14 @@ >>> > #define av_warn_unused_result >>> > #endif >>> > >>> > +#if AV_GCC_VERSION_AT_LEAST(3,0) >>> > +#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 >>> > + >>> > #if AV_GCC_VERSION_AT_LEAST(3,1) >>> > #define av_noinline __attribute__((noinline)) >>> > #elif defined(_MSC_VER) >>> > >>> >>> Ive used these builtins before and can confirm that they are useful >>> (although requires devs to use them obviously). I will also point out that >>> these are also supported by ICC/ICL as well. >> >> They also make code ugly as fuck, and are more cargo-cult than anything >> else. They might possibly be ok in some very critical parts of the >> code, but otherwise not at all. >> > > I agree with wm4 on the ugly aspect, I always hate reading code from > projects that use it, its terrible on readability. > If its used at all, it should be proven that (a) the function is > performance relevant, and (b) that the improvement is actually > tangible in the grand scheme. I mostly agree with the sentiments. Patchset shelved until a non-trivial, critical use case can be found. @Matt: feel free to extend to ICC if/when this happens; I lack it and can't test. Thanks all. > > - Hendrik > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
Hi, On Sat, Feb 27, 2016 at 8:11 AM, Ganesh Ajjanagadde wrote: > On Thu, Feb 25, 2016 at 8:55 AM, Hendrik Leppkes > wrote: > > On Thu, Feb 25, 2016 at 8:27 AM, wm4 wrote: > >> On Thu, 25 Feb 2016 15:48:17 +1100 > >> Matt Oliver wrote: > >> > >>> On 25 February 2016 at 13:20, Ganesh Ajjanagadde > wrote: > >>> > >>> > From: Ganesh Ajjanagadde > >>> > > >>> > These use __builtin_expect, and may be useful for optimizing nearly > >>> > certain branches, to yield size and/or speed improvements. > >>> > > >>> > Note that this is used in the Linux kernel for the same purpose. For > >>> > some idea as to potential benefits, see e.g > >>> > > http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html. > >>> > > >>> > Signed-off-by: Ganesh Ajjanagadde > >>> > --- > >>> > libavutil/attributes.h | 8 > >>> > 1 file changed, 8 insertions(+) > >>> > > >>> > diff --git a/libavutil/attributes.h b/libavutil/attributes.h > >>> > index 5c6b9de..1547033 100644 > >>> > --- a/libavutil/attributes.h > >>> > +++ b/libavutil/attributes.h > >>> > @@ -58,6 +58,14 @@ > >>> > #define av_warn_unused_result > >>> > #endif > >>> > > >>> > +#if AV_GCC_VERSION_AT_LEAST(3,0) > >>> > +#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 > >>> > + > >>> > #if AV_GCC_VERSION_AT_LEAST(3,1) > >>> > #define av_noinline __attribute__((noinline)) > >>> > #elif defined(_MSC_VER) > >>> > > >>> > >>> Ive used these builtins before and can confirm that they are useful > >>> (although requires devs to use them obviously). I will also point out > that > >>> these are also supported by ICC/ICL as well. > >> > >> They also make code ugly as fuck, and are more cargo-cult than anything > >> else. They might possibly be ok in some very critical parts of the > >> code, but otherwise not at all. > >> > > > > I agree with wm4 on the ugly aspect, I always hate reading code from > > projects that use it, its terrible on readability. > > If its used at all, it should be proven that (a) the function is > > performance relevant, and (b) that the improvement is actually > > tangible in the grand scheme. > > I mostly agree with the sentiments. Patchset shelved until a > non-trivial, critical use case can be found. I can think of a few. E.g. buffer overflow protection in bitstream reader (get_bits.h), maybe the arith readers (vp56.h, cabac.h) could also use these. These codepaths are actually quite performance sensitive and I'm not sure they're all running hand-coded assembly. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
On 25 February 2016 at 13:20, Ganesh Ajjanagadde wrote: > From: Ganesh Ajjanagadde > > These use __builtin_expect, and may be useful for optimizing nearly > certain branches, to yield size and/or speed improvements. > > Note that this is used in the Linux kernel for the same purpose. For > some idea as to potential benefits, see e.g > http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html. > > Signed-off-by: Ganesh Ajjanagadde > --- > libavutil/attributes.h | 8 > 1 file changed, 8 insertions(+) > > diff --git a/libavutil/attributes.h b/libavutil/attributes.h > index 5c6b9de..1547033 100644 > --- a/libavutil/attributes.h > +++ b/libavutil/attributes.h > @@ -58,6 +58,14 @@ > #define av_warn_unused_result > #endif > > +#if AV_GCC_VERSION_AT_LEAST(3,0) > +#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 > + > #if AV_GCC_VERSION_AT_LEAST(3,1) > #define av_noinline __attribute__((noinline)) > #elif defined(_MSC_VER) > Ive used these builtins before and can confirm that they are useful (although requires devs to use them obviously). I will also point out that these are also supported by ICC/ICL as well. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
On Thu, 25 Feb 2016 15:48:17 +1100 Matt Oliver wrote: > On 25 February 2016 at 13:20, Ganesh Ajjanagadde wrote: > > > From: Ganesh Ajjanagadde > > > > These use __builtin_expect, and may be useful for optimizing nearly > > certain branches, to yield size and/or speed improvements. > > > > Note that this is used in the Linux kernel for the same purpose. For > > some idea as to potential benefits, see e.g > > http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html. > > > > Signed-off-by: Ganesh Ajjanagadde > > --- > > libavutil/attributes.h | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/libavutil/attributes.h b/libavutil/attributes.h > > index 5c6b9de..1547033 100644 > > --- a/libavutil/attributes.h > > +++ b/libavutil/attributes.h > > @@ -58,6 +58,14 @@ > > #define av_warn_unused_result > > #endif > > > > +#if AV_GCC_VERSION_AT_LEAST(3,0) > > +#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 > > + > > #if AV_GCC_VERSION_AT_LEAST(3,1) > > #define av_noinline __attribute__((noinline)) > > #elif defined(_MSC_VER) > > > > Ive used these builtins before and can confirm that they are useful > (although requires devs to use them obviously). I will also point out that > these are also supported by ICC/ICL as well. They also make code ugly as fuck, and are more cargo-cult than anything else. They might possibly be ok in some very critical parts of the code, but otherwise not at all. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
On 2/25/2016 7:27 AM, wm4 wrote: > They also make code ugly as fuck, and are more cargo-cult than anything > else. They might possibly be ok in some very critical parts of the > code, but otherwise not at all. +1 to this. I will absolutely *not* ACK any patch sets that start littering likely/unlikely all over the codebase. I am indifferent to very limited use, but you must include tests showing it actually does something better. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely
On Thu, Feb 25, 2016 at 8:27 AM, wm4 wrote: > On Thu, 25 Feb 2016 15:48:17 +1100 > Matt Oliver wrote: > >> On 25 February 2016 at 13:20, Ganesh Ajjanagadde wrote: >> >> > From: Ganesh Ajjanagadde >> > >> > These use __builtin_expect, and may be useful for optimizing nearly >> > certain branches, to yield size and/or speed improvements. >> > >> > Note that this is used in the Linux kernel for the same purpose. For >> > some idea as to potential benefits, see e.g >> > http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html. >> > >> > Signed-off-by: Ganesh Ajjanagadde >> > --- >> > libavutil/attributes.h | 8 >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/libavutil/attributes.h b/libavutil/attributes.h >> > index 5c6b9de..1547033 100644 >> > --- a/libavutil/attributes.h >> > +++ b/libavutil/attributes.h >> > @@ -58,6 +58,14 @@ >> > #define av_warn_unused_result >> > #endif >> > >> > +#if AV_GCC_VERSION_AT_LEAST(3,0) >> > +#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 >> > + >> > #if AV_GCC_VERSION_AT_LEAST(3,1) >> > #define av_noinline __attribute__((noinline)) >> > #elif defined(_MSC_VER) >> > >> >> Ive used these builtins before and can confirm that they are useful >> (although requires devs to use them obviously). I will also point out that >> these are also supported by ICC/ICL as well. > > They also make code ugly as fuck, and are more cargo-cult than anything > else. They might possibly be ok in some very critical parts of the > code, but otherwise not at all. > I agree with wm4 on the ugly aspect, I always hate reading code from projects that use it, its terrible on readability. If its used at all, it should be proven that (a) the function is performance relevant, and (b) that the improvement is actually tangible in the grand scheme. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel