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.
* 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)
> {
>
>
Received on 2010-10-30 15:11:59 CEST