On Wed, May 15, 2019 at 9:21 PM Adam Richter <adamricht...@gmail.com> wrote: > > 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.
You seem to be totally overthinking this. I'm not concerned about code generation or anything like that, just the simple fact that I believe that the checks are more logical and actually easier to understand if they are logically grouped and combined. And I really don't see the advantage in any of these changes, personally. > > > > > > > > > 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. > Check the mailing list archives for previous discussion of using "likely"/"unlikely" and the likes. There is serious contention to their usefulness in general, so we should rather avoid making a precedent for them in some debug statements. - Hendrik _______________________________________________ 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".