On 30.10.2010 15:09, Daniel Shahaf wrote:
> stefan2_at_apache.org wrote on Sat, Oct 30, 2010 at 12:09:49 -0000:
>> Author: stefan2
>> Date: Sat Oct 30 12:09:49 2010
>> New Revision: 1029038
>>
>> URL: http://svn.apache.org/viewvc?rev=1029038&view=rev
>> Log:
>> Improve on r1028104:
>> Move string buffer size alignment logic down the call stack.
>> Also, extend the logic to cover string growth as well.
>>
>> * subversion/libsvn_subr/svn_string.c
>> (svn_stringbuf_ncreate): remove alignment logic
>> (svn_stringbuf_create_ensure): add it here
>> (svn_stringbuf_ensure): and here
>>
>> Suggested by: Daniel Shahaf<d.s_at_daniel.shahaf.name>
>>
> You can say "Suggested by: danielsh" for people in COMMITTERS.
>
>> Modified:
>> subversion/branches/performance/subversion/libsvn_subr/svn_string.c
>>
>> Modified: subversion/branches/performance/subversion/libsvn_subr/svn_string.c
>> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/svn_string.c?rev=1029038&r1=1029037&r2=1029038&view=diff
>> ==============================================================================
>> --- subversion/branches/performance/subversion/libsvn_subr/svn_string.c (original)
>> +++ subversion/branches/performance/subversion/libsvn_subr/svn_string.c Sat Oct 30 12:09:49 2010
>> @@ -255,7 +255,13 @@ svn_stringbuf_create_empty(apr_pool_t *p
>> svn_stringbuf_t *
>> svn_stringbuf_create_ensure(apr_size_t blocksize, apr_pool_t *pool)
>> {
>> - char *data = apr_palloc(pool, ++blocksize); /* + space for '\0' */
>> + /* apr_palloc will allocate multiples of 8.
>> + * Thus, we would waste some of that memory if we stuck to the
>> + * smaller size. Note that this is safe even if apr_palloc would
>> + * use some other aligment or none at all. */
>> +
>> + blocksize = APR_ALIGN_DEFAULT(++blocksize); /* + space for '\0' */
>> + char *data = apr_palloc(pool, blocksize);
>>
> Two issues:
>
> * Declaration ('char *data') after statement ('blocksize = ...').
> We can't do this because we promise (and one buildbot enforces) C89
> compatibility. Please avoid this.
Can't we just move to C++? ;)
> * The blocksize line. It assigns to 'blocksize' and uses ++blocksize on
> the same line... and on top of that, it uses ++blocksize as an
> argument to a macro (and I don't know that that macro promises not to
> multiply-evaluate its argument). All in all, it touches 'blocksize'
> too many times for my poor brain. Could you change it to:
>
> blocksize = APR_ALIGN_DEFAULT(blocksize+1);
>
> please?
The ++part would have been ok for function calls because
the evaluation order is well-defined (assignment after r-value
calculation, increment before r-value calculation may access
blocksize).
However, you are right about the potential macro problem -
even if this macro will currently access the parameter only once.
Fixed in r1029090.
> Assuming these two changes, +1 to merge to trunk. (Feel free to just
> make the changes and merge them without waiting for another +1; if
> further review is necessary, it can be done on trunk.)
Merged in r1029108.
-- Stefan^2.
Received on 2010-10-30 18:26:08 CEST