subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@wandisco.com>
Subject Re: svn commit: r1694533 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Date Tue, 25 Aug 2015 14:23:44 GMT
On 25.08.2015 14:10, Branko ─îibej wrote:
> On 06.08.2015 18:10, stefan2@apache.org wrote:
>> Author: stefan2
>> Date: Thu Aug  6 16:10:39 2015
>> New Revision: 1694533
>>
>> URL: http://svn.apache.org/r1694533
>> Log:
>> Fix an alignment problem on machines with 32 bit pointers but atomic 64
>> data access that may not be misaligned.
>>
>> * subversion/libsvn_ra_svn/marshal.c
>>   (read_item): Ensure that the array contents are always have the APR
>>                default alignment.
>>
>> Found by: Rainer Jung <rainer.jung{_AT_}kippdata.de>
>>
>> Modified:
>>     subversion/trunk/subversion/libsvn_ra_svn/marshal.c
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1694533&r1=1694532&r2=1694533&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Thu Aug  6 16:10:39 2015
>> @@ -1190,14 +1190,20 @@ static svn_error_t *read_item(svn_ra_svn
>>      }
>>    else if (c == '(')
>>      {
>> +      /* On machines with 32 bit pointers, array headers are only 20 bytes
>> +       * which is not enough for our standard 64 bit alignment.
>> +       * So, determine a suitable block size for the APR array header that
>> +       * keeps proper alignment for following structs. */
>> +      const apr_size_t header_size
>> +        = APR_ALIGN_DEFAULT(sizeof(apr_array_header_t));
>> +
>>        /* Allocate an APR array with room for (initially) 4 items.
>>         * We do this manually because lists are the most frequent protocol
>>         * element, often used to frame a single, optional value.  We save
>>         * about 20% of total protocol handling time. */
>> -      char *buffer = apr_palloc(pool, sizeof(apr_array_header_t)
>> -                                      + 4 * sizeof(svn_ra_svn_item_t));
>> -      svn_ra_svn_item_t *data
>> -        = (svn_ra_svn_item_t *)(buffer + sizeof(apr_array_header_t));
>> +      char *buffer = apr_palloc(pool,
>> +                                header_size + 4 * sizeof(svn_ra_svn_item_t));
>> +      svn_ra_svn_item_t *data = (svn_ra_svn_item_t *)(buffer + header_size);
>>  
>>        item->kind = SVN_RA_SVN_LIST;
>>        item->u.list = (apr_array_header_t *)buffer;
>
> How exactly is all this different from:
>
>         apr_array_make(pool, 4, sizeof(svn_ra_svn_item_t)?
>
> Other than relying on magic knowledge of APR implementation details?

All right, so I figured that the difference is that apr_array_make does
two allocations compared to one in this code. Still: relying on
knowledge of APR implementation details is a really bad idea. As it
stands, APR will correctly resize this array if necessary; but there's
no guarantee that, sometime in the future, resizing such arrays would
fail horribly.

I very strongly suggest that we put a plain ol' apr_array_make here and,
if this is really perceived as such a huge *overall* performance
problem, put the allocation hack into APR itself.

I suspect that apr_palloc is fast enough that we really don't need this
hack.

-- Brane

Mime
View raw message