Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely

[Ronald S. Bultje]

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

[Ganesh Ajjanagadde]

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

[Hendrik Leppkes]

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely

[Derek Buitenhuis]

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

[wm4]

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

[Matt Oliver]

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

[FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely

[Ganesh Ajjanagadde]

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