[Ffmpeg-devel] [PATCH 8/9] Doxygenize av_fast_realloc comments

[Panagiotis Issaris]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Michael Niedermayer schreef:
> On Fri, Feb 23, 2007 at 01:55:57PM +0100, Panagiotis Issaris wrote:
>> Michael Niedermayer schreef:
>>> On Thu, Feb 22, 2007 at 10:32:31PM +0100, Panagiotis Issaris wrote:
>>>> Panagiotis Issaris schreef:
>>>>> Michael Niedermayer schreef:
>>>>>> On Thu, Feb 22, 2007 at 08:47:58PM +0100, Panagiotis Issaris wrote:
>>>> [...]
>>>>>>>> additionally, this function is a internal function of avcodec
>>>>>>>> and should not be used outside, it shouldnt have a av_ prefix  but ff_
>>>>>>>> and it should be droped if normal av_realloc() is as fast (benchmark a
>>>>>>>> large number of reallocs)
>>>>>>>>
>>>>>>>>      
>>>>>>> I?ll first prepare a patch for moving it and doing the s/av/ff/ 
>>>>>>> thing. Afterwards I'll post
>>>>>>> some benchmarking code so that others correct it and run the 
>>>>>>> benchmark too.
>>>>>>>    
>>>>>> hmm wouldnt it be better to benchmark it first before doing a possibly
>>>>>> redundant move/rename patch?
>>>>>>  
>>>>> Yes, but I thought it might take a while till consensus is reached on 
>>>>> whether it is faster or not, and
>>>>> that it might also take awhile before it gets benchmarked on enough 
>>>>> different architectures/CPUs.
>>>> A first attempt of some benchmarking code for av_fast_realloc() 
>>>> hideously embedded within FFmpeg's main function.
>>>>
>>>> I do not know if I'm benchmarking the correct type of usage of 
>>>> av_fast_realloc() vs av_realloc() nor if I am using
>>>> START_TIMER|STOP_TIMER in the way you had intended it...
>>>>
>>>> For each test of av_fast_realloc() I tested two runs, one with 
>>>> size<min_size and one with size>min_size, to test both
>>>> cases. (Besides the obvious av_realloc() run ofcourse.)
>>>>
>>>> _Obviously_ not intended for Subversion.
>>> you change the internal size variable for av_fast_realloc() so the result
>>> is meaningless at least as far as i can see, the size variable is never
>>> ever chnanged by anything except av_fast_realloc() itself
>> Ah! You are right, I didn't really know what the min_size parameter was
>> about, which is also why I didn't enhance the doxygen documentation.
>>
>> If I understand it correctly now, the idea is that you do an:
>> av_fast_realloc(ptr, &size, req_size);
>> Where the first time both size and req_size have the same value.
> 
> first time size is 0
Ah yes, you are right, there's no point in setting it the first time to
the same size.

>> av_fast_realloc() increases the value of size a bit to get better
>> performance (I guess). If the requested size in subsequent calls to
>> av_fast_realloc() is smaller than the previous requested sizes, it will
>> just return the pointer and not reallocate it (not realloc a smaller
>> buffer). So, you might "waste" some memory, right?
> 
> yes, the realloc to smaller generally doesnt happen in the code AFAIK
Ah, okay.

>> int size = 40000;
>> int req_size = 40000;
>> ptr = av_fast_realloc(ptr, &size, req_size);
>>
>> And the following code will be faster because no reallocation occurs:
>> req_size = 10;
>> ptr = av_fast_realloc(ptr, &size, req_size);
>>
>> And the code below should be a lot faster with av_fast_realloc()
>> then with a plain av_realloc() because no reallocation occurs
>> because size is still > 40000, right?
> 
> yes
> 
> 
>> req_size = 40000;
>> ptr = av_fast_realloc(ptr, &size, req_size);
>>
>> *size= FFMAX(17*min_size/16 + 32, min_size);
>> Why is the maximum of both taken? Isn't the first part always bigger
>> than the second?
> 
> you forgetting the possibility of overflows
> 
> [...]
Ah! Thanks for the explanation!

If after benchmarking the code appears to be worthy to stay in SVN,
I'd like to add a comment such as this:
/* FFMAX is used here to protect against overflows */


I'll update the benchmarking patch with my new acquired knowledge and
hopefully help determining the performance benefits of av_fast_realloc.

With friendly regards,
Takis
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF3upY9kOxLuzz4CkRAr1AAJ0WKczYNAIVZsbatzc38b2BCEWJKACdEj2g
LC8A+D4KY6kjI3U4StMiexM=
=NM7o
-----END PGP SIGNATURE-----