[email protected] 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. * 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? 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.) > data[0] = '\0'; > > @@ -266,21 +272,7 @@ svn_stringbuf_create_ensure(apr_size_t b > svn_stringbuf_t * > svn_stringbuf_ncreate(const char *bytes, apr_size_t size, apr_pool_t *pool) > { > - char *data; > - > - /* 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. */ > - apr_size_t aligned_size = APR_ALIGN_DEFAULT(size + 1) - 1; > - > - /* Ensure string buffer of aligned_size + 1. > - * This should allocate the same amount of memory as "size" would. */ > - svn_stringbuf_t *strbuf = svn_stringbuf_create_ensure(aligned_size, pool); > - > - /* Actually, aligned_size+1 would be faster but we cannot be entirely > - * sure that the source string has been aligned properly such that > - * all the extra bytes would actually come from addressible memory.*/ > + svn_stringbuf_t *strbuf = svn_stringbuf_create_ensure(size, pool); > memcpy(strbuf->data, bytes, size); > > /* Null termination is the convention -- even if we suspect the data > @@ -385,12 +377,18 @@ svn_stringbuf_ensure(svn_stringbuf_t *st > if (str->blocksize < minimum_size) > { > if (str->blocksize == 0) > - str->blocksize = minimum_size; > + /* APR will increase odd allocation sizes to the next > + * multiple for 8, for instance. Take advantage of that > + * knowledge and allow for the extra size to be used. */ > + str->blocksize = APR_ALIGN_DEFAULT(minimum_size); > else > while (str->blocksize < minimum_size) > { > + /* str->blocksize is aligned; > + * doubling it should keep it aligned */ > apr_size_t prev_size = str->blocksize; > str->blocksize *= 2; > + > /* check for apr_size_t overflow */ > if (prev_size > str->blocksize) > { > >
