On Tue, May 14, 2019 at 6:48 PM myp...@gmail.com <myp...@gmail.com> wrote:
>
> On Wed, May 15, 2019 at 7:01 AM Hendrik Leppkes <h.lepp...@gmail.com> wrote:
> >
> > On Tue, May 14, 2019 at 11:25 PM Adam Richter <adamricht...@gmail.com>
> > wrote:
> > >
> > > Consider, for example, if you agree that columnization makes this range
> > > check
> > > more recognizable in a glance and makes it easier to spot what the bounds
> > > are
> > > (the sixteen space indentation is taken from the code in which it
> > > appeared):
> > >
> > > av_assert0(par->bits_per_coded_sample >= 0 &&
> > > par->bits_per_coded_sample <= 8);
> > >
> > > ...vs...
> > >
> > > av_assert0(par->bits_per_coded_sample >= 0);
> > > av_assert0(par->bits_per_coded_sample <= 8);
> > >
> > > A possible counter-argument to this might be that, in a long sequence
> > > of assertions, can be informative to group related assertions
> > > together, which I think is true, but it is possible to get this other
> > > means, such as by using blank lines to separate express such grouping.
> > >
> > > So, Mark, if you decide you are OK with my complete patches, I encourage
> > > you to let me know. Otherwise, if there are any of those changes which
> > > you
> > > are OK with, I would like to just to to get those merged. Finally, if
> > > you would
> > > rather see none of those changes merged (one one to split the assertions
> > > in
> > > libavformat and one was for everywhere else in ffmpeg), please let me know
> > > about that too, in which case, if no one advocates for their
> > > inclusion, I'll drop
> > > my proposal to merge these changes.
> > >
> >
> > Unfortunately I have to agree with Mark. asserst that check the same
> > value or extremely closely related values should stay combined.
> >
> I agree this part
I am trying to understand what problem you see with this.
It occurs to me that you might be concerned about generating less
efficient code for the common assert success case, in particular,
in the example I showed for readability, potentially dereferencing
"par" twice, but I made a test program (attached) and determined
from reading the generated assembly that at least for gcc with
optimization -O2 on x86_64, x86_32, aarch64 and arm32, as
long as the abort function has "__attribute__ ((noreturn))", the
compiler seems to be able to avoid repeating the memory fetch
for the second assertion. I also check this for clang -O2 on
on x86_64.
Of course, without the noreturn attribute on the abort function,
the compiler realizes that the abort function could have written
to memory, so it refetches the value in the split assertion case,
which I think might have been your concern, but as long as
the abort function is declared with an attribute noreturn, we
should be OK for gcc.
On the other hand, I'm not sure what compilers people use
for other platforms such as Microsoft Windows and if you tell
me that it is a known problem on a specific platform that is
potentially relevant to ffmpeg, that would probably change my
mind.
Of course, it's not necessary for you change my mind or for
you to invest further time in this discussion, as I imagine you
and other participants have other things to do. So, if I don't
get a further explanation, I may still believe that you're wrong,
but I'll respect your need to prioritize tasks other than continuing
this discussion, and will not expect to see my proposed change
merged unless the predominant opinion of others in this discussion
changes to being in favor it, which, so far, I acknowledge, it is not.
> > >
> > > Also after this, I may take a look at adding a branch hint to the
> > > av_assertN
> > > macros if nobody objects.
> > >
> >
> > Please don't, we don't do branch hints anywhere, and this is a bad
> > place to start.
> > If an assert is truely performance sensitive (as in, it makes a
> > measurable difference), it should probably be moved from assert0 to
> > assert1, and as such is only enabled in testing builds.
> >
> If assert0 or assert1 lead to performance drop, we need some profiling
> data, then try to fix it.
The above comments by Hendrick and you does not identify a cost to
adding a branch hint to assert. There would be a downside in the form of
a few lines of code complexity in libavutil/avassert.h. If that is
your concern,
I guess our disagreement is that I see reducing the cost of assert so that
perhaps CPU usage with and without would be a tiny bit more similar for
reproducing problems and maybe (I'm not saying it's likely) it might tip a
trade-off in favor of keeping an assert enabled in some borderline
circumstance. I'd take that trade (add the branch prediction).
If you want to educate me on some other reason why I may be wrong,
about adding the branch prediction for assertion checks, I'd been keen
to know, but I realize everyone's time is limited, and if I haven't
convinced you and also created consensus in favor of adding the
branch prediction to assertion checking, then I don't expect to advocate
further on this list for merging such a change into your tree at this time.
Thanks to everyone who has participated in this discussion for your input.
Adam
extern void my_assert_fail(const char *file, const char *func, int line,
const char *condition) __attribute__ ((noreturn));
extern int my_array[];
#define expect_true(condition) __builtin_expect((condition), 1)
#define my_assert(condition) \
do { \
if (!(condition)) { \
my_assert_fail(__FILE__, __func__, __LINE__, \
#condition); \
} \
} while (0)
void myfunc_combined(void)
{
my_assert(my_array[0] != 100 && my_array[0] != 102);
}
void myfunc_split(void)
{
my_assert(my_array[0] != 100);
my_assert(my_array[0] != 102);
}
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
