On 25.08.2015 14:10, Branko Čibej wrote: > On 06.08.2015 18:10, stef...@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
