On 8/10/2020 9:14 PM, Andreas Rheinhardt wrote: > James Almer: >> On 8/10/2020 8:12 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 8/10/2020 7:11 PM, Nicolas George wrote: >>>>> James Almer (12020-08-10): >>>>>> Personally, i don't like it. It's extra complexity to save a single 8 or >>>>>> 12 byte allocation that happens once during bsf alloc. It's kind of a >>>>>> pointless micro-optimization. >>>>> >>>>> I do not agree at all. >>>>> >>>>> First, it is not extra complexity, it actually makes the code simpler: >>>>> less mutually dependant allocations that can lead to leaks if they are >>>>> not handled properly, better guarantees, for no more code. >>>> >>>> It adds an extra struct and makes the code harder to read. Might as well >>>> just do >>>> >>>> ctx = av_mallocz(sizeof(*ctx) + sizeof(AVBSFInternal)); >>>> ctx->internal = &ctx[1]; >>> >>> This is not ok due to padding/alignment. >> >> libdav1d would have broke by now if that was the case. Do you know a >> system where this could fail? >> > > Imagine the context to only contain elements that require a alignment of > 4 and the internal structure to require an alignment of eight. Then it > is perfectly possible for &ctx[1] to only have an alignment of four and > not of eight as internal requires it.
But is that the case here? AVBSFContext is 64 bytes on x86_64 (six 8 byte pointers, four ints, no padding), and AVBSFInternal is 16 (8 byte pointer properly aligned after AVBSFContext, 4 byte int, 4 byte padding). On 32 bit systems with 4 byte alignment for pointers, it would also be fine. In any case, I'm not suggesting to do this. I'd rather just not make any change at all. > > Could you elaborate the point about libdav1d? It uses this same approach to insert a byte array at the end of a context rather than a struct, so probably not the same situation. > >>> >>>> >>>> if removing one tiny allocation in an incredibly cold function is so >>>> important. Less code, same result. >>> >>> I don't see an obfuscation/complication (and also no problem with >>> maintainability); I see a very simple method to save an allocation. And >>> I actually think that it simplifies the code, because now one doesn't >>> have to worry at all any more whether internal has been properly >>> allocated or not in av_bsf_free(). >> >> I don't deny it simplifies the code in regards to freeing the context in >> failure paths, but adding a third struct does not make things clearer at >> all. So Mark's suggestion to add AVBSFContext into AVBSFInternal may be >> a better approach. >> > There are two reasons why I like my approach more than Mark's: It allows > to hide the details of the context<->internal relationship to the place > where allocations happen (and if one asserted as Nicolas suggested > (av_assert2(ctx->internal == &((AVBSFCombined *)ctx)->internal), it > would also need to be visible for the function doing the freeing, but > even then it can still be confined to one file). After all, the internal > of several important other structures are in private headers to be used > by lots of code. > The second point is another micro-optimization: If one puts the > AVBSFContext at the beginning of AVBSFInternal, then the offsets of all > the fields currently existing in AVBSFInternal will increase and the > encoding of the "read address from pointer + offset" is typically > shorter if offset is smaller. This is not a problem here as the > structures here are small, but other structures are bigger. This could > of course be overcome by putting the AVBSFContext at the end of > AVBSFInternal; but somehow this feels unnatural, although I could get > used to it. Instruction encoding for bigger offsets (>=128 on x86) is not an acceptable reason to choose one approach or another here. That is beyond ricing. This is not a hot SIMD loop in a video DSP function where every byte counts. > >>> Do you remember 31aafdac2404c5e01b21e53255db3fb5ed53c7c9 >>> (https://ffmpeg.org/pipermail/ffmpeg-devel/2019-October/251816.html) >>> where you fixed the bug that avformat_free_context() gets called upon >>> failure to allocate the AVFormatInternal despite avformat_free_context() >>> requiring the internal to be successfully allocated? That issue would >>> have never even existed if one allocated the context and its internal in >>> one piece. >>> >>> - Andreas > _______________________________________________ > 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". > _______________________________________________ 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".
