On 25.08.2015 14:10, Branko Čibej wrote:
> On 06.08.2015 18:10, stefan2_at_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
Received on 2015-08-25 16:31:22 CEST